Make WordPress Core

Opened 4 months ago

Last modified 5 weeks ago

#60414 new enhancement

Core PHP autoloader proposal

Reported by: aristath's profile aristath Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests early
Focuses: performance, sustainability Cc:

Description (last modified by aristath)

Using a PHP autoloader can improve performance, and can be seen as a first step towards modernizing the WP codebase in the future.

This ticket is a continuation of the discussion on #36335
The discussion in the original ticket was derailed many times, so this fresh ticket is an opportunity to discuss the Core proposal submitted on the Make blog

The concerns that were brought-up in the original ticket:

  • PHP 5.2 compatibility: No longer relevant.
  • Files containing classes also contain other functions: This has already been addressed since then. The few instances where this was still an issue were addressed in the patch/PR.
  • Composer: Outside the scope of this initial implementation.
  • Overriding Core classes from plugins: Added a site-health check with a critical warning in case a Core class is not loaded from the default WP path.
  • API for plugins/themes to register additional classes: Outside the scope of this PR.
  • WordPress is not OOP: True, it's not currently OOP, but as we integrate the new editor in WP-Core, more and more OOP concepts creep in Core. There is definitely a trend, and WP is slowly becoming more OOP than it used to be. An autoloader will unblock us and allow us to further optimize and improve Core as we go.
  • What does autoloading do for "loose" functions?: Nothing. But it does give us the flexibility to properly group them in classes in the future, and only load what's needed. But that is a future discussion that will probably take a few more years.
  • Backwards-compatibility issues: There are none.
  • Performance: This patch has a negligible positive effect on performance. That is not its point. It opens the door to future improvements that can't be done easily without an autoloader. This is not an improvement with an immediately tangible outcome, it's just paving the way for more meaningful future improvements.

Change History (24)

This ticket was mentioned in PR #3470 on WordPress/wordpress-develop by @aristath.


4 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 follow-up: @justlevine
4 months ago

Love this!

@aristath can you clarify how (if at all) this relates to the Autoload class used by Requests])? You mentioned in the Proposal that an API is outside of the scope, and I'm assuming that since Requests is also intended to be consumed outside of WP contexts that it'l continue to maintain it's own implementation, and I'm not seeing anything directly related in the diff,

(Don't want to derail this by another decade by bringing up Composer again - so for people coming here directly and not from the Make post, let's hold on to this:

This is the first step towards modernizing the WP codebase. Though in itself it may appear like a marginal (but still very worthy) performance improvement, it opens the door to more improvements in the future of WordPress without any backwards-compatibility concerns.

and this followup comment)

#3 in reply to: ↑ 2 @aristath
4 months ago

Replying to justlevine:

@aristath can you clarify how (if at all) this relates to the Autoload class used by Requests])? You mentioned in the Proposal that an API is outside of the scope, and I'm assuming that since Requests is also intended to be consumed outside of WP contexts that it'l continue to maintain it's own implementation, and I'm not seeing anything directly related in the diff,

Check out the WP_Autoload::register_external_bundled() method.
It registers the autoloaders from external libraries inside the Core autoloader. Hard includes/requires for these were then removed from the rest of the WP codebase, and the Core autoloader handles them 👍

@aristath commented on PR #3470:


4 months ago
#4

Adding some numbers from tests ran today.

## Included files

Adding this snippet in mu-plugins/snippets.php:

<?php
add_action( 'shutdown', function() {
        var_dump( count( get_included_files() ) );
} );

I got the following numbers:
| | Trunk | Autoload | Diff

Front 442 347 -95 files (-21.5%)
Dashboard 479 370 -109 files (-22.7%)

I only checked the frontpage and the main dashboard page, so these numbers will differ depending on the context. Some pages will have smaller differences, others will have larger. I expect that REST-API endpoints will have a significantly larger benefit, but I don't know exactly how to measure that.

## Memory usage.

Tested using the Debug Bar community plugin. Numbers in Kb
| | Trunk | Autoload | Diff

Front 4327 4329 0.04%
Dashboard 4074 4078 0.09%

This shows that the autoloader implementation currently uses 0.04-0.09% more memory. I don't know if that is because I have XDebug running (which admittedly is a performance hog), these numbers contradict previous tests both by @spacedmonkey in https://github.com/WordPress/wordpress-develop/pull/3470#issuecomment-1490083546 as well as previous tests of mine. Worth examining what changed these past few months.

## Time

Tested using XDebug profiling. Report built using webgrind.
These numbers are the average of 5 runs because XDebug-profiling produces rather inconsistent numbers.

Trunk Autoload Diff
Front 392ms 377ms -3.82%
Dashboard 317ms 295ms -6.94%

The above tests are somewhat basic.
If someone else has the ability and/or ability to run some more detailed tests to get more consistent numbers, it would be greatly appreciated 👍

@bartj commented on PR #3470:


4 months ago
#5

I have played a little with benchmarking, doing 100 sequential requests to each endpoint. The results should be more consistent, but I'm not sure what is the reason for quite large variance (look at the standard derivation value for timing).

Benchmarking was running against PHP 8.1 with nginx server. Profiling data comes from xhprof (slightly better than xDebug and provides actual memory usage info).

### Average Run Times (mean) / (median) / (stddev)

trunk autoload Percentage Diff
admin 125.79ms / 121.37ms / ±22.38 125.02ms / 121.66ms / ±17.82 -0.62%
front 228.13ms / 223.95ms / ±13.45 230.23ms / 224.68ms / ±16.14 0.92%
rest 91.55ms / 89.32ms / ±13.23 87.20ms / 86.25ms / ±5.17 -4.75%

### Average Memory Usage (mean) / (median) / (stddev)

trunk autoload Percentage Diff
admin 6.04M / 6.04M / ±0.04 6.23M / 6.23M / ±0.00 3.08%
front 6.66M / 6.66M / ±0.00 6.69M / 6.69M / ±0.00 0.49%
rest 5.68M / 5.68M / ±0.00 5.71M / 5.71M / ±0.00 0.5%

I've left notes on how my benchmark was conducted, along with collected results in repository: https://github.com/bart-jaskulski/wordpress-benchmark

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


4 months ago

#8 @johnbillion
4 months ago

  • Keywords early added
  • Version trunk deleted

@aristath commented on PR #3470:


3 months ago
#9

Since I lack the ability to test memory consumption reliably, I think a good measure would be the test-suite results posted in this PR.

https://github.com/WordPress/wordpress-develop/actions/runs/8264293168

  • There seems to be virtually no difference in the memory before/after this PR.
  • The differences in load-times is ±1ms in almost all areas when ran in the test-suite.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


3 months ago

kktsvetkov commented on PR #3470:


2 months ago
#11

Have you considered having multiple smaller autoload.php files instead of just one big src/wp-includes/class-wp-autoload.php ? This way each module will have its own autoload map. Those issues with simple-pie and parser.php having multiple classes in them will be addressed this way, with a very local approach. In theory, having smaller autoloads will also help to test each module in isolation without relying on the global core autoload.

kktsvetkov commented on PR #3470:


2 months ago
#12

I'm curious if you have explored bundling Composer\Autoload\ClassLoader instead of building a new autoloader? It's more or less the standard nowadays, it's battle-tested, and it already supports working with classmaps

$composer = new \Composer\Autoload\ClassLoader();

 $composer->addClassMap([
        'wp_hook'    => 'wp-includes/class-wp-hook.php',
        'wp_locale'  => 'wp-includes/class-wp-locale.php',
]);

$composer->register();

It seems to me there will be an additional benefit as there are a lot of plugins and themes that are already using composer, although in a local capacity.

@TJNowell commented on PR #3470:


2 months ago
#13

I'm curious if you have explored bundling Composer\Autoload\ClassLoader instead of building a new autoloader? It's more or less the standard nowadays, it's battle-tested, and it already supports working with classmaps

Acutely aware, discussion of it completely derailed the last attempt to include composer setting things back years. Having said that there was discussion of using composer to generate the file mappings, but you're best referring to the Make WP post for specifics.

@Kaloyan commented on PR #3470:


2 months ago
#14

I'm not suggesting using composer as it is used in general, instead only utilizing the ClassLoader as available solution. I did manage to read the previous thread, and you are right - the composer discussion did seem to get out of hand.

#15 @Kaloyan
2 months ago

There are places in the code with defensive class_exists() checks before class declarations, like this:

if ( ! class_exists( 'Some_Class', false ) ) class Some_Class {}

Considering the autoloading changes, do you think those defensive checks are still going to be necessary?

#16 @aristath
2 months ago

@Kaloyan I just did a quick search in WP's code for class_exists checks.
It looks like the only remaining instances are in 3rd-party scripts, and one more instance in deprecated.php (which is safe to ignore) 👍

This ticket was mentioned in Slack in #core-performance by aristath. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


2 months ago

@aristath commented on PR #3470:


7 weeks ago
#19

Added an additional check in site-health.

If there are any overriden classes, it will show a critical security warning.

To test this, I added a file in mu-plugins/w.php with the following contents:

<?php
class WP_Community_Events {}
class WP_Users_List_Table extends WP_List_Table {}

https://github.com/WordPress/wordpress-develop/assets/588688/e24f5ad1-e108-42e9-8119-7e2b09fbcf88

We can fine-tune the text, presentation etc, but I believe this can serve us well and inform users if any Core classes are being overriden by a plugin/theme/host.

#20 @aristath
7 weeks ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


5 weeks ago

#22 @johnbillion
5 weeks ago

  • Milestone changed from Awaiting Review to Future Release

This was briefly discussed during today's performance team ticket scrub.

  • What is needed to get this approved and ready for a merge into core?
  • Any outstanding concerns from the comments on the make/core post or from the PR?
  • This is likely now too late for 6.6, so let's target 6.7.
  • This needs a core committer to own it at the beginning of the 6.7 cycle.

This ticket was mentioned in Slack in #core-upgrade-install by johnbillion. View the logs.


5 weeks ago

#24 @aristath
5 weeks ago

A quick update of what happens in this ticket and a recap of the concerns that were brought up in the proposal - along with what was done to address them:

  • Composer vs non-composer: It's easier for us to start with the basic classmap implementation that exists in the PR, and then discuss separately if we want to automate the classmap generation using Composer.
  • Added an extra check in the health-check screen, to check if any Core classes are being overridden, and show a critical warning if something like that occurs
  • PHPUnit tests were added and all existing automated tests pass without issues.
  • Performance impact: Adding an autoloader seems to reduce the memory use, but - at this stage - has no other significantly measurable impact.

Any original concerns that were mentioned in the original ticket (#36335) have already been added to this ticket's description, along with what was done to address them

The PR is complete and it works. There are no backward-compatibility issues, and I constantly update it so it's up to date with trunk.

Note: See TracTickets for help on using tickets.