WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 8 months ago

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

48082-1.patch (83.8 KB) - added by ayeshrajans 12 months ago.
48082.diff (80.2 KB) - added by desrosj 12 months ago.
valentinbora-48082-parentheses.diff (226.5 KB) - added by valentinbora 8 months ago.
This patch simply removes extraneous parentheses upon @desrosj's patch 48082.diff per @netweb's suggestion
valentinbora-48082.diff (254.8 KB) - added by valentinbora 8 months ago.
A diff combining @desrosj's patch 48082.diff together with valentinbora-48082-parentheses.diff against trunk
valentinbora-48082-phpcs.diff (288.9 KB) - added by valentinbora 8 months ago.
A more complete diff onto [47105] having applied @jrf's, @joostdevalk's, @netweb's suggestions, based upon @desrosj's patch

Download all attachments as: .zip

Change History (23)

#1 follow-up: @jrf
12 months ago

  • Keywords needs-refresh added; has-patch removed
  • Version set to trunk

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

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

Last edited 12 months ago by jrf (previous) (diff)

#2 @desrosj
12 months 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.

@desrosj
12 months ago

#3 @desrosj
12 months ago

  • Milestone changed from Awaiting Review to Future Release

#4 in reply to: ↑ 1 @netweb
12 months ago

Replying to jrf:

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

#5 @ayeshrajans
12 months 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 @joostdevalk
12 months 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 @jrf
12 months 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:

  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.

  1. 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 @desrosj
12 months 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 @valentinbora
8 months ago

@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
8 months ago

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

@valentinbora
8 months ago

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

@valentinbora
8 months ago

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

#10 follow-up: @desrosj
8 months 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 @valentinbora
8 months 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 @SergeyBiryukov
8 months 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.

#13 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#14 @ayeshrajans
8 months ago

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

Thank you.

#15 follow-up: @danielbachhuber
8 months 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 @SergeyBiryukov
8 months ago

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.

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#17 @SergeyBiryukov
8 months ago

In 47200:

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

See #48082.

#18 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.