Performance issues from strongarm_init()

Strongarm is a real memory hog. We've had staging servers give up on strongarm to the point that we had to restart the server.
Throwing away strongarm always solved the problem.

We did some investigation but I can't remember our conclusion. I think it had something to do with the cache being rebuilt on every page load.
We havent given up on strongarm yet though. We still use it to get settings from A to B, but the module never goes on a production environment.
We're also looking into a way to preserve strongarm without the huge performance hit.

hi krisahli, just wanted to let you know I put up my exprimental fork..
http://drupal.org/sandbox/stijndm/1134952

http://drupal.org/sandbox/stijndm/1134952 is great - this should be a patch against the module.

Here's a description of what strongarm currently does:

1. Every request, reloads the variable cache in hook_init(), along with strongarm's own cache. This triples the number of cache requests to build $conf compared to core.

2. If the created date of the variables cache differs from the created date of the strongarm cache (which will be the case after any variable_del() or variable_set() - these can be called very often by core and contrib modules other than settings pages), it does a full rebuild of its own cache. This full rebuild triggers a full schema cache rebuild and can take up to a second.

Marking needs work since there is code to review, but it needs to be in proper patch form.

ooooh no :(

I just gave the sandbox of stijndm (thanks!) mentioned by catch in #5 a try and decided to create a patch.
I've posted the patch here: #1229506: Patch against strongarm
Would be nice if someone could give it a review.

We've got Stijn's fork running on some production sites. Proven stable and a good performance increase btw. This needs to be in the strongarm module by default.

I've re-rolled das-peter's patch and added a couple more patches that were posted against the sandbox.

Hi catch, I'm sorry but I couldn't apply your patch against 7.x-2.x, also you seem to have a lot of trailing whitespaces.

I fixed an error in das-peter's patch #1229506: Patch against strongarm that broke reverting variables.

Edit: this patch is against 7.x-2.x!

Sorry catch, das-peter just pointed out that your patch is against 6.x-2.x, and indeed this ticket is for the 6.x version.

das-peter also mentioned, that you had already (partially) found the error I mentioned in #11: #1240308: Features revert code has an error.

Rerolled your patch and fixed the trailing whitespaces.

Now :-) … this is getting a real mess! catch has done some nice work and I would really appreciate to have catch's work merged into mainline Strongarm as soon as possible. Otherwise contributing will become really painful! Is there any reason not to merge?

Just a note the vast majority of this is not my work, that is all stijndm's, I've just done a bit of testing and posted a patch for a small bug.

Since it's not actually my patch, I'm going to go ahead and mark this RTBC, I agree it'd be good to get this merged as soon as possible - the performance issues in strongarm are very serious, the solution here is very elegant, and having the main project plus a fork (plus patches against the fork) isn't an ideal situation at all.

I guess we should re-roll the patch since I've found some issues and fixed them in the 7.x-2.x patch right now. The changes made are described to make a re-roll easier: #1229506-3: Patch against strongarm

Subscribe.

subscribe

Ouch. Subscribed.

Wow, this is basicly everything I hoped and dreamed for. We need to get this in =)

Subscribing, with a hope of being able to help out once I wrapped my head around all this.

subscribe.

The patch catch posted in #10 has been working well, but I think it's missing an update hook.

As it is, any variable in strongarm that's not yet in the db will just be unassigned, and your features will not show as overridden. The attached patch adds an update hook to take care of this.

It also cleans up some whitespace and removes some commented out code.

Last patch depended on features, new one doesn't.

New update looks much better. Marking this back to CNR.

@das-peter I've closed the issue against faux-strongarm in favour of trying to concentrate on 6.x/7.x patches here. It'd be good to roll your fixes into the patches on this issue so everything's in sync.

Hey everyone, sorry for the lack of response. We'll be picking things up here shortly.

Personally I have been focusing on 7.x stuff, any clue how applicable this is to 7.x? (about to start investigating)

So far 3 bits of feedback on the patch.

1) Looks like the $conf global in strongarm_admin_form can go away.

2) Inside variable_features_revert, the line..

if (isset($variable->in_code_only) || ($default->value != $vars[$name]->value)) {

References

$variable->in_code_only

, but there is no

$variable

defined. Did you mean

$default

?

3) There are 2 instances of

//ctools_component_features_revert('variable', $module);

Assuming those should just be removed?

Subscribe; will try it out when have time to play

Here's a new patch that should address febbraro's concerns.

I also removed my update hook because I now see that the only reason it was necessary was because variable_features_revert was broken. This patch pushes variables in code to the db on features revert just fine.

afaik the 7.x branch has exactly the same design although I've not reviewed it in depth. The last 7.x patch was posted in #1229506: Patch against strongarm but should be moved here as well.

@catch I'll roll a patch for 7.x

@msonnabaum Thanks. I saw 2 more things that I had questions about.

1) In strongarm_admin_form is the $hardcoded variable is no longer needed? Or is the patch now missing the check where a Strongarm variable is also hardcoded into the $conf array in settings.php?

2) In variable_features_rebuild this line

$vars = strongarm_vars_load(TRUE, TRUE);

does not look like $vars is used anywhere, guessing it can be removed?

Looking good, thanks!

@febbraro: Let me know if something is wrong / missing in the already existing 7.x patch here #1229506-3: Patch against strongarm In that case I'll do a re-roll asap.

I'm not really aware what happened in the last 6.x patches, but the last time I checked the 7.x patch was ahead of them and contained some changes that need to be back-ported.

@das-peter Got it, thanks, will dig in there. The only thing about the existing patch is related to 1 in #29 w/r/t the $hardcoded variable.

In an attempt to integrate the various queues into one for getting this done...

This is the D7 version of the patch, a slight modification to keep the Hardcoded notion around where a variable is overridden in settings.php.

Here's another D7 version.

Very similar to febbaro's above, but I cleaned up the admin section and tried to make it a bit easier to parse.

@msonnabaum Do you mean D7 or D6, I only ask b/c of a d6 in the patch file name.

@msonnabaum Also, I like your admin changes, that part was hard to interpret and also did not show the values in the table that I expected.

Had to make a slight mod to the patch. If the variable was not in the DB at all the first check for hardcode would throw a warning.

Attached is a new version of the D7 patch with @msonnabaum's admin changes.

So the patch in #32 is not working quite as expected in the scenario that the variable is in code but there is no record of the variable in DB. The simplest way I could test this was by exporting a Feature with the site_name variable, then doing a drush vdel site_name. The strongarm admin screen now shows the site_name variable as "In code" but Feature revert (even with a force) will not put the variable into the DB.

Attached patch fixes that, the variable_features_revert was testing the $default object and not the $vars[$name] object. The latter only had the in_code_only property.

Yes, sorry about the file naming, that was D7.

And I did forget my revert fix from #22 as well, which you got there in #37.

Here's basically the same patch, but without carriage returns in the drush command, and without the added REVERT header as this wasn't there before and makes the column awkwardly long.

Sorry, wrong patch.

And here's a D6 version.

So I think the patch is looking great, but there is one thing left to resolve...

This patch effectively makes Strongarm variables a "faux-exportable". The part I have not yet decided on is if we have a Strongarm variable in code and it is removed from the database, should a Feature diff show the variable as removed or not? and beyond that should a Feature update (fu!) remove it from the Feature.

I have the code working to remove the Strongarm variable on a feature update if they are not in the DB I'm just not entirely sure that is correct from a conceptual level. Before this patch the system would still use the "in code" value from the Strongarm variable via variable_get(), but now if it only lives in code that variable does not exist via variable_get(). I'm leaning towards an fu removing it, but wondering if anyone has a justification for not doing that?

(also moving this to D7 for now)

Here is the patch from #41 if anyone wants to check it out.

+1!!!! Awesome work here.

I have the code working to remove the Strongarm variable on a feature update if they are not in the DB I'm just not entirely sure that is correct from a conceptual level.

I believe this is the direction being taken by the D8 config management, and thus, is conceptually sound. The web interface is the primary source for configuration.

I'm leaning towards an "fu" removing it, but wondering if anyone has a justification for not doing that?

I don't have any reasons to *not* do this, but would like to remind everyone that if you really need to hardcode any strongarm setting into the code, such that it cannot be changed from the interface, regardless of if it has a value or not in the database, use the $conf[] variable in your settings.php file. :)

@febbraro, as this is the way other faux-features (like fields) work I think this is the right strategy, but in general it seems like we are missing using a NULL value here. Here is the use case where there is a problem (and this is also true for fields)

1. A variable is currently set in a feature.

2. On a dev env one delete the variable (it needs to not be set for some reason) and then does an update and checks in the code. Now the variable is not in the exported module

3. when the code is add to prod and a feature revert is run, nothing happens.

If instead the variable is set to NULL on export,then in the revert the variable would be unset in prod. And if someone want the variable to not be part of the feature then they cans still removed it as an item to be exported.

But I think that it is better to be consistent and have all faux-features do the same thing.

Ok, one last re-roll. This reintroduces msonnabaum's update hook. This removed the need to revert every Feature when this new version of Strongarm is installed. Going to run a few more tests then commit. Speak now....

@hefox thanks, cache_clear_all was not only clearing the wrong bin, it was not needed at all.

Looks good. Tested the attached D6 version and it works quite well.

The noticeable difference in performance is definitely visible on the features screen, provided you have your share of exported variables. Tested on my end and it works well too.

Actually, ignore that last patch, this one is good.

Sweet, thanks everyone, patch committed to 7.x: http://drupalcode.org/project/strongarm.git/commit/feb9b80

Looks like #49 still has some issues. Looking into it, should have a new patch soon. (unless @msonnabaum already has one).

Yeah, sorry, there was an issue in that last patch.

Here's a new one.

Thanks @msonnabaum, great patch. Still had one last d7 call in to drupal_installation_attempted. Replaced it with a direct check on the MAINTENANCE_MODE in this patch.

Tested and working.

Committed to 6.x-2.x https://drupal.org/node/455454/commits

We were having issues with cron thinking it was still running for along time, but there was no cron_semaphore variable. It took some time to find, but we finally found strongarm setting $GLOBALS['conf']['cron_semaphore'] from cache_get('strongarm', 'cache'), while cron_semaphore was not in the main variable cache (cache_get('variables', 'cache')) or the {variable} database table. Strongarm's hook_init()/strongarm_set_conf() were responsible, which lead me here. (Also, FWIW we're using memcache.)

We're applying the patch to to 6.x-2.x, but two things look like they still need to be addressed:

- Shouldn't hook_update_N() at least return an empty array?
- strongarm.drush.inc didn't make it into the 6.x-2.x commit

+1 to always returning an array in the update hook.

I've re-tested this code against the 6.x-2.x branch and have found no issues.

we finally found strongarm setting $GLOBALS['conf']['cron_semaphore']

I just filed a feature request to try to avoid situations like ^ from happening: #1302064: Variables list to exclude from cache / export

For the record...

Cut the page execution time in half. Awesome improvement.

And besides that it scattered the discussion about where to trigger strongarm in the bootstap ;)

There's a slight issue with the features_revert hook in 6.x-2.x. The if statement only checks for equivalence not that the two variables are identical so arrays stored as variables, even if they don't match the feature, won't be reverted. I've attached a patch which fixes this issue.

I've not checked 7.x so the bug may exist there as well.

The D7 patch already uses:

if (isset($vars[$name]->in_code_only) || ($default->value !== $vars[$name]->value)) {

Thus I'd say #60 should be absolutely fine.

#55 and #60 both look good. Here they are as a combined patch with no other changes. Moving back to RTBC.

Just subscribing. Awesome work!

Good catch, I've committed your patch to 6.x-2.x. Thanks for the assist!

doh. forgot to change the status. Patches committed and 6.x-2.1 being rolled.

The patch in #53 seems to be triggering an undefined MAINTENANCE_MODE constant error as it's not always defined. Patch coming in a sec.

Issue and patch filed at http://drupal.org/node/1489792 (eek - and accidentally attached the patch here, too - feel free to ignore)