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

CommentFileSizeAuthor
#6 509342.patch32.81 KBstella
#3 509342.patch32.64 KBstella
og_user_roles_variable.patch64.03 KBstella

Comments

douggreen’s picture

douggreen commented

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.

douggreen’s picture

douggreen commented
Status: Needs review » Needs work
stella’s picture

stella commented
Status: Needs work » Needs review
StatusFileSize
new 509342.patch32.64 KB

New patch to address the concerns raised by douggreen above.

douggreen’s picture

douggreen commented

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.

douggreen’s picture

douggreen commented

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?

stella’s picture

stella commented
StatusFileSize
new 509342.patch32.81 KB

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@douggreen/stella: Do you still need these patches in or can we close this (and the other) issue? With the rise of the rewritten OGUR 4.x for Drupal 6, in which many unrelated features of OGUR were removed, and nearing a release of Drupal 7, I'm currently closing down old issues...

stella’s picture

stella commented

I don't believe we need this any more.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Closed (won't fix)

Thanks! Feel free to re-open though.