? Success

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
25 Sep 2016

[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 #13234
  • 2) Replace unnecessary double quotes in libraries/cms #13235
  • 3) Remove one-time-use variables in libraries/cms #13236
  • 4) Various changes in libraries/cms #13238
  • 5) Type safe comparison of strings only, in libraries/cms #13275
  • 6) Type safe comparison in libraries/cms - second iteration #13276
avatar frankmayer frankmayer - open - 25 Sep 2016
avatar frankmayer frankmayer - change - 25 Sep 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Category Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 25 Sep 2016
Labels Added: ?
avatar zero-24
zero-24 - comment - 25 Sep 2016

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

avatar frankmayer
frankmayer - comment - 25 Sep 2016

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

avatar frankmayer
frankmayer - comment - 25 Sep 2016

on it...

c2fd895 25 Sep 2016 avatar frankmayer DYC!
avatar frankmayer
frankmayer - comment - 25 Sep 2016

That about sums this PR up. No new commits, as far as optimizations are concerned.
Let me know if you need me to change anything.

avatar sovainfo
sovainfo - comment - 26 Sep 2016

Would you mind motivating the other types of performance improvements as well?

avatar frankmayer
frankmayer - comment - 26 Sep 2016

@sovainfo I'll gladly point out my motivation for this.

Generally, all of the improvements are based on modern PHP best practices.

Most of them are dropping function calls in favor of using cheaper (in terms of cpu usage and memory) implementations.

Often, readability is also improved as a welcome side-effect of this.

Other things like the optimized if-conditions, are moving the cheaper conditions to the beginning of the conditions, in order to not invoke more expensive operations, if not necessary.

Of course there are some situations, where optimization is not possible, because of the necessity of maintaining PHP 5.3 compatibility. (Are there really people still running 5.3???)
IMHO, Joomla 4, whenever it comes out, should radically drop the complete 5.* line of PHP and have a minimum of PHP 7.0. Because 5.6 is going into 'security fixes only' mode on 1.1.2017 - see here

That should about sum it up.

As you have probably noticed, this PR is only focusing on the files in libraries/cms + it's subfolders, as I though it was a good starting point. If you guys are OK with those changes, I'll gladly expand the implementation of that kind of optimizations and cleanup to the whole project.

avatar sovainfo
sovainfo - comment - 26 Sep 2016

Thanks, was wondering about the optimization of if-conditions.

The costs of the condition shouldn't be the only factor. Sounds like you want to ignore the likely hood of the conditions, together they should determine the order.
This assumes it was given thought in the first place.

avatar frankmayer
frankmayer - comment - 26 Sep 2016

@sovainfo the cost actually isn't the only factor. For that reason, not all if-conditions were touched. I hand-picked only those, were it would not change their semantics. Did you notice some inconsistency after those changes?

avatar photodude
photodude - comment - 26 Sep 2016

Surprisingly almost 14% of submitted user statistics were for PHP 5.3
I personally question how many of those were submitted by developers doing testing
https://developer.joomla.org/about/stats.html

avatar frankmayer
frankmayer - comment - 26 Sep 2016

@photodude yes, that's a valid question. I too think the developer tests are tinkering with real statistics. I wonder if there should be some kind of 'isDeveloper' switch, which developers could set, in order to not have unwanted side effects like this.

avatar Bakual
Bakual - comment - 26 Sep 2016

Personally I think almost no dev tests with 5.3. Devs are the ones with the PHP7 servers ?
But users really are still using PHP 5.3. If you ask why, it's because they don't care about underlying systems and just trust their hosters. And those hosters don't want to risk breaking sites by upgrading the PHP version.

avatar mbabker
mbabker - comment - 26 Sep 2016

Part of it is we didn't want to make a distinction between dev and production environments in the statistics. It honestly doesn't matter to us either (well, that's my opinion anyway, YMMV). That's in part why you get the nag message versus default enabled (every fresh install I do I immediately hit never on it). We have provisions in the stats collection making it possible to report things that nobody will be using in production (i.e. our 4.0 branch doesn't get rejected, if you're compiling against PHP master which has a 7.2 version number right now it's accepted, etc.) so that too can skew things because it favors either ambitious or curious folks who want to try things out on bleeding edge technology and that can also be used as an indicator to see what folks are doing.

avatar frankmayer
frankmayer - comment - 27 Sep 2016

@Bakual I don't know about the devs testing with 5.3, but if the number really depicts users still using 5.3, then that's one more reason to require PHP 7.0 for Joomla 4, because if Joomla 4 would be released in 2017, then PHP 5.6 would already be in security only-fixes mode for about a year until its final EOL end of 2018.

This is Joomla's chance to help hosters get rid of old versions and also be able to use the newer technologies and speed provided by the PHP 7 series.

avatar Bakual
Bakual - comment - 27 Sep 2016

I don't think it's reasonable to raise the minimum PHP requirement for Joomla 4 to PHP 7, but PHP 5,.6 for sure. If it is released next year that is.
By the same logic you should raise the minimum requirement to PHP 7.1, because security support for PHP 7.0 actually runs out before PHP 5.6 does ?

But it is not up to me to make that decision.

avatar photodude
photodude - comment - 27 Sep 2016

@frankmayer In the past (and currently) there is a dynamic between what hosters are supporting (influenced by what's available and supported in standard distros) vs what developers want. Personally, I would love to see J3.7 drop php5.3 but that likely won't happen. On the other hand J4 is being developed with a php5.6+ design. Sadly most hosters are still hesitant to fully support php 7 even when they offer the option to use php7 (when problems arise they typically tell people to go back to using php5.6, even some developers of extensions tell customers that)

Balancing the hoster support, 3rd party extensions, and platform needs is an unpleasant challenge. IMO php5.6+ is a good compromise for J4.

avatar matrikular
matrikular - comment - 28 Sep 2016

From what I could grasp by reviewing the code alone, all looks good.

There are some areas, like database queries without a try / catch block neither before or after the optimisation, but I guess that alone might be a thing for different PR. Anonther thing that caught my attention is this passage from JApplicationSite::initialiseApp().

Good work, thanks!

avatar frankmayer
frankmayer - comment - 28 Sep 2016

@matrikular You're very welcome! I always wanted to give back to the Joomla project, and decided to give it a try by doing this. And as I mentioned earlier, if you (as the steering users) are OK with those kinds of changes I am open to expand the scope of optimizations to the whole project (minus the vendor imports... as I learned ? )

avatar andrepereiradasilva andrepereiradasilva - test_item - 4 Oct 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 4 Oct 2016

I have tested this item successfully on 171f66b

on code review


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

avatar frankmayer frankmayer - change - 12 Dec 2016
The description was changed
avatar frankmayer
frankmayer - comment - 12 Dec 2016

Fixed some conflicts that had occurred in the meantime. It's essentially ready to be merged, when the team finds the time and PR is OK.

avatar andrepereiradasilva
andrepereiradasilva - comment - 12 Dec 2016

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.

avatar andrepereiradasilva andrepereiradasilva - test_item - 12 Dec 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Dec 2016

conflicts

avatar frankmayer
frankmayer - comment - 18 Dec 2016

@andrepereiradasilva Yes, but after the referenced open PR's are merged. Don't want to do this another three times ;)

avatar frankmayer frankmayer - edited - 18 Dec 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Dec 2016

right i see

avatar frankmayer frankmayer - edited - 6 Jan 2017
avatar frankmayer
frankmayer - comment - 6 Jan 2017

Edited initial post to show which sub-PRs still have to go through, in order to make this one easier to digest.
It would be great if the initial big batch PRs of mine could make it into alpha2. Thanks!

avatar frankmayer frankmayer - change - 25 Apr 2017
The description was changed
avatar frankmayer frankmayer - edited - 25 Apr 2017
avatar frankmayer frankmayer - change - 29 May 2017
The description was changed
avatar frankmayer frankmayer - change - 9 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 9 Jun 2017
avatar frankmayer frankmayer - change - 13 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 13 Jun 2017
avatar frankmayer frankmayer - change - 13 Jun 2017
The description was changed
avatar frankmayer frankmayer - edited - 13 Jun 2017
avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jun 2017

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.

avatar andrepereiradasilva andrepereiradasilva - test_item - 13 Jun 2017 - Tested successfully
avatar peteruoi
peteruoi - comment - 13 Jun 2017

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!

avatar frankmayer
frankmayer - comment - 13 Jun 2017

@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.

avatar frankmayer
frankmayer - comment - 13 Jun 2017

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

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jun 2017

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.

avatar andrepereiradasilva andrepereiradasilva - test_item - 13 Jun 2017 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jun 2017

@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.

avatar Quy
Quy - comment - 13 Jun 2017

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.

avatar Quy Quy - test_item - 13 Jun 2017 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 14 Jun 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 14 Jun 2017

RTC after two successful tests.

avatar peteruoi
peteruoi - comment - 14 Jun 2017

@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 ;)

avatar rdeutz rdeutz - change - 15 Jun 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-06-15 06:48:00
Closed_By rdeutz
Labels Added: ?
avatar rdeutz rdeutz - close - 15 Jun 2017
avatar rdeutz rdeutz - merge - 15 Jun 2017
avatar frankmayer
frankmayer - comment - 15 Jun 2017

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

Add a Comment

Login with GitHub to post a comment