Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autoloading WordPress classes #3470

Open
wants to merge 174 commits into
base: trunk
Choose a base branch
from
Open

Conversation

aristath
Copy link
Member

@aristath aristath commented Oct 14, 2022

Trac ticket: https://core.trac.wordpress.org/ticket/60414


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@aristath aristath force-pushed the autoload branch 2 times, most recently from 6f5d207 to aa17c97 Compare November 14, 2022 12:05
@tomdevisser
Copy link
Member

Ohhh, love this PR!

src/index.php Outdated Show resolved Hide resolved
@spacedmonkey
Copy link
Member

Run some profiles, before and after, the results, are sadly not promising.

2023 theme

Before

Screenshot 2023-03-01 at 10 31 16

After

Screenshot 2023-03-01 at 10 31 20

2021 theme

Before

Screenshot 2023-03-01 at 10 31 11

After

Screenshot 2023-03-01 at 10 31 06

@aristath
Copy link
Member Author

aristath commented Mar 1, 2023

Run some profiles, before and after, the results, are sadly not promising.

Not quite... 😄
The numbers on the screenshots above point out something interesting:
PHP time is increased by ~1.5%, but memory consumption is decreased by ~4.5%.

Worth noting:
This is just the 1st step towards a much bigger project to modernize WP and make it more efficient, it's not the end of the road.
There are many things we can do to improve and modernize WP, but none of that can happen if WP keeps loading everything, always.
Implementing a simple autoloader doesn't have a big impact in itself (the benefits and costs are marginal, and one could argue that a 4.5% decrease in memory-cost is more important than a 1% increase in time-cost), but it does open the door to many other improvements 👍
The real performance improvements will come after the autoloader has landed, where we'll be able to take advantage of it. This PR just adds the necessary infrastructure for the future 😉

@lkraav
Copy link

lkraav commented Mar 2, 2023

Implementing a simple autoloader doesn't have a big impact in itself (the benefits and costs are marginal, and one could argue that a 4.5% decrease in memory-cost is more important than a 1% increase in time-cost), but it does open the door to many other improvements +1

I'm also thinking that at very least, it's a DX improvement and future unlocker. If it lands with no performance regressions, that's cherry on top.

@lkraav
Copy link

lkraav commented Mar 16, 2023

Hey @aristath are you daily driving this by any chance, or is anybody else?

Wondering if I could become real hands-on for testing this in my semi-production / staging env 🤔 or will everything blow up instantly.

@aristath
Copy link
Member Author

aristath commented Mar 16, 2023

Hey @lkraav!

The implementation here is complete, and the autoloader works as expected. I haven't seen anything "blow up"...
Once/week I rebase the PR and update it, pulling the latest changes from Core, updating anything that needs updating, resolving conflicts etc, keeping it up to date with WP trunk.
This definitely needs lots of testing, so if you use it on your semi-production/staging sites that would be great, and it would allow us to further polish the implementation! 🙇

I plan to create a new trac ticket when the time comes and publish a formal proposal, so any testing and additional data we have will be greatly appreciated! 👍

@lkraav
Copy link

lkraav commented Mar 16, 2023

Once/week I rebase the PR and update it, pulling the latest changes from Core, updating anything that needs updating, resolving conflicts etc, keeping it up to date with WP trunk.

Ah, this brings up a compatibility question - I'd like to stay in at least release candidate stability, but I guess this branch is constantly moving with trunk? Maybe you have tags available so I could merge only up to some "latest compatible with 6.1, or 6.2" state?

@aristath
Copy link
Member Author

I'd like to stay in at least release candidate stability, but I guess this branch is constantly moving with trunk?

Yeah, I'm keeping it up to date with trunk constantly...
However, the patch should apply cleanly to RC!

@lkraav
Copy link

lkraav commented Mar 16, 2023

I'd like to stay in at least release candidate stability, but I guess this branch is constantly moving with trunk?

Yeah, I'm keeping it up to date with trunk constantly... However, the patch should apply cleanly to RC!

I'm running into trouble trying to apply it to release WP at https://github.com/WordPress/WordPress.git didn't think of that before.

src/ would be easy to strip, but looks like I also need to skip tests 🤔

EDIT git apply 3470.patch --exclude=tests/* --exclude=phpunit/* -p2 --reject 💪

This hunk gets rejected on top of 6.2 RC2

$ [-] cat index.php.rej 
diff a/index.php b/index.php    (rejected hunks)
@@ -19,9 +19,8 @@
 
 /*
  * Load the actual index.php file if the assets were already built.
- * Note: WPINC is not defined yet, it is defined later in wp-settings.php.
  */
-if ( file_exists( ABSPATH . 'wp-includes/js/dist/edit-post.js' ) ) {
+if ( file_exists( ABSPATH . WPINC . '/js/dist/edit-post.js' ) ) {
        require_once ABSPATH . '_index.php';
        return;
 }

Aha but I see this index.php is build-only, so perhaps all good!

Skimmed through our sites, and everything seems to be working 💪 no crashes!

@aristath aristath force-pushed the autoload branch 3 times, most recently from 3a6050e to daac22d Compare March 24, 2023 09:11
@kasparsd
Copy link

@spacedmonkey The front-end performance test suite for this pull request appears to show a performance improvement for both classic and block themes:

performance-test-results

That's with 20 runs against the trunk baseline.

@aristath aristath force-pushed the autoload branch 2 times, most recently from 24d7ccd to 81aabba Compare March 29, 2023 11:14
@spacedmonkey
Copy link
Member

@aristath
Copy link
Member Author

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 {}
Screenshot 2024-04-22 at 1 04 54 PM

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet