search

humanmade / Cavalcade

A better wp-cron. Horizontally scalable, works perfectly with multisite.
https://engineering.hmn.md/projects/cavalcade/
Other
517 stars 46 forks source link

Use object cache #26

Closed tillkruss closed 7 years ago

tillkruss commented 7 years ago

We've noticed up to 40 duplicate database queries using Cavalcade, which is adding around 100-150ms to each page load for us, even with DISABLE_WP_CRON set.

screen shot 2016-11-06 at 1 09 10 pm

This PR adds wp_cache_get() to HM\Cavalcade\Plugin\Job::get_by_site() and deletes the cache when saving/deleting jobs.

I also went ahead and made the coding style more uniform.

tillkruss commented 7 years ago

@rmccue: I removed the coding standard changes and made the cavalcade-jobs a non persistent group.

rmccue commented 7 years ago

@joehoyle Can I get a sanity check from you on this? Can you think of anything that'd break?

joehoyle commented 7 years ago

I think this should be ok - it's possible that we could get races because of it, though we are caching all jobs together. In the event of the runner starting / completing a job half way through the script execution for the web request you'd potentially have stale data in the cache, for example:

  1. Starts web request, jobs are cached
  2. Cavalcade runner starts a job a
  3. Web request from 1. check if a job if job a is queued, it thinks it is, i.e. not running
  4. Logic fails as job a is not known to have started.

However, realistically I can't really see that being an issue, it would have to be a very obscure case where this would be an issue - but it is technically an issue.

tillkruss commented 7 years ago

@joehoyle: Is that scenario too risky to have this PR merged? I don't see a way to prevent that stale cache from happening.

joehoyle commented 7 years ago

So, I think we can probably role with this, depending what @rmccue thinks. I think it's probably worth it. In most cases, this just means that one script execution will mean one cached list of jobs.

There's really no way except for trying to load object-cache.php into the Runner which I don't think is a great thing to do.

tillkruss commented 7 years ago

@joehoyle: Regarding the "jobs:{$this->site}" cache key. I don't see Cavalcade Runner calling wp_cache_switch_to_blog() or setting the global variable $blog_id that's why site key is necessary IMO. Unless I'm missing something?

rmccue commented 7 years ago

(Just a quick note: avoid rebasing in PRs please, just push your changes. :) )

tillkruss commented 7 years ago

@rmccue: I removed the site id from the cache key.