WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 weeks ago

#31792 reviewing defect (bug)

register_uninstall_hook() causes large amounts of unnecessary option updates

Reported by: jeichorn Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-patch needs-testing bulk-reopened
Focuses: performance Cc:

Description

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.

Attachments (4)

plugin.diff (1.1 KB) - added by jeichorn 4 years ago.
wp-include/plugin.diff
31792.diff (1.1 KB) - added by MikeHansenMe 4 years ago.
Patched from src dir with a few format changes. Code is all the same as original patch.
wordpress.register_uninstall_hook.31792.patch (817 bytes) - added by tha_sun 3 months ago.
wordpress.register_uninstall_hook.31792-23.patch (836 bytes) - added by tha_sun 2 months ago.

Download all attachments as: .zip

Change History (26)

@jeichorn
4 years ago

wp-include/plugin.diff

@MikeHansenMe
4 years ago

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

#1 @MikeHansenMe
4 years ago

  • Keywords has-patch needs-testing added

#2 @DrewAPicture
4 years ago

  • Component changed from General to Options, Meta APIs

31792.diff still applies as of r32294.

Last edited 4 years ago by DrewAPicture (previous) (diff)

#3 @jeichorn
4 years ago

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

#4 @jorbin
4 years ago

  • Milestone changed from Awaiting Review to 4.3

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.

#5 follow-up: @dd32
4 years ago

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.

#6 @obenland
4 years ago

  • Owner set to jorbin
  • Status changed from new to assigned

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

#7 @obenland
4 years ago

  • Milestone changed from 4.3 to Future Release

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

#8 in reply to: ↑ 5 @jeichorn
4 years ago

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?

#9 follow-up: @dd32
4 years ago

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?

#10 in reply to: ↑ 9 @jeichorn
4 years ago

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.

#11 @MikeHansenMe
3 years ago

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

#12 follow-up: @jeichorn
3 years ago

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.

Last edited 3 years ago by jeichorn (previous) (diff)

#13 in reply to: ↑ 12 @dd32
3 years ago

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.

#14 @jeichorn
3 years ago

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?

#15 @dd32
3 years ago

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.

#16 @Rahe
3 years ago

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 :)

#19 @tha_sun
3 months ago

  • Focuses performance added
  • Keywords bulk-reopened added
  • Summary changed from register_uninstall_hook causes large #s of unnecessary updates to wp_options to register_uninstall_hook() causes large amounts of unnecessary updates to wp_options

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.

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

#20 @tha_sun
3 months ago

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.

#21 @tha_sun
3 months ago

  • Summary changed from register_uninstall_hook() causes large amounts of unnecessary updates to wp_options to register_uninstall_hook() causes large amounts of unnecessary option updates

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.

#22 @SergeyBiryukov
3 months ago

  • Milestone set to 5.3
  • Owner changed from jorbin to SergeyBiryukov
  • Status changed from assigned to reviewing

#23 @tha_sun
2 months ago

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.

#24 @tha_sun
3 weeks ago

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

Note: See TracTickets for help on using tickets.