WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#7372 closed defect (bug) (fixed)

Plugin update check running on each pageload is slow

Reported by: DD32 Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.6
Component: Administration Keywords: needs-patch dev-feedback
Focuses: Cc:

Description

The plugin update checker was changed to run on every page load, Unfortunately this has had the effect of get_plugins() being called for *every* pageload.

get_plugins appears to be rather slow, Eating up a massive 420ms (of a 450ms page load).

due to the way which get_plugins() is implemented, get_plugin_data is called on every .php file, In some cases, plugin files can be rather large, and when using conditional loading for large plugins, The time to read & parse each file can be expensive, Taking on average 10~50ms on my computer. (ENV: Win32)

It appears that plugin_has_required_fields() which is called often takes on average of 2ms, being called anywhere between 5 to 20 times per file.

Increasing the file read block size to 4096 has sped it up enough for me (Plugin check is now only 190ms in total) for now ( plugin_get_contents() -> fread()

200ms still seems like a rather large ammount of processing time which is being used mearly for the update checker.

Another option would be to move the update checks to be run via CRON unless loading the plugins page directly. This would mean a delay is present when plugins are changed and the plugins page is not visited, but it results in a snappier page load for those with lots of plugins.

(Note: No '2.7' in trac version list)

Attachments (7)

plugin_data.7372.diff (3.8 KB) - added by santosj 7 years ago.
Performance improvements for #7372
plugin_data.7372.2.diff (5.9 KB) - added by santosj 7 years ago.
Further combination of functions for increased performance and decrease function stack overhead.
7372.diff (1.8 KB) - added by ryan 7 years ago.
Schedule plugin updates with cron
7372.2.diff (2.2 KB) - added by ryan 7 years ago.
Check for updates from dashboard every 12 hours
7372.3.diff (2.8 KB) - added by ryan 7 years ago.
Cron every 12
plugins-only.diff (1.9 KB) - added by Otto42 7 years ago.
Only check for plugin data changes when loading plugins.php anyway (corrected version)
7372.4.diff (2.8 KB) - added by ryan 7 years ago.
Check if update needed on every admin page load

Download all attachments as: .zip

Change History (43)

comment:1 @santosj7 years ago

The algorithm needs to be revisited. I had worried that performance might actually be affected. Also, it appears memory will also be an issue, if the required fields are not found then it will just continue until the file is completely loaded or the fields are found.

These are not what the solution called for. I'm unaware how to handle it so that if comments aren't found in the plugin to just back out and fail to continue reading the file.

It could be faster if the code was combined into a single function, there is some overhead in creating the stack, which is not required. That might shave off 1 or 2 ms.

Need some way to halt reading certain files that either don't have comments or don't have plugin data. It also needs to make sure that all of the plugin data is pulled in.

comment:2 @ryan7 years ago

Run via cron every x minutes or every time the plugins page is loaded seems fine. We definitely don't need to be hitting this every page load.

comment:3 @Otto427 years ago

Well, I like the idea of hitting the plugin update function on every admin page load, like it does now, but I do agree that it is ridiculous that it basically loads every single PHP file up to the very end if it can't find the plugin data. I think adding a limitation on this might be cleaner:

Examine this bit of code down in plugin_get_contents():

// Keep reading the contents of the file until End of File is
// reached, or we grabbed all of the required plugin data.
while( !feof($fp) && !plugin_has_required_fields($contents) )
	$contents .= fread( $fp, 1024 );

Really? Do we need to load the whole bloody file just to look for plugin data? How about we add that those 5 required fields must be within the first, say, 2k of the file. Limit this read to twice only. If you can't put the plugin comments at the top and within 2k, then fix your plugin.

I really can't see this sort of a limit affecting any existing plugins in a significant way. And it will severely reduce the load time for plugin files in all the cases where we load the list.

comment:4 @ryan7 years ago

A few plugins were identified last time we discussed this that put their headers way down. I don't think we should all suffer for them though. Let's set a limit and be done with it. A header should be at the top. :-)

comment:5 @santosj7 years ago

Agreed. Who cares about Google Sitemap and fringe cases when performance is at stake? Well, besides the one or two plugin authors who are going to be affected.

comment:6 @santosj7 years ago

{{{ Keep reading the contents of the file until End of File is
reached, or we grabbed all of the required plugin data.
while( !feof($fp) && !plugin_has_required_fields($contents) )

$contents .= fread( $fp, 1024 );

}}}

Can be

$contents .= fread( $fp, 4096 );

comment:7 @santosj7 years ago

Also you can remove the plugin_has_required_fields() function.

comment:8 @filosofo7 years ago

When we last talked about plugin data's location, I grepped through wp-plugins.org to see where plugin authors actually were putting their data, and as you can see from the results, there are few people outside of the norm.

@santosj7 years ago

Performance improvements for #7372

comment:9 @santosj7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

The patch pulls in 8kiB, which should be enough. If that isn't the case, then something is seriously up. Again, needs to be tested to ensure that plugins such as Google sitemap function properly.

Removes the redundant helper function, since it is no longer needed.

comment:10 @santosj7 years ago

  • Keywords needs-unit-tests added

comment:11 @santosj7 years ago

Test cases for several plugins wouldn't hurt either. Probably also need to note on the Codex the restriction.

comment:12 @ryan7 years ago

Testing now. Looking good.

@santosj7 years ago

Further combination of functions for increased performance and decrease function stack overhead.

comment:13 @santosj7 years ago

New patch improves documentation and tries to further decrease function stack overhead by combining the last function into the get_plugin_data() function.

comment:14 @ryan7 years ago

(In [8402]) Performance improvments to get_plugin_data() from santosj. see #7372

comment:15 @santosj7 years ago

Is this complete?

comment:16 @santosj7 years ago

  • Keywords needs-patch added; has-patch needs-testing needs-unit-tests removed

Need to define the CRON task somewhere probably before this is finished.

comment:17 @ryan7 years ago

Introduced with #7265

@ryan7 years ago

Schedule plugin updates with cron

comment:18 @ryan7 years ago

Patch runs wp_update_plugins() via cron every 5 minutes and every time plugins.php is loaded.

comment:19 @ryan7 years ago

We might want this for 2.6.1 since those with lots of plugins notice slowness in the admin.

comment:20 @Otto427 years ago

Running this via cron is not a great idea, methinks. Lots of people still have issues with the cron system, I wouldn't put important things like update checks on it yet.

There's no reason not to have it hit the update function every page hit, the fix simply needs to be where it's checking the plugin data to see if the installed plugin list has changed. That's the bottleneck.

comment:21 @ryan7 years ago

If they have problems with cron they'll probably have problems with the update check since both use fsockopen.

comment:22 @Otto427 years ago

Not necessarily. Some of the problems I've seen were a broken cron (future posts not working etc), while other uses of fsockopen to external hosts worked fine (rss feeds showed on dashboard without issues). These were basically cases of bad DNS or some such, where a server could resolve anybody else except its own hostname, or if it resolved it, it could not connect back to itself (loopback issues). I have no idea why (I am not a network admin guru), but the issue is surprisingly commonplace.

comment:23 @ryan7 years ago

How about having wp_update_plugins() run each time the dashboard is loaded and bail out early if we've checked for updates within the last twelve hours. That'd provide some coverage for those with broken cron without slowing down a whole bunch of page loads for a freakin' version check. :-)

@ryan7 years ago

Check for updates from dashboard every 12 hours

comment:24 @Otto427 years ago

Ergh... I like the fact that it checks to see if the version has changed, and don't want that delayed. It makes it easy for me to test upgrading during plugin development, as I can simply decrement the version number and perform an upgrade.

Here's the real problem as I see it: get_plugins() does a whole crapload of work. It recurses through the plugin directory, reads every bloody *.php file, does regexp in all of them for the plugin headers... Having to do all that every admin page is rough.

My suggestion is to limit *only that part of it* to hits to the plugins.php page. When you go to look at the plugins, then it has to load all the plugin data anyway. No loss there on load time (and it's using caching too, so that will work out just fine). For all the rest of the admin pages, we only do the 12 hour check.

comment:25 @Otto427 years ago

There. No need for a cron job at all. It's already doing a 12 hour check inside wp_update_plugins, so adding a cron job for it is rather pointless.

comment:26 @ryan7 years ago

That looks like it makes running wp_update_plugins() on any page other than plugins.php useless since we don't have a list of plugins to send to api.wordpress.org.

@ryan7 years ago

Cron every 12

comment:27 @ryan7 years ago

Here's my pitch:

  • Cron job to schedule async updates every 12 hours from either admin or front page loads
  • Update syncronously from dashboard if no update in > 12 hours. This offers a fallback update method for those with bad cron that doesn't create synchronous updates on other page loads
  • plugins.php always search plugins for changes. Handy for those that are testing.

I never want a plugin scan or api.wp.org request to slow down a request for the Write Post page, for example. Not even once every 12 hours. It just looks bad. Taking the hit on just the dashboard is tolerable. Never, ever, ever should we check for updates synchronously on the front page.

@Otto427 years ago

Only check for plugin data changes when loading plugins.php anyway (corrected version)

comment:28 @Otto427 years ago

Oops. Fixed diff now attached. It'll have the plugin list if it needs it.

I don't like your idea, ryan, because you're very much limiting where it's allowed to check. I dislike that a lot. I want it to never, ever, not check. In your proposal, for example, what if I never visit the dashboard? Many people don't after all, they just make bookmarks to the part of the admin they hit the most often.

A cron job is fine, if it can *always* check synchronously as a backup. Not just on the dashboard, but anywhere. Yes, even when you hit Write Post. If it's time to check, then it's time to check.

The maybe_check is a good idea and I support it, as long as you hook it to admin_init instead.

Also, call it "twicedaily" instead of "12hours". I've seen other plugins and people appending to the cron stuff and they've all used that so far.

@ryan7 years ago

Check if update needed on every admin page load

comment:29 @ryan7 years ago

Okay, fair enough. I moved the maybe check to admin_init. Cron stays so that updates can be triggered from front page loads. If cron doesn't work for someone, they still have the update check done on admin page loads.

comment:30 @ryan7 years ago

(In [8514]) Don't run get_plugins() on every admin page load. Use cron for async update plugin requests. see #7372

comment:31 @ryan7 years ago

(In [8521]) Don't run get_plugins() on every admin page load. Use cron for async update plugin requests. see #7372 for 2.6

comment:32 @jacobsantos7 years ago

I was wondering: Why don't you just check the plugins that we know exist instead of seeking for new plugins? It stands to reason that if a person is going to upload a plugin, then to activate the plugin they'll have to go to the one place that will update the list of available plugins and also check whether there is an update for said plugin. We can save a lot of time just checking the plugins that we know exist.

comment:33 follow-up: @DD327 years ago

I was wondering: Why don't you just check the plugins that we know exist instead of seeking for new plugins?

Its more of checking that the plugin still exists rather than checking to see if new plugins have been installed AFAIK.

comment:34 in reply to: ↑ 33 @jacobsantos7 years ago

Replying to DD32:

I was wondering: Why don't you just check the plugins that we know exist instead of seeking for new plugins?

Its more of checking that the plugin still exists rather than checking to see if new plugins have been installed AFAIK.

Makes sense, but you can still check for that file existence and run the plugin_data on just that file instead of checking the entire directory. My follow up question is if the entire plugins directory is still being checked or if my suggestion is already implemented.

comment:35 @jacobsantos7 years ago

This is fixed correct?

comment:36 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.