Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Spin-off from #347988: Move $user->data into own table:
Shouldn't the very same pattern we apply to cache items also apply to variables?
The cached variables are nuked whenever variable_set() is invoked. So variables from {variable} are processed quite often.
I've included an update that performs the database change. To properly benchmark this, we'd have to put the following onto the end of index.php:
cache_clear_all('variables', 'cache');
However, my own system is known to be unsuitable to provide proper benchmark results. So would be cool when someone else could do that.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | variable_head.png | 60.2 KB | catch |
| #15 | variable_patch.png | 44.01 KB | catch |
| #14 | drupal.variables-serialized.14.patch | 5.39 KB | sun |
| #12 | drupal.variables-serialized.10.patch | 5.3 KB | asimmonds |
| #10 | drupal.variables-serialized.9.patch | 4.13 KB | asimmonds |
Comments
Comment #2
sunoopsie ;)
Comment #4
catchSubscribing. This seems like a good change to me.
Comment #5
sunNot sure why it fails to install.
Comment #7
asimmonds commentedsystem_update_7040() should be system_update_7046()
Comment #8
sunheh, thanks - I guess that explains it ;)
Comment #10
asimmonds commentedThere is a unserialize() in install.php when fetching install_task from the variable table (there must be a valid reason why variable_get() is not used here, probably because variable table may not yet exist when called)
Anyway, assuming install_task will always be a string, just removing the unserialize() for now and let the testbot do it's thing.
Comment #12
asimmonds commentedAnother unserialize() in modules/dblog/dblog.test
Comment #14
sunRe-rolled against HEAD.
Comment #15
catchThis needs a re-roll for system_update_N() changes, also doesn't it need to be in update_fix_d7_requirements()?
I profiled a page before/after the patch and it didn't look very encouraging - the main cost is the cache_set(), not the unserializing of variables, which is extremely cheap. On a site with a lot of contrib modules we'd see more of an improvement, but given we call unserialize() a few dozen times on normal requests I'm not sure it's going to make enough of a difference. However it's a transparent API change, and a schema change that no-one should ever notice, so not moving to D8 just yet. We'd probably need a database with 1,000-1,500 rows in the variables table to make this more testable. I have one of those but kinda waiting until the upgrade path is without obvious errors before I want to try upgrading it for testing.
Comment #16
sunYes, on clean HEAD, there are almost no variables.
Could we append the following to index.php (once) before applying the patch and profile once again?
Let me also note that we should only do this if there is a visible performance improvement. I'm slightly worried that not serializing integers (
123) will always return them as strings ("123"), because {variable}.value is of type text. The bot seems to be happy, so this is not necessarily a technical problem - however, type-agnostic comparisons of integer variables won't work this way. OTOH, all integer variables that are stored via system_settings_form() and forms in general are stored as strings anyway already... well, just wanted to mention it. ;)Comment #17
catchOK, generated the 500 variables, and the array_map() is still only 1% of variable_initialize(), there's a chance that using array_map() with a callback hides some of the performance implications, but I think it's more likely that it's just fast. Afraid this is won't fix.