Skip to content

Performance optimizations #1 #12171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 48 commits into from
Jun 15, 2017
Merged

Performance optimizations #1 #12171

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
033aa88
Fixed callable call in loop termination condition
frankmayer Sep 25, 2016
a650515
Fixed non-optimal regular expression
frankmayer Sep 25, 2016
a33ad89
Shortened syntax for applied operations
frankmayer Sep 25, 2016
4173a7f
Don't use strlen() to check if string is empty.
frankmayer Sep 25, 2016
186f975
Reverted preg changes. Look into those later again.
frankmayer Sep 25, 2016
19eb362
Replaced call_user_func_array() with call_user_func()
frankmayer Sep 25, 2016
f9cb3fe
Merge unset() calls
frankmayer Sep 25, 2016
d30d1d7
Replaced old intval() call with modern typecasting
frankmayer Sep 25, 2016
0d1f128
Optimize away substr() as index-based access
frankmayer Sep 25, 2016
321c813
Merge branch 'Performance_2' into Performance_1
frankmayer Sep 25, 2016
d30fe28
Replace some cases of substr() with strpos()
frankmayer Sep 25, 2016
5dfd4e9
Replace some stristr() with stripos()
frankmayer Sep 25, 2016
4c1cfdd
Replace is_null() with null === ...
frankmayer Sep 25, 2016
271d02a
Removed unnecessary ternary operators
frankmayer Sep 25, 2016
c500e83
Fix for Juri...
frankmayer Sep 25, 2016
44d234d
Revert some changes for ... PHP5.3 compatibility
frankmayer Sep 25, 2016
6fb499b
Make travis happy ....
frankmayer Sep 25, 2016
5ada9b9
Make codesniffer happy...
frankmayer Sep 25, 2016
c2fd895
DYC!
frankmayer Sep 25, 2016
de1a93e
Optimize non-optimal if conditions.
frankmayer Sep 25, 2016
2aa353b
codsniffer...
frankmayer Sep 25, 2016
307cd94
Inline one-time use variables
frankmayer Sep 25, 2016
062ef22
Codesniffer...
frankmayer Sep 25, 2016
1bc9d8f
Flipped comparisons
frankmayer Sep 28, 2016
7d287f0
Flipped comparisons
frankmayer Sep 28, 2016
53d4854
Fix removal of ternary operator. Forgot inversion.
frankmayer Sep 28, 2016
5f6d634
More flipping of comparisons ;)
frankmayer Sep 28, 2016
2e2df19
Removed another unnecessary set of ternary operators.
frankmayer Sep 28, 2016
6b3e0fb
Codesniffer...
frankmayer Sep 28, 2016
171f66b
Type-safe comparisons on strings
frankmayer Oct 4, 2016
a730ca0
More type-safe comparisons
frankmayer Oct 4, 2016
1ff621a
Changes that occurred during PR discussion
frankmayer Oct 4, 2016
ba6ac8a
Merge branch 'staging' into Performance_1
frankmayer Dec 12, 2016
c67b374
Remove some unnecessary parentheses
frankmayer Dec 12, 2016
7e02da0
Removed empty if-group and reversed condition
frankmayer Dec 12, 2016
a924e5c
Merge branch 'staging' into Performance_1
frankmayer Dec 15, 2016
e0b11c7
Merge branch 'staging' into Performance_1
frankmayer May 29, 2017
22fc1c4
Merge branch 'staging' into Performance_1
frankmayer Jun 13, 2017
4b30c17
Corrections on errors during merge (Conflict resolving)
frankmayer Jun 13, 2017
e3f8617
Missed one.
frankmayer Jun 13, 2017
7d7b3cd
Remove a parenthesis
frankmayer Jun 13, 2017
42a6146
Changes according to reviewer's comment
frankmayer Jun 13, 2017
cb5ee93
Revert change because of failure
frankmayer Jun 13, 2017
bb10022
Changes according to reviewer's comment
frankmayer Jun 13, 2017
c4157d9
reversed change, though according to the method's doc that value shou…
frankmayer Jun 13, 2017
ec2c21a
Hopefully fixed...
frankmayer Jun 13, 2017
c0d3b44
Hopefully fixed...
frankmayer Jun 13, 2017
3619945
Re-apply some changes
frankmayer Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/cms/application/administrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ public static function purgeMessages()
$config = $db->loadObject();

// Check if auto_purge value set
if (is_object($config) and $config->cfg_name == 'auto_purge')
if (is_object($config) && $config->cfg_name === 'auto_purge')
{
$purge = $config->cfg_value;
}
Expand Down
10 changes: 5 additions & 5 deletions libraries/cms/application/cms.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,13 @@ public function __construct(JInput $input = null, Registry $config = null, JAppl
}

// Enable sessions by default.
if (is_null($this->config->get('session')))
if ($this->config->get('session') === null)
{
$this->config->set('session', true);
}

// Set the session default name.
if (is_null($this->config->get('session_name')))
if ($this->config->get('session_name') === null)
{
$this->config->set('session_name', $this->getName());
}
Expand Down Expand Up @@ -231,7 +231,7 @@ public function checkSession()
public function enqueueMessage($msg, $type = 'message')
{
// Don't add empty messages.
if (!strlen(trim($msg)))
if (trim($msg) === '')
{
return;
}
Expand Down Expand Up @@ -581,7 +581,7 @@ public function getUserState($key, $default = null)
$session = JFactory::getSession();
$registry = $session->get('registry');

if (!is_null($registry))
if ($registry !== null)
{
return $registry->get($key, $default);
}
Expand Down Expand Up @@ -1137,7 +1137,7 @@ public function setUserState($key, $value)
$session = JFactory::getSession();
$registry = $session->get('registry');

if (!is_null($registry))
if ($registry !== null)
{
return $registry->set($key, $value);
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/application/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public static function getClientInfo($id = null, $byName = false)
}

// If no client id has been passed return the whole array
if (is_null($id))
if ($id === null)
{
return self::$_clients;
}
Expand Down
10 changes: 4 additions & 6 deletions libraries/cms/application/site.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,7 @@ public function getLanguageFilter()
*/
public function getMenu($name = 'site', $options = array())
{
$menu = parent::getMenu($name, $options);

return $menu;
return parent::getMenu($name, $options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find the reference at the moment, but I remember reading something along the lines that storing a function return in a variable and returning that variable in your method is less error prone and faster than directly returning an evaluated function in your method.

Technically they both work, and off the top of my head I don't think we have a standard on which way to write code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, such kind of variables should not be introduced.

They serve no practical purpose. And they do actually introduce another unnecessary step in our minds, when we humans read the code - and also when the machine aka the interpreter/compiler reads and processes it.

Why introduce something whose only purpose is to be returned on the next line, without any further processing?

As for the compiler, I would think modern PHP compilers do this step automatically internally, but they have to do it nevertheless, using cpu and memory, so why not leave it out in the first place?

Just my 2 cents 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of code readability I've been leaning toward less creation/assignment of variables that aren't needed than "explicit" structures like this one. There isn't a standard since both are valid syntax, but unless there is a majorly strong argument (like engine performance) for one over the other, I'd lean toward the readability argument and I find it easier to read one line instead of two code lines and the blank line in the middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, there are some interesting numbers here on inlining and redundant variables. These numbers depict different implementations in loops, but you can see the difference, which is even bigger in 7.0 (than even the latest 5.6), when using redundant variables.
Of course, it's not like Joomla is looping several thousands of times through these functions to create some content, but why not save a few bytes on memory and also a few cpu cycles? We can only benefit from it. ... + it's easier to read...

Copy link
Contributor Author

@frankmayer frankmayer Sep 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@photodude I think you were possibly referring to this q&a...

Copy link
Contributor

@photodude photodude Sep 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might have been it, but maybe not. I just checked some of my book references to see if it was in one of those books, and it seems the general consensus in the examples leans towards the inlining the return. (but nothing specific was stated in those books)

The argument of code readability is a good one.

It might be one of those specific circumstances things. Like ternary operators are ok in php, unless there is large objects in the variables being passed, then ternary operators in php suck up time and memory due to the copy operation they preform on the variables before evaluating.

In anycase I can't seem to find the reference at the moment, so Code Readability wins me over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankmayer I think part of what I remember was something like this comment on the PHP manual
http://php.net/manual/en/function.pg-connect.php#82117
considering that it's 8 years old I'm not sure if it still applies in more modern php

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@photodude I see what you mean. The case of the comment is partially true (at least with PHP7 which I just tested). In PHP7, it will die() even inline. However, if the returned value is not false, it will return a bool, which automatically will be true.
So this is actually a case where caution is advised and those cases should be dealt with, the old way.

}

/**
Expand Down Expand Up @@ -618,7 +616,7 @@ protected function initialiseApp($options = array())
}
}

if ($this->getLanguageFilter() && empty($options['language']))
if (empty($options['language']) && $this->getLanguageFilter())
{
// Detect cookie language
$lang = $this->input->cookie->get(md5($this->get('secret') . 'language'), null, 'string');
Expand All @@ -642,7 +640,7 @@ protected function initialiseApp($options = array())
}
}

if ($this->getDetectBrowser() && empty($options['language']))
if (empty($options['language']) && $this->getDetectBrowser())
{
// Detect browser language
$lang = JLanguageHelper::detectLanguage();
Expand Down Expand Up @@ -744,7 +742,7 @@ protected function render()
$template = $this->getTemplate(true);
$file = $this->input->get('tmpl', 'index');

if (!$this->get('offline') && ($file == 'offline'))
if (($file === 'offline') && !$this->get('offline'))
{
$this->set('themeFile', 'index.php');
}
Expand Down
4 changes: 2 additions & 2 deletions libraries/cms/captcha/captcha.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public function initialise($id)
public function display($name, $id, $class = '')
{
// Check if captcha is already loaded.
if (is_null($this->_captcha))
if ($this->_captcha === null)
{
return;
}
Expand Down Expand Up @@ -188,7 +188,7 @@ public function display($name, $id, $class = '')
public function checkAnswer($code)
{
// Check if captcha is already loaded
if (is_null(($this->_captcha)))
if ($this->_captcha === null)
{
return;
}
Expand Down
11 changes: 3 additions & 8 deletions libraries/cms/component/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ public static function getComponent($option, $strict = false)
*/
public static function isEnabled($option)
{
$result = static::getComponent($option, true);

return $result->enabled;
return static::getComponent($option, true)->enabled;
}

/**
Expand Down Expand Up @@ -117,9 +115,7 @@ public static function isInstalled($option)
*/
public static function getParams($option, $strict = false)
{
$component = static::getComponent($option, $strict);

return $component->params;
return static::getComponent($option, $strict)->params;
}

/**
Expand Down Expand Up @@ -403,9 +399,8 @@ protected static function executeComponent($path)
{
ob_start();
require_once $path;
$contents = ob_get_clean();

return $contents;
return ob_get_clean();
}

/**
Expand Down
10 changes: 5 additions & 5 deletions libraries/cms/editor/editor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ public function detach($observer)
public function initialise()
{
// Check if editor is already loaded
if (is_null(($this->_editor)))
if ($this->_editor === null)
{
return;
}
Expand All @@ -261,7 +261,7 @@ public function initialise()

$document = JFactory::getDocument();

if (method_exists($document, "addCustomTag") && !empty($return))
if (!empty($return) && method_exists($document, 'addCustomTag'))
{
$document->addCustomTag($return);
}
Expand Down Expand Up @@ -293,7 +293,7 @@ public function display($name, $html, $width, $height, $col, $row, $buttons = tr
$this->_loadEditor($params);

// Check whether editor is already loaded
if (is_null(($this->_editor)))
if ($this->_editor === null)
{
JFactory::getApplication()->enqueueMessage(JText::_('JLIB_NO_EDITOR_PLUGIN_PUBLISHED'), 'error');

Expand Down Expand Up @@ -344,7 +344,7 @@ public function save($editor)
$this->_loadEditor();

// Check whether editor is already loaded
if (is_null(($this->_editor)))
if ($this->_editor === null)
{
return;
}
Expand Down Expand Up @@ -508,7 +508,7 @@ public function getButtons($editor, $buttons = true)
protected function _loadEditor($config = array())
{
// Check whether editor is already loaded
if (!is_null(($this->_editor)))
if ($this->_editor !== null)
{
return;
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/form/field/captcha.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public function setup(SimpleXMLElement $element, $value, $group = null)

if (strpos($this->class, 'required') === false)
{
$this->class = $this->class . ' required';
$this->class .= ' required';
}
}

Expand Down
3 changes: 1 addition & 2 deletions libraries/cms/form/field/chromestyle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,7 @@ protected function getTemplates()

// Set the query and load the templates.
$db->setQuery($query);
$templates = $db->loadObjectList('element');

return $templates;
return $db->loadObjectList('element');
}
}
2 changes: 1 addition & 1 deletion libraries/cms/form/field/editor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ protected function getEditor()
}

// Create the JEditor instance based on the given editor.
if (is_null($editor))
if ($editor === null)
{
$conf = JFactory::getConfig();
$editor = $conf->get('editor');
Expand Down
4 changes: 1 addition & 3 deletions libraries/cms/form/field/frontend_language.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,9 @@ protected function getOptions()
}

// Merge any additional options in the XML definition.
$options = array_merge(
return array_merge(
parent::getOptions(),
$languages
);

return $options;
}
}
4 changes: 1 addition & 3 deletions libraries/cms/form/field/menu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ protected function getOptions()
}

// Merge any additional options in the XML definition.
$options = array_merge(parent::getOptions(), $menus);

return $options;
return array_merge(parent::getOptions(), $menus);
}
}
2 changes: 1 addition & 1 deletion libraries/cms/form/field/tag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ protected function prepareOptionsNested(&$options)
*/
public function isNested()
{
if (is_null($this->isNested))
if ($this->isNested === null)
{
// If mode="nested" || ( mode not set & config = nested )
if ((isset($this->element['mode']) && $this->element['mode'] == 'nested')
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/form/rule/notequals.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public function test(SimpleXMLElement $element, $value, $group = null, Registry
throw new UnexpectedValueException(sprintf('$field empty in %s::test', get_class($this)));
}

if (is_null($input))
if ($input === null)
{
throw new InvalidArgumentException(sprintf('The value for $input must not be null in %s', get_class($this)));
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/help/help.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public static function createUrl($ref, $useComponent = false, $override = null,
$local = false;
$app = JFactory::getApplication();

if (is_null($component))
if ($component === null)
{
$component = JApplicationHelper::getComponentName();
}
Expand Down
6 changes: 2 additions & 4 deletions libraries/cms/helper/content.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public static function _getActions($categoryId = 0, $id = 0, $assetName = '')
public static function getActions($component = '', $section = '', $id = 0)
{
// Check for deprecated arguments order
if (is_int($component) || is_null($component))
if (is_int($component) || $component === null)
{
$result = self::_getActions($component, $section, $id);

Expand Down Expand Up @@ -174,9 +174,7 @@ public static function getLanguageId($langCode)
->where($db->quoteName('lang_code') . ' = ' . $db->quote($langCode));
$db->setQuery($query);

$id = $db->loadResult();

return $id;
return $db->loadResult();
}

/**
Expand Down
4 changes: 1 addition & 3 deletions libraries/cms/helper/helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ public function getLanguageId($langCode)
->where($db->quoteName('lang_code') . ' = ' . $db->quote($langCode));
$db->setQuery($query);

$id = $db->loadResult();

return $id;
return $db->loadResult();
}

/**
Expand Down
10 changes: 5 additions & 5 deletions libraries/cms/helper/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public function canUpload($file, $component = 'com_media')
$finfo = finfo_open(FILEINFO_MIME);
$type = finfo_file($finfo, $file['tmp_name']);

if (strlen($type) && !in_array($type, $allowed_mime) && in_array($type, $illegal_mime))
if ($type !== '' && !in_array($type, $allowed_mime) && in_array($type, $illegal_mime))
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_MIME'), 'notice');

Expand All @@ -174,7 +174,7 @@ public function canUpload($file, $component = 'com_media')
// We have mime magic.
$type = mime_content_type($file['tmp_name']);

if (strlen($type) && !in_array($type, $allowed_mime) && in_array($type, $illegal_mime))
if ($type !== '' && !in_array($type, $allowed_mime) && in_array($type, $illegal_mime))
{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNINVALID_MIME'), 'notice');

Expand Down Expand Up @@ -206,7 +206,7 @@ public function canUpload($file, $component = 'com_media')
foreach ($html_tags as $tag)
{
// A tag is '<tagname ', so we need to add < and a space or '<tagname>'
if (stristr($xss_check, '<' . $tag . ' ') || stristr($xss_check, '<' . $tag . '>'))
if (false !== stripos($xss_check, '<' . $tag . ' ') || false !== stripos($xss_check, '<' . $tag . '>'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankmayer regarding the flipping (my ocd) this might be changed to ...
if (stripos($xss_check, '<' . $tag . ' ') !== false [...]
... as well, if you may.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matrikular whoops! sorry, those ones slipped through... will do

{
$app->enqueueMessage(JText::_('JLIB_MEDIA_ERROR_WARNIEXSS'), 'notice');

Expand Down Expand Up @@ -271,13 +271,13 @@ public function countFiles($dir)

while (false !== ($entry = $d->read()))
{
if (substr($entry, 0, 1) != '.' && is_file($dir . DIRECTORY_SEPARATOR . $entry)
if ($entry[0] !== '.' && is_file($dir . DIRECTORY_SEPARATOR . $entry)
&& strpos($entry, '.html') === false && strpos($entry, '.php') === false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO is_file should be the last check sine it makes I/O call to the filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, missed that one. Nice catch.

{
$total_file++;
}

if (substr($entry, 0, 1) != '.' && is_dir($dir . DIRECTORY_SEPARATOR . $entry))
if ($entry[0] !== '.' && is_dir($dir . DIRECTORY_SEPARATOR . $entry))
{
$total_dir++;
}
Expand Down
4 changes: 2 additions & 2 deletions libraries/cms/helper/route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ protected function findItem($needles = array())
$language = isset($needles['language']) ? $needles['language'] : '*';

// $this->extension may not be set if coming from a static method, check it
if (is_null($this->extension))
if ($this->extension === null)
{
$this->extension = $app->input->getCmd('option');
}
Expand Down Expand Up @@ -180,7 +180,7 @@ protected function findItem($needles = array())
* $language != * can override existing entries
* $language == * cannot override existing entries
*/
if (!isset(static::$lookup[$language][$view][$item->query['id']]) || $item->language != '*')
if ($item->language != '*' || !isset(static::$lookup[$language][$view][$item->query['id']]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed one !== here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a lot actually... not sure if I was so eager to include that many changes in that first PR.

{
static::$lookup[$language][$view][$item->query['id']] = $item->id;
}
Expand Down
2 changes: 1 addition & 1 deletion libraries/cms/helper/tags.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public function createTagsFromField($tags)
foreach ($tags as $key => $tag)
{
// User is not allowed to create tags, so don't create.
if (strpos($tag, '#new#') !== false && !$canCreate)
if (!$canCreate && strpos($tag, '#new#') !== false)
{
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions libraries/cms/helper/usergroups.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public function count()
*/
public static function getInstance()
{
if (null === static::$instance)
if (static::$instance === null)
{
// Only here to avoid code style issues...
$groups = array();
Expand Down Expand Up @@ -189,7 +189,7 @@ private function isSingleton()
*/
public function total()
{
if (null === $this->total)
if ($this->total === null)
{
$db = JFactory::getDbo();

Expand Down
Loading