Don't overload the variable table - performance improvement
| Project: | OG User Roles |
| Version: | 5.x-3.7 |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Possibly related to the discussion at #321965: Performance improvements.
We're using og_user_roles module on a site, and have ended up with over 4700 og_user_roles variables in Drupal's variable table. This is having a negative impact on our site's performance. This is because the data in the variable table is cached and stored in $conf, which means it is loaded on every page. With over 4700 variables for this module alone, it is just too much data to load on every page.
I've patched the module so it now has it's own table for storing its settings, og_user_roles_variable. I added three new functions to replicate the existing variable_get, etc functionality (e.g. og_user_roles_variable_get()). All variables for this module are now stored in the og_user_roles_variable table and fetched from there when needed. I've also included an update handler to create the table, and move over all existing settings.
Cheers,
Stella
| Attachment | Size |
|---|---|
| og_user_roles_variable.patch | 64.03 KB |
#1
I think that we only need to move the variables with $gid in them to their own table. I propose keeping the normal variables in the {variable} table (with one modification to this below). I think the extra unhashed SELECT and INSERT of the new og_user_roles_variable functions will actually have a negative performance impact.
I'd also change the vars that use $type to be an array'd value, so that we only have a single variable.
This will reduce the number of variables to order N, where N is the number of variables really needed. Currently the number of variables is N + M * num-groups + P * num-types.
#2
#3
New patch to address the concerns raised by douggreen above.
#4
This probably is far enough along to solve my immediate concerns. However, the next step, now that the og_user_roles_variable_get/set are based solely on gid, it makes sense to have a real table with a key of gid, and columns for each of the variables. You'd then add a get function that cached the results by gid, and a set function that just updated the single column.
#5
Actually while thinking about this, I think it makes sense to have two tables, one for type data and one for gid data, and these tables should be real tables with columns and not just the variable style.
We're missing a few variable_del's in the .install file
variable_set('og_user_roles_group_'. $type, $group_info);+ variable_del('og_user_roles_roles_'. $type);
+ variable_del('og_user_roles_roles_assign_typegrouprole'. $type);
+ variable_del('og_user_roles_roles_typegrouprole_value'. $type);
Do you really need to replace system_settings_form, can't we just add our submit handler using #submit? or is this necessary to handle reset defaults?
#6
Attached patch adds in the missing variable_del() calls.
The system_settings_form() can't be used if you want to override how the 3 separate variables are merged into one. I suppose you could try adding a #submit handler to process just those settings and then unset them in $form_values but that would make the submit handlers order dependent. I don't know which would be called first and if it's not our submit handler then you'll still have those three separate group settings in the variable table, so no advantage. I wonder if using FAPI's #tree would help, but I imagine it would only help with the structure the submit function receives those values in.