register_uninstall_hook() causes large amounts of unnecessary option updates

[jeichorn]

register_uninstall_hook hook causes a write to wp_options everytime it is run. Plugin authors don't seem to have noticed this (this fact isn't made clear in the documentation), so they are registering it like any other hook instead of inside of their activation hook.

While you could update the documentation, fixing all the existing themes would be a huge chore (in a same of 100 plugins 5 are calling the hook on every page load).

Removing the write is easily doable with a couple line patch (attached). This is not optimal since it still causes an extra database read, but its much better at least.

[jeichorn]

wp-include/plugin.diff

[MikeHansenMe]

Patched from src dir with a few format changes. Code is all the same as original patch.

[DrewAPicture]

31792.diff​ still applies as of r32294.

[jeichorn]

What can I do to get some movement on this ticket?

[jorbin]

On the face, This looks good. I want to do a bit of testing, but I think this could help remove a DB write when it is unneeded.

[dd32]

This shouldn't be causing any extra DB writes realistically.

update_option() should (and does) notice that the array is unchanged, which prevents the writes occurring.
The only way for this to trigger a write on every call of the function, would be if the callback value was changing every pageload, and if so, then that couldn't be used as a uninstall callback in the first place..

That being said, there's nothing wrong with the attached patch, it just handles skipping the update at a higher level.

[obenland]

@jorbin could you revisit this and and determine in which direction you want to take it?

[obenland]

No movement in a month, let's try in a future release.

[jeichorn]

Replying to dd32:

This shouldn't be causing any extra DB writes realistically.

update_option() should (and does) notice that the array is unchanged, which prevents the writes occurring.
The only way for this to trigger a write on every call of the function, would be if the callback value was changing every pageload, and if so, then that couldn't be used as a uninstall callback in the first place..

That being said, there's nothing wrong with the attached patch, it just handles skipping the update at a higher level.

I missed this reply. I've seen this heavy write behavior on multiple sites, the driver of writing this patch was seeing the query piling up in the processlist. Any thoughts on what would drive update_option check to fail, when the value isn't changing?

[dd32]

I missed this reply. I've seen this heavy write behavior on multiple sites, the driver of writing this patch was seeing the query piling up in the processlist. Any thoughts on what would drive update_option check to fail, when the value isn't changing?

@jeichorn I can guess that perhaps an Object cache isn't playing nice, or maybe a value being stored within the option is invalid? Can you post an example of one of the UPDATE's if it doesn't contain anything sensitive?

[jeichorn]

Replying to dd32:

I missed this reply. I've seen this heavy write behavior on multiple sites, the driver of writing this patch was seeing the query piling up in the processlist. Any thoughts on what would drive update_option check to fail, when the value isn't changing?

@jeichorn I can guess that perhaps an Object cache isn't playing nice, or maybe a value being stored within the option is invalid? Can you post an example of one of the UPDATE's if it doesn't contain anything sensitive?

I will see if i can find a problem case again. It was a # of different customer sites, but I don't think i've see any problems recently.

[MikeHansenMe]

31792.diff​ still applies. @jorbin want to take a look and see if this is something we want to get in for 4.5?

[jeichorn]

So this finally happened to me again. I was able to trace down the cause.

Objects are being stored in the uninstall_plugins due to the uninstall being callbacks being on classes. The un-serization happens in the first plugin that calls register_uninstall_hook, but the classes used by other plugins aren't loaded yet, set they un-serialize as PHP_Incomplete_Class. Then when the next plugin calls register_uninstall_hook, its serializing an object with the actual class name so its a different value in PHP, so the comparison in update_option fails.

[dd32]

Replying to jeichorn:

So this finally happened to me again. I was able to trace down the cause.

Objects are being stored in the uninstall_plugins due to the uninstall being callbacks being on classes. The un-serization happens in the first plugin that calls register_uninstall_hook, but the classes used by other plugins aren't loaded yet, set they un-serialize as PHP_Incomplete_Class. Then when the next plugin calls register_uninstall_hook, its serializing an object with the actual class name so its a different value in PHP, so the comparison in update_option fails.

This should be prevented since WordPress 3.1 - [16339]. Can you see how that's being bypassed?

I have a feeling this may be caused because the plugin registered it prior to that code being added in 3.1, and ultimately having that value stuck in the array?
If the plugin were to fix itself and register a valid uninstall hook, the issue would clear itself - but I'm guessing the plugin never did.

[jeichorn]

So it turns out there are a bunch of entries like that in the array, but they are all from plugins that are no longer installed.

But it seems that just sitting around is enough to cause the problem.

So i guess the correct fix is too cleanup the uninstall_plugins option. Is that the sort of cleanup thats reasonable to do as part of the upgrade process? Or something that is best handled outside of wordpress?

[dd32]

So i guess the correct fix is too cleanup the uninstall_plugins option. Is that the sort of cleanup thats reasonable to do as part of the upgrade process? Or something that is best handled outside of wordpress?

A upgrade routine to clear out any non-function/static method callbacks seems like something we can do here.

[Rahe]

This could be nice that this patch is merged saving multiple update queries.
We have sites with 8 to 10 plugins using this function and making this much calls at every admin/ajax page load.

Is the cleanup of the array can be moved to an another ticket since the performance of the function is an another subject ?
So we can merge this ticket and gain performance :)

[tha_sun]

I can confirm that this is still a problem. Excessive write and delete operations are not a problem when looking at them in isolation, of course. But they do become a problem with increased concurrency (server responds to many parallel requests). And if the site uses an external (object) cache, it causes a higher latency and longer processing queues, up to a point where the additional data traffic nulls the effect of the cache.

Rule of thumb: GETting a page anonymously must only be a read operation internally. (idempotence)

As the API function is used by a lot of plugins, it does not appear feasible to follow the path of fixing the calling code. Instead, we should fix the API function to be smarter.

In particular, the function can simply check the currently stored option value before writing the same value again. This ensures that new values are picked up immediately, while the API stays the same, while regular page views do not perform a write operation.

Attached patch implements that.

[tha_sun]

This performance optimization patch is applied to several of our production sites in the meantime and did not cause any problems.

Some of our websites are set up using Redis as an object cache. We found this bug after identifying a poor cache efficiency caused by many unexpected key writes during regular page view requests of anonymous visitors.

Measuring the performance impact of this isn't easily possible, because it mainly affects the server operations under higher load and concurrency. Some sites appear to be up to 200ms faster under load with the patch applied.

[tha_sun]

The patch retains the currently existing API behavior as-is, so it is fully backwards-compatible.

It only prevents the function from re-saving the option with the identical value that it had before already.

Doing so prevents the option cache from being rewritten on every request.

[tha_sun]

There was an unnecessary second call to plugin_basename(), which has been optimized in attached patch.

Also corrected the coding style. I originally followed the style of the file, but I assume that Core wants all new code to follow WordPress's actual standards.

[tha_sun]

This is a very trivial patch that improves the performance of all WordPress sites. What can I do to move this forward?

[andraganescu]

The latest patch looks good, a simple change that offers a performance benefit.

[kirasong]

Refreshed wordpress.register_uninstall_hook.31792-23.patch

[whyisjake]

In 46333:

Options: register_uninstall_hook() causes large amounts of unnecessary option updates.

Prevent extra database read/writes from the the hook.

Props jeichorn, MikeHansenMe, DrewAPicture, Rahe, tha_sun, mikeschroder.
Fixes #31792.