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

Clear WP object cache periodically on media regenerate/import. #62

Merged
merged 3 commits into from Mar 20, 2018

Conversation

3 participants
@gitlost
Contributor

gitlost commented Jan 15, 2018

For discussion.

Noticed this old report of memory issues using wp media regenerate in slack by @dws122 https://wordpress.slack.com/archives/C02RP4T41/p1511409150000032 and thought it might make sense to call wp_clear_object_cache() periodically like wp import and wp export do.

Testing of wp media regenerate suggests it clears around 3 or 4 MB each time, using the same 500 interval as wp import. Testing of wp media import was less convincing, but seemed to clear around 1 MB each time.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Jan 16, 2018

Member

Oh... I just looked at how the current cache clearing works, and I think that might potentially produce disasters on a high-traffic production site.

For some high-traffic sites, the cache is vital to keep the site online, and a cache invalidation will lead to a cache stampede. If we're forcing such a cache invalidation, without even making that obvious in the documentation or giving people a switch to turn it off, we risk breaking such sites. We will not only mess with the in-memory cache, but also with the persistent (Memcached/Redis/...) cache.

I'd like to first investigate whether we cannot just replace the global variable $wp_object_cache that contains the current WP_Object_Cache instance with a sort of dummy cache object that just not stores anything for these commands.

Member

schlessera commented Jan 16, 2018

Oh... I just looked at how the current cache clearing works, and I think that might potentially produce disasters on a high-traffic production site.

For some high-traffic sites, the cache is vital to keep the site online, and a cache invalidation will lead to a cache stampede. If we're forcing such a cache invalidation, without even making that obvious in the documentation or giving people a switch to turn it off, we risk breaking such sites. We will not only mess with the in-memory cache, but also with the persistent (Memcached/Redis/...) cache.

I'd like to first investigate whether we cannot just replace the global variable $wp_object_cache that contains the current WP_Object_Cache instance with a sort of dummy cache object that just not stores anything for these commands.

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jan 16, 2018

Contributor

Good points ta.

I'd like to first investigate whether we cannot just replace the global variable $wp_object_cache that contains the current WP_Object_Cache instance with a sort of dummy cache object

That sounds like a great solution, easy to implement and likely to have speed benefits as well. I'll close this PR and create an issue for it on wp-cli/wp-cli.

Contributor

gitlost commented Jan 16, 2018

Good points ta.

I'd like to first investigate whether we cannot just replace the global variable $wp_object_cache that contains the current WP_Object_Cache instance with a sort of dummy cache object

That sounds like a great solution, easy to implement and likely to have speed benefits as well. I'll close this PR and create an issue for it on wp-cli/wp-cli.

@gitlost gitlost closed this Jan 16, 2018

@gitlost gitlost deleted the flush-wp-object-cache branch Jan 16, 2018

@danielbachhuber danielbachhuber restored the flush-wp-object-cache branch Feb 28, 2018

@danielbachhuber danielbachhuber added this to the 1.1.5 milestone Feb 28, 2018

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Feb 28, 2018

Member

For some high-traffic sites, the cache is vital to keep the site online, and a cache invalidation will lead to a cache stampede. If we're forcing such a cache invalidation, without even making that obvious in the documentation or giving people a switch to turn it off, we risk breaking such sites. We will not only mess with the in-memory cache, but also with the persistent (Memcached/Redis/...) cache.

Because the underlying utility, Utils\wp_clear_object_cache(), doesn't flush persistent cache, this isn't applicable.

Member

danielbachhuber commented Feb 28, 2018

For some high-traffic sites, the cache is vital to keep the site online, and a cache invalidation will lead to a cache stampede. If we're forcing such a cache invalidation, without even making that obvious in the documentation or giving people a switch to turn it off, we risk breaking such sites. We will not only mess with the in-memory cache, but also with the persistent (Memcached/Redis/...) cache.

Because the underlying utility, Utils\wp_clear_object_cache(), doesn't flush persistent cache, this isn't applicable.

@danielbachhuber danielbachhuber requested a review from wp-cli/committers Feb 28, 2018

@danielbachhuber danielbachhuber requested a review from schlessera Mar 2, 2018

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Mar 2, 2018

Member

Requesting a second review from @schlessera

Member

danielbachhuber commented Mar 2, 2018

Requesting a second review from @schlessera

@schlessera schlessera merged commit 30dfca6 into master Mar 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera deleted the flush-wp-object-cache branch Mar 20, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Mar 20, 2018

Member

I agree that this solves the problem immediately, instead of risking to introduce potential issues through a new mechanism.

However, I will keep the idea of a dummy cache object in mind, as it would move the special cache handling logic from multiple different commands in multiple different repositories into one single, isolated location.

Member

schlessera commented Mar 20, 2018

I agree that this solves the problem immediately, instead of risking to introduce potential issues through a new mechanism.

However, I will keep the idea of a dummy cache object in mind, as it would move the special cache handling logic from multiple different commands in multiple different repositories into one single, isolated location.

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