Performance optimizations #1 by frankmayer · Pull Request #12171 · joomla/joomla-cms

frankmayer · 2016-09-25T16:18:28Z

[UPDATED (5.1.2017) - see second paragraph]
Those are some performance optimizations for the libraries/cms folder. If you like those, there are plenty more where those came from.
Since I don't know if you guys are open to these kinds of PR's. I am awaiting the developments of my open PR's before I do more.

[Update 5.1.2017]
In order to lighten this PR up, I have introduced 6 sub-PRs, which are easier to digest and mostly with very specific changes. In order to continue with this one, those ones should be tested/reviewed/merged first. After that, I can resolve the conflicts and there should finally be only a few changes left.

1) Simplify some ternary operations using elvis operator and remove unnecessary parentheses in libraries/cms Simplify some ternary operations using elvis operator and remove unnecessary parentheses in libraries/cms #13234
2) Replace unnecessary double quotes in libraries/cms Replace unnecessary double quotes in libraries/cms #13235
3) Remove one-time-use variables in libraries/cms Remove one-time-use variables in libraries/cms #13236
4) Various changes in libraries/cms Various changes in libraries/cms #13238
5) Type safe comparison of strings only, in libraries/cms Type safe comparison of strings only, in libraries/cms #13275
6) Type safe comparison in libraries/cms - second iteration Type safe comparison in libraries/cms - second iteration #13276

zero-24

looks good from here. Thanks!

zero-24

Please have a look at: https://travis-ci.org/joomla/joomla-cms/jobs/162592381 It looks like travis is not happy.

zero-24 · 2016-09-25T16:39:23Z

Travis error is:

1) JComponentRouterLegacyTest::testParse
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'option' => 'com_test'
-    42 => 'test-test'
+    42 => 'test:test'
 )
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/cms/component/router/JComponentRouterLegacyTest.php:130
2) JRouterTest::testDecodeSegments with data set #3 (array('42-test', 'testing-this-method'), array('42:test', 'testing:this-method'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => '42:test'
-    1 => 'testing:this-method'
+    1 => 'testing:this:method'
 )
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/cms/router/JRouterTest.php:872

https://travis-ci.org/joomla/joomla-cms/jobs/162592381

frankmayer · 2016-09-25T16:43:56Z

Yes, I noticed... :) Will fix errors.

frankmayer · 2016-09-25T18:12:25Z

on it...

andrepereiradasilva

some minor comments

andrepereiradasilva · 2017-06-13T18:44:44Z

libraries/cms/helper/media.php

 			{
-				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)


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

Yes, missed that one. Nice catch.

andrepereiradasilva · 2017-06-13T18:46:15Z

libraries/cms/html/tabs.php


 defined('JPATH_PLATFORM') or die;

+


please remvoe this extra line

andrepereiradasilva · 2017-06-13T18:49:32Z

libraries/cms/plugin/plugin.php

-			$appProperty = $reflection->getProperty('app');

-			if ($appProperty->isPrivate() === false && is_null($this->app))
+			if ($reflection->getProperty('app')->isPrivate() === false && $this->app === null)


change the order of the conditions?

andrepereiradasilva · 2017-06-13T18:49:40Z

libraries/cms/plugin/plugin.php

-			$dbProperty = $reflection->getProperty('db');

-			if ($dbProperty->isPrivate() === false && is_null($this->db))
+			if ($reflection->getProperty('db')->isPrivate() === false && $this->db === null)


change the order of the conditions?

andrepereiradasilva · 2017-06-13T19:28:44Z

I have tested this item ✅ successfully on 171f66b

code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12171.}

peteruoi · 2017-06-13T19:43:20Z

Just an idea...
If there is a considerable performance in joomla paging loading maybe a metric could be done with and without all these performance prs and if is enough it could be advertises in joomla 3.8 release campaign?
thanks frankmayer for your work!

frankmayer · 2017-06-13T19:58:21Z

@peteruoi that is an interesting idea. However I assume that any improvements from all the PRs will vary a lot with mileage. Unfortunately I haven't got the time to set this up and do the tests.
Oh, and a lot of things made it already into 3.7.

Another situation where I would hope to see some measurable optimization would be, if the indexer performance PR would be merged (see #13511).
This would of course mostly be measureable with initial (or re-) indexing of a lot of index-able content.
But I also would like to think that on changing of content, it might work a little bit faster.

Quy · 2017-06-13T20:00:57Z

libraries/cms/html/menu.php

 			array(
 				'id' => isset($config['id']) ? $config['id'] : 'assetgroups_' . (++$count),
-				'list.attr' => (is_null($attribs) ? 'class="inputbox" size="1"' : $attribs),
+				'list.attr' => ($attribs === null ? 'class="inputbox" size="1"' : $attribs),


Remove parentheses. Should I report them here?

Yes, that's just the right place ;)

Got more? Shall I wait?

Also align =

More reported and I am done for now. 😉

Thanks. Done a few more things I found along the way.

Quy · 2017-06-13T20:04:29Z

libraries/cms/pagination/pagination.php

 	{
 		// Allow to receive a null layout
-		$layoutId = (null === $layoutId) ? 'joomla.pagination.links' : $layoutId;
+		$layoutId = ($layoutId === null) ? 'joomla.pagination.links' : $layoutId;


Remove parentheses and more below.

Quy · 2017-06-13T20:05:38Z

libraries/cms/table/contenthistory.php

 				foreach ($value as $subName => $subValue)
 				{
-					$object->$subName = (is_int($subValue) || is_bool($subValue) || is_null($subValue)) ? (string) $subValue : $subValue;
+					$object->$subName = (is_int($subValue) || is_bool($subValue) || $subValue === null) ? (string) $subValue : $subValue;


Remove parentheses

Quy · 2017-06-13T20:05:45Z

libraries/cms/table/contenthistory.php

 			else
 			{
-				$object->$name = (is_int($value) || is_bool($value) || is_null($value)) ? (string) $value : $value;
+				$object->$name = (is_int($value) || is_bool($value) || $value === null) ? (string) $value : $value;


Remove parentheses

+ a few more changes found last minute

…ld have been an integer...

frankmayer · 2017-06-13T21:12:50Z

Bump @andrepereiradasilva & @Quy Problem solved. Pls test (review changes) to go for RTC

andrepereiradasilva · 2017-06-13T21:33:25Z

I have tested this item ✅ successfully on 171f66b

Code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12171.}

andrepereiradasilva · 2017-06-13T21:44:53Z

@peteruoi when we have levels like Joomla server side performance things are not like 30% Faster in all Pages. Is really relative to each change and to the amount of scenarios that change apllies to
For instance in the past made several changes that boosted performance only in multilingual sites.

Imo the most important performance improvements are the ones done to the critical page that apllies to all pages and also most used parts (com_content, jroute, etc).
That said general improvements like this set of prs also contribute as a whole.

Quy · 2017-06-13T21:58:45Z

I have tested this item ✅ successfully on 171f66b

Code review.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12171.}

ghost · 2017-06-14T04:05:43Z

RTC after two successful tests.

peteruoi · 2017-06-14T12:45:14Z

@andrepereiradasilva Even with a 5% percent general it will be awesome news for advertisment for 3.8 campaign. And no one forbids us to say 5% to 20% (at miltilingual for example).
I think it's a pity that the optimization you awesome fellows do and make joomla and the internet faster is not recognized enough :).
This metric of course it cannot be pr dependent but before release 3.7 vs 3.8 tests and if they are favourite for us we advertise ;)

frankmayer · 2017-06-15T08:54:57Z

Hooray, this really big chunk was finally merged! Thank you all for your comments and efforts in testing

frankmayer added 4 commits September 25, 2016 18:53

Fixed callable call in loop termination condition

033aa88

Fixed non-optimal regular expression

a650515

Shortened syntax for applied operations

a33ad89

Don't use strlen() to check if string is empty.

4173a7f

joomla-cms-bot added the PR-staging label Sep 25, 2016

zero-24 approved these changes Sep 25, 2016

View reviewed changes

zero-24 suggested changes Sep 25, 2016

View reviewed changes

Reverted preg changes. Look into those later again.

186f975

zero-24 approved these changes Sep 25, 2016

View reviewed changes

frankmayer added 9 commits September 25, 2016 20:16

Replaced call_user_func_array() with call_user_func()

19eb362

Merge unset() calls

f9cb3fe

Replaced old intval() call with modern typecasting

d30d1d7

Optimize away substr() as index-based access

0d1f128

Merge branch 'Performance_2' into Performance_1

321c813

Replace some cases of substr() with strpos()

d30fe28

Replace some stristr() with stripos()

5dfd4e9

Replace is_null() with null === ...

4c1cfdd

Removed unnecessary ternary operators

271d02a

frankmayer added 9 commits September 25, 2016 21:13

Fix for Juri...

c500e83

Revert some changes for ... PHP5.3 compatibility

44d234d

Make travis happy ....

6fb499b

Make codesniffer happy...

5ada9b9

DYC!

c2fd895

Optimize non-optimal if conditions.

de1a93e

codsniffer...

2aa353b

Inline one-time use variables

307cd94

Codesniffer...

062ef22

frankmayer added 3 commits June 13, 2017 18:12

Corrections on errors during merge (Conflict resolving)

4b30c17

Missed one.

e3f8617

Remove a parenthesis

7d7b3cd

andrepereiradasilva reviewed Jun 13, 2017

View reviewed changes

Changes according to reviewer's comment

42a6146

Revert change because of failure

cb5ee93

Quy reviewed Jun 13, 2017

View reviewed changes

frankmayer added 5 commits June 13, 2017 23:17

Changes according to reviewer's comment

bb10022

+ a few more changes found last minute

reversed change, though according to the method's doc that value shou…

c4157d9

…ld have been an integer...

Hopefully fixed...

ec2c21a

Hopefully fixed...

c0d3b44

Re-apply some changes

3619945

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 14, 2017

rdeutz merged commit bcfbfa0 into joomla:staging Jun 15, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 15, 2017

rdeutz added this to the Joomla 3.7.3 milestone Jun 15, 2017

frankmayer deleted the Performance_1 branch August 23, 2017 18:58

Uh oh!

Conversation

frankmayer commented Sep 25, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero-24 left a comment

Choose a reason for hiding this comment

Uh oh!

zero-24 left a comment

Choose a reason for hiding this comment

Uh oh!

zero-24 commented Sep 25, 2016

Uh oh!

frankmayer commented Sep 25, 2016

Uh oh!

frankmayer commented Sep 25, 2016

Uh oh!

andrepereiradasilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

peteruoi commented Jun 13, 2017

Uh oh!

frankmayer commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankmayer commented Jun 13, 2017

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

andrepereiradasilva commented Jun 13, 2017

Uh oh!

Quy commented Jun 13, 2017

Uh oh!

ghost commented Jun 14, 2017

Uh oh!

peteruoi commented Jun 14, 2017

Uh oh!

frankmayer commented Jun 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

frankmayer commented Sep 25, 2016 •

edited by joomla-cms-bot

Loading

andrepereiradasilva Jun 13, 2017 •

edited

Loading

andrepereiradasilva Jun 13, 2017 •

edited

Loading