Replace `dirname(__FILE__)` with `__DIR__` (Performance improvement, massive patch)

[ayeshrajans]

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.

[jrf]

Hi @ayeshrajans Thanks for taking the initiative for this!

I've reviewed the patch and have a couple of remarks:

1. In a lot of places the patch does not comply with the coding standards regarding whitespace on the inside of parentheses. Please run composer format over the patch to fix this.
2. 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 the Extra ruleset to the Core ruleset. /cc @pento

3. I wonder whether it may be more descriptive to use ABSPATH . WPINC instead of __DIR__ (where applicable). Along the same lines, the test bootstrap.php file defines a DIR_TESTROOT constant which can (and probably should) be used in quite a lot of places.
4. I don't think files from external dependencies - such as GetID3, PHPMailer, SimplePie or Text_Diff (etc) should be patched as it may make the upgrade path in the future more difficult (checking if there are any customizations should only yield real customizations.

Other than that, this looks good.

[desrosj]

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.

[netweb]

Replying to jrf:

2. 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 the Extra ruleset to the Core 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.

[ayeshrajans]

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

[joostdevalk]

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

[jrf]

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:

1. Add a new file to the root of your clone of WP Core (directory above src) called phpcs.xml.
2. 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.

3. Run PHPCS with the below command to just and only see the errors coming from this sniff:

vendor/bin/phpcs --sniffs=PEAR.Files.IncludingFile

[desrosj]

Let's prioritize this in 5.4 as part of the continued modernization/performance efforts.

[valentinbora]

@desrosj's patch 48082.diff​ still applies cleanly onto the latest changeset at the time of writing [47105]

1. Build succeeded
2. grunt test did not reveal any failures pertaining to this patch

[valentinbora]

This patch simply removes extraneous parentheses upon @desrosj's patch 48082.diff​ per @netweb's suggestion

[valentinbora]

A diff combining @desrosj's patch 48082.diff​ together with valentinbora-48082-parentheses.diff​ against trunk

[valentinbora]

A more complete diff onto [47105] having applied @jrf's, @joostdevalk's, @netweb's suggestions, based upon @desrosj's patch

[desrosj]

It would still be great to run some performance testing here to quantify the improvements here.

[valentinbora]

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?

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 47198:

Code Modernization: Replace dirname( __FILE__ ) calls with __DIR__ magic constant.

This avoids the performance overhead of the function call every time dirname( __FILE__ ) was used instead of __DIR__.

This commit also includes:

• Removing unnecessary parentheses from include/require statements. These are language constructs, not function calls.
• Replacing include statements for several files with require_once, for consistency:
  • wp-admin/admin-header.php
  • wp-admin/admin-footer.php
  • wp-includes/version.php

Props ayeshrajans, desrosj, valentinbora, jrf, joostdevalk, netweb.
Fixes #48082.

[ayeshrajans]

That's one small step for WordPress, one giant patch for SVN.

Thank you.

[danielbachhuber]

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?

[SergeyBiryukov]

Replying to danielbachhuber:

For the purposes of not causing these explosions, could we revert back to dirname( __FILE__ ) in wp-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.

[SergeyBiryukov]

In 47200:

Tests: Change a few remaining include_once statements to require_once, for consistency.

See #48082.

[SergeyBiryukov]

In 47201:

Tests: Revert the dirname( __FILE__ ) replacement in wp-tests-config-sample.php for now, to avoid breaking unit tests created with WP-CLI scaffold command.

Follow-up to [47198].

Props kaggdesign, danielbachhuber, bwmarkle.
Fixes #48082, #49377.

[azaozz]

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.

[SergeyBiryukov]

Replying to azaozz:

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.

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

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.

[azaozz]

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.