#48082 closed enhancement (fixed)
Replace `dirname(__FILE__)` with `__DIR__` (Performance improvement, massive patch)
Reported by: | ayeshrajans | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | minor | Version: | |
Component: | General | Keywords: | has-patch needs-testing |
Focuses: | performance, coding-standards | Cc: |
Description
PHP 5.3 added the __DIR__
magic constant, that is same as dirname(__FILE__)
. Because dirname()
is a function call, this can have a performance overhead of the function call every time we use dirname(__FILE__)
instead of __DIR__
.
Now that we require PHP 5.6 as the minimum version, we can drop the dirname()
calls in favor of __DIR__
.
Attached is a massive patch that replaces all such calls with the magic constant. I have grepped the source for any further occurrences and we are clean now.
I understand reviewing such a big patch is difficult, but hopefully we can contain this set of changes within one ticket, so they are easy to merge/deal with.
Thank you.
Attachments (5)
Change History (26)
#1
follow-up:
↓ 4
@
5 years ago
- Keywords needs-refresh added; has-patch removed
- Version set to trunk
#2
@
5 years ago
- Keywords has-patch added; needs-refresh removed
- Version trunk deleted
Thanks for this one, @ayeshrajans!
At first glance, your patch also updates external libraries included in WordPress (SimplePie, Requests, etc.). Because these libraries are maintained elsewhere and copied verbatim into the code base as much as possible, these would need to be changed upstream before including in Core.
I have an incoming patch with those libraries skipped, and with some PHPCS fixes.
I have set up a branch on my fork and it shows the tests passing: https://travis-ci.org/desrosj/wordpress-develop/builds/587250892.
I'm tempted to tag this as 5.4 early
, just in case there are any issues that may take a while to detect.
#4
in reply to:
↑ 1
@
5 years ago
Replying to jrf:
- As nearly all of these touch
include/require
statements, may this be a good time to remove the unnecessary parentheses which are often used with them ?
include
/require
are language constructs, not function calls and not using the parentheses has an - albeit marginal - performance benefit.
<?php // Bad: require_once( dirname( __FILE__ ) . '/admin.php' ); // Good: require_once dirname( __FILE__ ) . '/admin.php';The
WordPress-Extra
PHPCS ruleset actually contains a sniff which already checks for this. If the unneeded parentheses would be removed, the sniff could move from theExtra
ruleset to theCore
ruleset. /cc @pento
I think this is a great idea, two birds with one stone and all that, or in other words, it would make SVN/Git blame a little nicer doing this at the same time.
#5
@
5 years ago
Thanks for the responses. I realized the PHPCS failures in CI, and thanks a lot for the fixes and improvements with require_once __DIR__
pattern too! +1 from me too :)
#6
@
5 years ago
Looking through this patch I'm really liking where you're going with this. Some minor feedback: I see a lot of include
statements where I think we should be using require_once
. For instance includes on admin-header.php
don't make sense: without that the admin doesn't make any sense. Or an include
on wp-load.php
in xmlrpc.php
, if that fails, there's no WP, a require would be much better...
#7
@
5 years ago
I very much support @joostdevalk's suggestion.
While it may be a lot in one patch, it does make sense to me to make these changes all in one go as they all touch the same lines of code.
The sniff I previously mentioned which is in the WordPress-Extra
ruleset also checks for use of unconditional include[_once]
use.
To run it over WP Core while respecting the existing exclusions for the files coming from dependencies, follow the below steps:
- Add a new file to the root of your clone of WP Core (directory above
src
) calledphpcs.xml
. - Add the following contents to that file:
<?xml version="1.0"?> <ruleset name="WP Core local"> <rule ref="./phpcs.xml.dist"/> <rule ref="PEAR.Files.IncludingFile.BracketsNotRequired"> <type>warning</type> </rule> <rule ref="PEAR.Files.IncludingFile.UseRequire"> <type>warning</type> </rule> <rule ref="PEAR.Files.IncludingFile.UseRequireOnce"> <type>warning</type> </rule> </ruleset>
Note: the file should automatically be git-ignored/svn-ignored.
- Run PHPCS with the below command to just and only see the errors coming from this sniff:
vendor/bin/phpcs --sniffs=PEAR.Files.IncludingFile
#8
@
5 years ago
- Milestone changed from Future Release to 5.4
Let's prioritize this in 5.4 as part of the continued modernization/performance efforts.
#9
@
5 years ago
@desrosj's patch 48082.diff still applies cleanly onto the latest changeset at the time of writing [47105]
- Build succeeded
grunt test
did not reveal any failures pertaining to this patch
@
5 years ago
This patch simply removes extraneous parentheses upon @desrosj's patch 48082.diff per @netweb's suggestion
@
5 years ago
A diff combining @desrosj's patch 48082.diff together with valentinbora-48082-parentheses.diff against trunk
@
5 years ago
A more complete diff onto [47105] having applied @jrf's, @joostdevalk's, @netweb's suggestions, based upon @desrosj's patch
#10
follow-up:
↓ 11
@
5 years ago
- Keywords needs-testing added
It would still be great to run some performance testing here to quantify the improvements here.
#11
in reply to:
↑ 10
@
5 years ago
Replying to desrosj:
It would still be great to run some performance testing here to quantify the improvements here.
Any pointers towards running such a benchmark?
#12
@
5 years ago
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
It's worth mentioning that `require_once` is slower than `require` due to an additional check whether the file has already been included, see the previous discussion on #20298 for more details.
It probably won't make a big difference here, since most of the existing admin-header.php
and admin-footer.php
inclusions already use require_once
, so I guess switching the remaining few to require_once
makes sense from a consistency point of view, as long as the same file is not included a significant amount of times in a loop, or something like that. Still, since performance is one of the considerations with changes like this, I thought I'd mention this.
#14
follow-up:
↓ 19
@
5 years ago
That's one small step for WordPress, one giant patch for SVN.
Thank you.
#15
follow-up:
↓ 16
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Neat improvement :)
My plugin tests have started failing on trunk after this changeset with:
Warning: require_once(/tmp/wordpress-tests-lib/src//wp-includes/class-phpmailer.php): failed to open stream: No such file or directory in /tmp/wordpress-tests-lib/includes/mock-mailer.php on line 2
This is because install-wp-tests.sh
(a very common bash script) uses sed
to modify wp-tests-config-sample.php
: https://github.com/wp-cli/scaffold-command/blob/838b61095fe87105456461b5c065dcf058902432/templates/install-wp-tests.sh#L115-L118
While using sed
to modify wp-tests-config-sample.php
certainly isn't the best idea in the world, it's going to be pretty painful for many (thousands of?) plugins to update install-wp-tests.sh
so the script handles both WP 5.3 and WP 5.4 scenarios.
For the purposes of not causing these explosions, could we revert back to dirname( __FILE__ )
in wp-tests-config-sample.php
?
#16
in reply to:
↑ 15
@
5 years ago
Replying to danielbachhuber:
For the purposes of not causing these explosions, could we revert back to
dirname( __FILE__ )
inwp-tests-config-sample.php
?
Thanks for catching that! The same issue was reported by @kaggdesign a couple hours ago.
He also submitted a PR for the scaffold
command: https://github.com/wp-cli/scaffold-command/pull/250
Yes, reverting the change in wp-tests-config-sample.php
for now makes sense.
#19
in reply to:
↑ 14
;
follow-up:
↓ 20
@
3 years ago
Replying to In 47198:
Coming back to this after some time, mostly for posterity. Think the replacement of dirname( __FILE__ )
calls with __DIR__
(the original purpose of this ticket) was great.
However the removal of parentheses around concatenated strings (that was "attached" here later on) was perhaps a bad idea. It seems to make the code a little bit harder to read in some places, mostly when several strings are concatenated.
Did a quick test to see how much faster include 'file.php';
is compared to include( 'file.php' );
as that was the reason to reduce code readability. Running this 10k times in a loop did not produce any measurable difference, seems the speed up is mostly theoretical.
The other change that was "attached" later seems to have a bit of negative effect. Replacing include
with require once
seems to slow down PHP by about 0.5% on my test server. Frankly I'm not sure why this was done without any tests and why on this ticket.
#20
in reply to:
↑ 19
;
follow-up:
↓ 21
@
3 years ago
Replying to azaozz:
Did a quick test to see how much faster
include 'file.php';
is compared toinclude( 'file.php' );
as that was the reason to reduce code readability. Running this 10k times in a loop did not produce any measurable difference, seems the speed up is mostly theoretical.
Thanks for the feedback! Just noting that include( 'file.php' );
triggers two WPCS warnings:
| WARNING | [x] "include" is a statement not a function; no parentheses are required | | (PEAR.Files.IncludingFile.BracketsNotRequired) | WARNING | [x] File is being unconditionally included; use "require" instead | | (PEAR.Files.IncludingFile.UseRequire)
The other change that was "attached" later seems to have a bit of negative effect. Replacing
include
withrequire once
seems to slow down PHP by about 0.5% on my test server. Frankly I'm not sure why this was done without any tests and why on this ticket.
I think this part was suggested in comment:6 and supported in comment:7 not necessarily for performance, but rather for consistency: include
implies that the file is optional and execution can continue even if it's missing, while require
implies a hard requirement, which was the case in pretty much all of the instances here.
#21
in reply to:
↑ 20
@
3 years ago
Replying to SergeyBiryukov:
Thanks for the explanation! Was really helpful :)
Just noting that
include( 'file.php' );
triggers two WPCS warnings:
| WARNING | [x] "include" is a statement not a function; no parentheses are required | | (PEAR.Files.IncludingFile.BracketsNotRequired) | WARNING | [x] File is being unconditionally included; use "require" instead | | (PEAR.Files.IncludingFile.UseRequire)
Yes, seems that perhaps WPCS got it wrong here :)
The "brackets are not required" does not mean they are "forbidden", right? If not forbidden, does it make sense to use them in cases where that may improve readability a bit? IMHO it is a tiny bit more readable to group together a path that is being concatenated from several strings at the same time.
The include
to require
replacements seem fine. Just thinking that perhaps it would have been better to do them in another ticket and perhaps do some testing how much the slow-down would be if at all measurable.
I'm not thinking of opening new tickets for the same things, or reverting removal of brackets, or anything. Just trying to figure out where these changes come from and how they were decided. Seems that code readability keeps being reduced little by little, and most of the decisions concerning it are done without realizing they would affect readability.
Hi @ayeshrajans Thanks for taking the initiative for this!
I've reviewed the patch and have a couple of remarks:
composer format
over the patch to fix this.include/require
statements, may this be a good time to remove the unnecessary parentheses which are often used with them ?include
/require
are language constructs, not function calls and not using the parentheses has an - albeit marginal - performance benefit.The
WordPress-Extra
PHPCS ruleset actually contains a sniff which already checks for this. If the unneeded parentheses would be removed, the sniff could move from theExtra
ruleset to theCore
ruleset. /cc @pentoABSPATH . WPINC
instead of__DIR__
(where applicable). Along the same lines, the testbootstrap.php
file defines aDIR_TESTROOT
constant which can (and probably should) be used in quite a lot of places.Other than that, this looks good.