WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#53670 closed defect (bug) (wontfix)

Lazy-load admin functions in the new widgets editor REST API instead of bootstrapping the entire admin on every request

Reported by: zieladam Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: dev-feedback
Focuses: Cc:

Description

This is a follow up on https://github.com/WordPress/gutenberg/issues/33443

As a summary, we added require_once ABSPATH . 'wp-admin/includes/admin.php'; to a few REST API endpoints reponsible for rendering widgets. This solved the problem of fatal errors, but bootstraping wp-admin is slow and consumes resources. It wasn't a big problem in the classic widgets editor where it was done just once, but in the new one it would be done once per widget.

There isn't any widget in core WordPress that uses admin functions so we only need to do it if there's a plugin or a theme which assumes the availability of these functions. An example of such plugin is the [Compact Archives plugin](https://wordpress.org/plugins/compact-archives/) which works just fine in the classic widgets editor.

To use admin functions we need to load them first, and since https://github.com/WordPress/gutenberg/pull/33454 they are loaded eagerly. That's okay but not ideal Can we load them lazily instead?

If we were talking about classes, we could just add an autoloader via spl_autoload_register and call it a day. Unfortunately, PHP does not support autoloading functions so no dice here.

The only other way I can think of is having an auto-generated file with "proxy functions" that would only load the admin when that's necessary. Much like the patch below:

<?php
diff --git a/src/wp-admin/includes/lazy-admin.php b/src/wp-admin/includes/lazy-admin.php
index e69de29bb2..b85b9ca001 100644
--- a/src/wp-admin/includes/lazy-admin.php
+++ b/src/wp-admin/includes/lazy-admin.php
+<?php
+
+// Ideally this file would be auto-generated
+
+function lazy_bootstrap_admin() {
+       require_once __DIR__ . '/admin.php';
+}
+
+// Bring the documentation over from the actual function
+function is_plugin_active($name) {
+       lazy_bootstrap_admin();
+       return \Admin\is_plugin_active($name);
+}


diff --git a/src/wp-admin/includes/plugin.php b/src/wp-admin/includes/plugin.php
index 6e332c247e..18aa080ef3 100644
--- a/src/wp-admin/includes/plugin.php
+++ b/src/wp-admin/includes/plugin.php
@@ -5,7 +5,7 @@
  * @package WordPress
  * @subpackage Administration
  */
-
+namespace Admin;
 /**
  * Parses the plugin contents to retrieve plugin's metadata.
  *

This would enable auto-loading of all the things, whether we're in the widgets editor world or any other area of WordPress. I am curious if that could help speed up the entire WP by avoiding eager loading in principle. The price to pay would be an additional layer of complexity, but this isn't far off from what we already do with JS and CSS build pipelines – only here we're in the PHP world.

Alternatively, we could deprecate the assumption that wp-admin has already been bootstrapped by the time widgets (or their forms) are being rendered. This doesn't solve anything for now, but it's a start.

cc @peterwilsoncc @hellofromtonya @noisysocks

Change History (5)

#1 @noisysocks
3 months ago

Do we still want to do this now that GB33454 is reverted?

#2 @azaozz
3 months ago

I am curious if that could help speed up the entire WP

Yeah, that's the "million dollars question" :)

If we are only talking about wp-admin/includes/admin.php and not "bootstrapping" all of wp-admin, it would be really good to know how much resources are needed to load that code.

There definitely will be some milliseconds of disk i/o to get the files, and some MB of memory to load them, but not sure if that would be a "deal breaker" amount of resources.

On the other hand having a big amount of "fake" functions defined when not in wp-admin has its costs and potential problems. For example it will increase the total number of functions and classes quite a bit which (as far as I remember) slows down PHP by a tiny bit for each one each time it makes a function call. That slow-down, even if it is very minimal, will be happening every time WP runs on the front-end.

So we're probably looking at slowing all WP sites by a little bit while speeding up some/few sites in some cases.

Last edited 3 months ago by azaozz (previous) (diff)

#3 @zieladam
3 months ago

@noisysocks good question! The answer hinges on what @azaozz mentioned: will the benefits surpass the costs. I see the proxy functions as something we could use across the entire WordPress core, not something that's specific to the new widgets eeditor.

The next step here would be to analyze what can we gain and what can we lose. Maybe the gains would be minuscule, a few microseconds here and there. Maybe they would be much larger than that in some contexts. It's hard to tell. I'll do some measurements and report back.

#4 @zieladam
3 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I measured 10 batches of 100 requests to /wp/v2/widget-types with and without https://github.com/WordPress/gutenberg/pull/33454 using apache benchmark with -n 100 (100 sequential requests).

With GB33454 I got results between 307ms and 317ms
Without GB33454 I got results between 307ms and 315ms

Some of the variance may be attributed to my local development setup, varying workload of the machine, things like that. Still, some of these will also occur on shared and even dedicated hosts.

The difference in speed is minimal if there even is one: Maybe this could be explained by sheer randomness. I am not going to calculate any confidence intervals though. Also, this is going matter even less in a production setting with something like APC enabled.

Memory-wise, I checked memory_get_usage() before and after the require_once ABSPATH . 'wp-admin/includes/admin.php'; statement and got the following reads:

Before: int(4282056)
After: int(4388768)

100KB of difference in ram usage per request on an admin-specific endpoint – I think that's negligible.

This tells me two things:

  1. Adding any additional complexity just to prevent including some functions isn't worth it, even it's 700 functions and a bunch of classes.
  2. https://github.com/WordPress/gutenberg/pull/33454 could likely be restored without any negative effects

Either way, we're good to close this ticket.

cc @hellofromtonya @adraganescu @desrosj

#5 @desrosj
3 months ago

Thanks for doing such detailed testing @zieladam!

That does put my mind at ease a bit about the performance implications. Though I do still expect it to be a problem for some sites and hosting environments. But those may be edge cases.

However, performance concern is only one of the concerns I had with including admin.php within the context of the REST API requests. Once we make a change that makes admin related functions available, it becomes very difficult to remove.

As far as I know, administration functions are not available for any other blocks used within the block editor. This would result in a mixed expectation. Which blocks have these functions available through REST requests and which do not?

The Widgets screen in the admin is also a transition to block-based themes fully supporting Full Site Editing. Not making admin functions available to widgets in REST requests now will potentially make future transitions much smoother.

I also think that making a change like this when the screen changes to being block-based is preferred and is inline with the types of changes made when the post editor was transitioned in 5.0.

Note: See TracTickets for help on using tickets.