Skip to content
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

Backport/Implement State/Key-Value storage system #141

Closed
quicksketch opened this issue Dec 24, 2013 · 5 comments
Closed

Backport/Implement State/Key-Value storage system #141

quicksketch opened this issue Dec 24, 2013 · 5 comments
Assignees

Comments

@quicksketch
Copy link
Member

Quote from the drupal.org issue: https://drupal.org/node/1175054

There are several core systems (and many more modules) that store things in the variables table that aren't actually configuration.

For example:

  • The installer
  • The path alias whitelist
  • The default caching backend (and memcache.inc uses variables too reluctantly).
  • The css and js aggregation systems

Also menu.inc has variable_set('menu_rebuild_needed');, cron has semaphore, there are lots in core, probably far more in contrib.

Most of these have the following in common:

  1. They are values that are never, ever set via the UI or custom code/config, sometimes an update might force a rebuild but that's about it.
  2. Some of them are not really cache items - more for tracking state between requests (and different sessions).
  3. Some that look like cache items (path alias whitelist, css/js) are extremely expensive to build and don't necessarily need to be wiped on a cache flush (ability to write through to a persistent caching backend like the database, redis or mongo but use memcache or apc for gets could be useful for that - http://drupal.org/sandbox/catch/1153584 has some notes on this but no code).
  4. They can be very volatile and prone to stampedes - this makes them poorly suited to the current variables system where they are lumped in with loads of other things, see #987768: Optimize variable caching , #886488: Add stampede protection for css and js aggregation and others.
  5. Some of them are requested on more or less every request to Drupal (path alias, css/js, caching state at least) - in this case they really take advantage of the single variables cache entry to avoid i/o. Also most are simple key => value, either that or some kind of array - neither are particularly suited to keeping in a custom table, the current variables table + cache is not too bad for this kind of simple and/or schema-less data.

Drupal 8 Changelog (for code samples): https://drupal.org/node/1787318

A followup to the state system issue was recently resolved to add caching to the state system here: https://drupal.org/node/1786490

The end system for Drupal 8 is as-usual a bit more abstract and indirect than we'd likely need in Backdrop. Instead of a swappable state system built on top of a abstract key-value store, we may simply want to replicate the existing variable system verbatim call the resulting functions state_set() and state_get(). This way we maintain all the existing functionality in a way that's familiar to developers and maintain the efficiency of an entirely-load always-available system.

@quicksketch
Copy link
Member Author

I'm making progress on this issue at https://github.com/quicksketch/backdrop/compare/141;state_system?expand=1. I've added a simple API for state_set/get/del that mirrors the variable table schema but doesn't load the entire table all at once. We may need to add some sort of global caching that mimics what variables did before, since some state settings (maintenance mode, css/js query string) will clearly be needed on every single request. Drupal 8's key-value store supports multiple load and delete, but doesn't actually use the multiple load in a way that reduces queries for most page loads that I can tell.

@quicksketch
Copy link
Member Author

I filed a PR at backdrop/backdrop#125.

This PR includes conversion of the following variables to the state system:

  • cron_key
  • cron_last
  • css_js_query_string
  • install_task
  • install_time
  • maintenance_mode

Overall, this matches the basic functionality of D8 but with a different approach (simple procedural code instead of a "service" with a class hierarchy). This currently means 2 additional SQL queries per page (for maintenance_mode and css_js_query_string), same as D8. Although these are going to be quick, easy queries, it is a potential cause for concern.

@quicksketch
Copy link
Member Author

Although these are going to be quick, easy queries, it is a potential cause for concern.

A/B testing for me shows that after this patch, we're adding about 2-6ms per authenticated page request. Anonymous page cache isn't affected by these changes, since none of these state values (or any others) are needed on cached pages.

On an authenticated page request, state_get() is called for 4 different values:

  • maintenance_mode
  • install_task
  • css_js_query_string
  • cron_last

4 different queries for such basic information makes me think that we should treat states similar to variables and select everything from the table and then cache that information. In Drupal 8, there are 15 items in the state table, and 7 are needed on every page load (the 4 we have here plus node.node_access_needs_rebuild, system.private_key, language_count, menu.masks, and menu_rebuild_needed).

If we add caching similar to variables, it means that it would be possible for users to shoot themselves in the foot if a module stored a huge amount of data into the state table, but considering the config system will be more commonly used and provide other advantages (moving between environments and translation), the likelihood of this happening should be less than Drupal 7.

I'll revise my PR to include caching on the state table and post another update on differences.

@quicksketch
Copy link
Member Author

Okay, PR updated with caching (interdiff). This shows only a 1.5ms slowdown per authenticated page request, and shouldn't be affected by additional other conversions to the state system. It also makes it so these values can be pulled from a faster cache backend, such as Memcache or Redis. Overall, this makes the state system nearly identical to the current variables system, just with on-demand loading and a specialized purpose.

@quicksketch
Copy link
Member Author

Merged in backdrop/backdrop#125 to pave the way for #155 and future CMI conversions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants