Parse js files from library definitions during rebuild to minimize variable_set() calls

What would you do with possibly dynamic asset libraries. That is a possibility, locale is actually exploiting that in #2419897: Javascript translations are loaded in the wrong order due to missing dependencies :)

What would you do when you encounter such an altered JS list situation that "people should not do"? I mean if its part of the API someone will have brilliant ideas :) Eg. config schema recently got exceptions if someone uses the schema altering hook to remove or add schema items (rather then actually altering them) so developer creativity is sandboxed in where it was supposed to be.

This isn't blocking but I don't see why we shouldn't do it prior to release if we can.

Adding #1677026: Less aggressive locale js file rebuilds as a related issue

I think we should go ahead with this approach in 8.x, so I'm moving that issue back to 7.x now.

#1677026: Less aggressive locale js file rebuilds is the issue, trying once more with the related issues field...

I think we could backport some of this logic to 7.x:

- when refreshing the cache, instead of just nuking the entire javascript_parsed variable, check hook_library() for every module, then parse those js files.

This would mean a lot of parsing moves to the process invalidating the cache, making things a lot less stampede-y. We'd need to leave in the rest of the dynamic logic though.

Here's a 7.x proof of concept. This pre-seeded the javascript_parsed with over 80 files on a real 7.x site for me.

The Proof of Concept looks great to me.

Actually going to run this past the bot, since on the site I'm testing with there are files missing from core libraries.

+  foreach (module_list() as $module) {
+    foreach (drupal_get_library($module) as $name => $library) {
+      if (!empty($library['js'])) {

minor update on this, can we remove the unused variable $name in the for loop?
since we are not using this $name variable.

+  foreach (module_list() as $module) {
+    foreach (drupal_get_library($module) as $library) {
+      if (!empty($library['js'])) {

Removed the unused variable.

Added support for scripts[] in themes.

Added support for misc/drupal.js since that's already hard-coded in core and not in a library.

Fatal error: Call to undefined function _locale_parse_js_file() in /var/lib/drupaltestbot/sites/default/files/checkout/includes/common.inc on line 5108
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: Call to undefined function _locale_parse_js_file() in
/var/lib/drupaltestbot/sites/default/files/checkout/includes/common.inc,
line 5108

is what PIFR reports.

Added a require_once for that.

Also handling scripts[] from module .info files. That feature's gone in 8.x but still in 7.x.

Not enough. Despite being in locale.inc, _locale_parse_js_file() depends on locale module.

Don't like it, but added a module_exists(). Would be cleaner to move some of the logic to locale module but that would require at least an API addition so it can implement a hook or something.

Would be RTBC, but 8.0.x needs to go in first - unless we want to split based on code now in classes.

Pinging @Gabor on it anyway.

While the concept is the same, I don't think there's any shareable code here between the versions at all. hook_library() has changed beyond recognition, module .info files can't have scripts, themes js definitions changed too. So the 8.x patch will need to be done from scratch, and it's the kind of change that might only be possible in a minor version too.

Also this patch doesn't change the runtime logic at all - that has to be kept for backwards compatibility in Drupal 7. However in 8.x since everything is in libraries, the runtime logic / hook_js_alter() can be retired completely.

And even further, 8.x doesn't suffer from variable_set() stampedes, because variable_set() has been removed completely. So while there's still a potential performance benefit moving from runtime/on-demand parsing to rebuilds, it's less severe, and more to do with simplification of the overall logic and the patch being in 7.x won't constitute 8.x having a performance regression if that lands first.

So with my 8.x maintainer hat on (work on this patch was for a client), I'd be in favour of different 7.x and 8.x patches and issues for this.

So if the 8.0.x Release Manager agrees, this should be split: Great! I am of the same opinion.

So lets split a new issue out for 8.0.x and get this one in for 7.x? :)

Looks good. Makes sense based on the explanation in #9 :) Needs issue summary update, so the committed patch makes sense vs. the summary. The summary still seems to be about 8.x, while the issue abandoned 8.x.

Only minor note is this line going over:

+++ b/includes/common.inc
@@ -5078,7 +5071,51 @@ function drupal_build_js_cache($files) {
+    // Hard code drupal.js since it is hard-coded in drupal_add_js() and is not a

Thanks!

Updated the issue title and summary.

Opened #2607376: Remove on-demand JavaScript translation parsing and do everything on rebuild.

Fixing that comment over 80 chars.

Also added a file_exists() around _locale_parse_js_file() to avoid errors with modules like jquery_ui that specify files that they don't actually provide in library definitions.

While those are bugs in the modules, suddenly having PHP warnings on cache clears makes it look like we're introducing a bug here.

cruft.

All right. I think it looks good. Ideally this would get some test coverage, if there is a reasonable way to do so.

This has a pretty negative effect on clearing cache performance. Was that intended?

From a drush cc all before and after the patch I'm getting 24ms to 440ms jump.

I've got a lot of modules (commerce site) which may be part of the reason but I thought I'd share before this gets committed since it has Performance tag.

Yes that's by design. The actual cache clear will take longer because all available js files are parsed. However the requests immediately after the cache clear save both the file parsing and the individual variable_set() calls, which in turn saves a lot of variable_init() cache misses on the requests after those.

Ah, we should get a profile on that then, thanks for clarifying @catch.

This isn't so much about profiling though - it's more about avoiding a lock stampede in variable_init().

Let's say variable_initialize() takes 100ms to get all variables from the database, unserialize() them and write to cache. This varies a lot depending on how many variables are in the database but I've seen it take 100ms on sites with over a thousand variables.

Looks a bit like this:
Cache is cleared.
Individual requests find js files to be parsed, parse them, set the variable.
Every request that comes in during the next 100ms either has to rebuild the variable cache or hits a lock_wait() pattern in variable_init()
Potentially, the first request to continue finds another js file that hasn't been parsed yet, and starts it over again. Some of the requests in lock_wait() get stuck on the second lock too.

So the ~100ms it takes to rebuild the variables on a large site needs to be multiplied by the number of requests that hit during that rebuild, and the number of times the rebuild is triggered by subsequent variable_set() calls.

One way to get numbers on this is to log the $name argument to variable_set() on production for a day or so (including after a cache clear).

I've run into this personally, I think I killed/staved off by patching most modules with large variables to stop seeing this stampeeding.

It was painful to debug when this happens, so I'll set back to RTBC.

1. +  $dir = 'public://' . variable_get('locale_js_directory', 'languages');
   +  file_unmanaged_delete_recursive($dir);
   

   I tested the patch and this broke things for me. It deletes all the files, but (unlike _locale_rebuild_js()) does nothing to recreate them.

   To see the problem, you can apply the attached testing patch, add a language (e.g. Spanish), and add a translation for the word "Node". Then clear caches via Drush and visit the homepage in Spanish as an anonymous user. With 7.x everything works fine and you see the Spanish translation at the bottom of the page. With the patch from this issue applied you start seeing English.

   I think it's possible everything will work OK when combined with #1677026: Less aggressive locale js file rebuilds, but if so we have to either postpone this issue on that one or combine the two.

2. +  variable_del('locale_js_files');
   

   Relatively minor, but I can't find this variable used anywhere else in the codebase?

@David_Rothstein thanks for the steps, sorry the test site has two english regions en-US and en-CA so I didn't run into that:S I'll have to dig deeper.