I've been tracking down a large number of calls to variable_set('javascript_parsed') on a client site, consistently several dozen per day on one of their web servers. The code on that site is usually only updated every week or so, so this feels a bit high to me.

Don't have much data yet about why the variable is being set so often, however I noticed this reading through the code:

  // If there are any new source files we parsed, invalidate existing
  // JavaScript translation files for all languages, adding the refresh
  // flags into the existing array.
  if ($new_files) {
    $parsed += locale_inc_callback('_locale_invalidate_js');
  }

And drupal_clear_js_cache() does this:

/**
 * Delete all cached JS files.
 */
function drupal_clear_js_cache() {
  file_scan_directory(file_create_path('js'), '.*', array('.', '..', 'CVS'), 'file_delete', TRUE);
  watchdog('javascript_parsed', 'Javascript parsed for cleared js cache');
  variable_set('javascript_parsed', array());
}

So this means:

- a cache clear sets the variable to an empty array.
- each time a file is found it's parsed for translations
- each time a file is found, a refresh flag gets set so that all previously parsed files are parsed again.

The latter seems unnecessary to me - if you're adding new files, then you're deploying code and likely clearing caches anyway. When the entire variable is reset, we don't need to keep refreshing it just when new files are pulled up.

No patch yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs review
FileSize
6.23 KB

OK a few more notes:

_locale_rebuild_js() doesn't do anything with source files, it instead creates a new translations file for the current language. So, there's absolutely no need to refresh that when a new source file is found, instead we'd want to refresh it when a translation is saved for a string that exists in a source file or if the cache is cleared. As far as I can see there's no need to do anything at all when a new file is found, except to scan it for source strings.

Attaching an untested patch. This removes the 'invalidate/refresh' logic altogether, and just rebuilds the translation files on demand. Clearing the js cache will delete the files rather than setting flags so that they get regenerated again.

CNR for the bot only.

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.23 KB

Let's at least run php -l on it...

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.27 KB

Looks like this is failing at least partly when languages are added, there's no need to rebuild this when a language is added either since it won't have any translations yet.

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.18 KB

This should pass tests - at least the two locale ones that were failing. We're now rebuilding the translation js on demand if it's missing, when a language is updated, when a translation is added, when the js cache is cleared and a couple of other places. All the logic related to refreshing is gone - either the files are deleted and recreated or we just rebuild them directly.

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.17 KB

Can't reproduce the failure locally, trying a patch (with a couple of other minor fixes).

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

nod_’s picture

Issue tags: +JavaScript

tag

catch’s picture

Status: Needs work » Needs review
FileSize
6.32 KB

This doesn't fix the notices (still unable to reproduce those), but it now handles the case where the translation file doesn't exist for some reason since that doesn't appear to be handled by the existing code. The only situation I can think of that the site could get into this state is 1. testing this patch 2. changing the location of the locale_js_directory variable.

Having made that change this makes me realise the entire rebuild logic is a possible candidate for #1014086: Stampedes and cold cache performance issues with css/js aggregation - generating one translation takes around 350ms on my localhost and it's very likely this could be refactored to do that in a separate request.

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
6.32 KB

Argh.

Status: Needs review » Needs work

The last submitted patch, locale_rebuild_js.patch, failed testing.

catch’s picture

Issue tags: +Performance

Still can't reproduce the failures locally, but I've tested a D6 backport of this on a production site now and it's cut out nearly all the translation rebuilds/variable sets - on one webserver they were reduced from over 6,000/week to nothing.

Gábor Hojtsy’s picture

I agree with the goal that the JS files should be rebuilt in a more targeted fashion! Thanks for looking into this. I did not look in detail at the code yet, just picked this sentence up and wanted to provide some background:

As far as I can see there's no need to do anything at all when a new file is found, except to scan it for source strings.

I think the reason we set to rebuild the JS files when a new file is found is that the newly parsed file might have strings that might have translations already in the database, and therefore the JS translations should be fully refreshed. Not sure if we are talking about the same thing or you have a better solution in the patch, just noting.

Gábor Hojtsy’s picture

A different kind of JS file rebuilding optimisation was added earlier which is based on the source strings touched in the file imports. THat is broken on sqlite currently if lots of strings were affected: #1884182: Import of large number of translations fails on SQLite

Jose Reyero’s picture

That optimization is not completely lost, since we are still using the list of string ids for other operations like manually editing translations, so I guess it may still fix the original issue.
Anyway, that may be an issue if we ever get configuration translated (rebuilding the full configuration is not as easy as rebuilding the js) so we should still look for a better strategy here.

The first idea that comes to my mind if we want to do some well targetted cache refreshing, but still keeping the thing scalable, is to add a timestamp to each translation so can have this kind of logic later:
If any js string was updated after 'js cache last rebuilt timestamp' ...then.. refresh js strings.

rares’s picture

Patch, any chance you could post or send me the D6 backport of the patch you posted in #16?

kenorb’s picture

catch’s picture

Version: 8.0.x-dev » 7.x-dev

#2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls is a much more comprehensive approach to this problem, but can only be done in 8.x.

The code here has diverged significantly in 8.x and I don't think it's worth trying to get it done there given we can just nuke it with #2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls, so moving this issue back to 7.x.

catch’s picture

Status: Needs work » Needs review
FileSize
5.47 KB

Drupal 7 version of the patch, passed the locale js tests locally but didn't run any more than that.

Status: Needs review » Needs work

The last submitted patch, 23: 1677026-23.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.92 KB

MIssed one call to _locale_invalidate_js().

catch’s picture

FileSize
6.89 KB

Avoiding building the translation file each time if there's no translations for a language

Fabianx’s picture

+++ b/includes/common.inc
@@ -897,13 +897,6 @@ function drupal_http_request($url, array $options = array()) {
-    variable_set('drupal_http_request_fails', TRUE);
-

This seems unrelated?

--

Overall looks great to me.

catch’s picture

FileSize
6.89 KB

Whoops yes that's only related in the sense of variable_set() stampedes.

catch’s picture

FileSize
6.89 KB
catch’s picture

FileSize
6.31 KB
catch’s picture

FileSize
6.31 KB
@@ -937,7 +938,7 @@ function locale_js_alter(&$javascript) {
     if ($files && !empty($language->javascript)) {
       // Add the translation JavaScript file to the page.
       $file = $dir . '/' . $language->language . '_' . $language->javascript . '.js';
-      if (!file_exists($files)) {
+      if (!file_exists($file)) {
         _locale_rebuild_js($language->language);
       }
       $javascript[$file] = drupal_js_defaults($file);

And fixing that typo.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs subsystem maintainer review

RTBC, but lets ping Gabor on this.

Gábor Hojtsy’s picture

Looks fine with me. Good find!

Gábor Hojtsy’s picture

Looks fine with me. Good find!

andypost’s picture

+++ b/includes/locale.inc
@@ -1961,40 +1961,6 @@ function _locale_translate_seek_query() {
-function _locale_invalidate_js($langcode = NULL) {

removal of this could affect contrib modules, at least that requires change record

Fabianx’s picture

#35: A CR is likely fine, but _ functions are always @internal. Else they would not be prefixed with an underscore.

catch’s picture

FileSize
5.75 KB

#2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls is also RTBC but the patches conflict.

Uploading a version of this patch that will still apply if the other issue gets in first. Helps if using composer to apply both patches.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
  1.    // Force JavaScript translation file recreation for all languages.
    -  _locale_invalidate_js();
    +  _locale_rebuild_js();
    

    This (and one or two other similar places in the patch) look wrong to me. _locale_invalidate_js() invalidates all languages on the site, but _locale_rebuild_js() only invalidates the current language. So at a minimum I think the code comment is wrong, but it also seems likely it's not working correctly either. Has anyone tested this patch in a scenario with more than one non-English language?

  2.  /**
    - * Force the JavaScript translation file(s) to be refreshed.
    - *
    - * This function sets a refresh flag for a specified language, or all
    - * languages except English, if none specified. JavaScript translation
    - * files are rebuilt (with locale_update_js_files()) the next time a
    - * request is served in that language.
    .....
    - */
    -function _locale_invalidate_js($langcode = NULL) {
    

    I don't think deleting this function is a great idea - not only might it be useful based on my above point, but for other reasons as well. Couldn't the function still exist, but just have the effect of saving $language->javascript as an empty value?

  3. +  if (!$data && $changed_hash) {
    +    $language->javascript = FALSE;
    +    $status = 'deleted';
    +  }
    
    .....
    
    +    // If there has not been an attempt to generate a JavaScript file, try now.
    +    if ($language->javascript === '') {
    +      _locale_rebuild_js($language->language);
    +    }
    

    I haven't fully followed the code flow, but is it actually intentional that FALSE and an empty string are distinguished here? If so, it would probably at least be worth a code comment explaining the difference.

  4. +  variable_del('locale_js_files');
    

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

David_Rothstein’s picture

The code here has diverged significantly in 8.x and I don't think it's worth trying to get it done there given we can just nuke it with #2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls, so moving this issue back to 7.x.

It looks like that Drupal 8 issue moved to #2607376: Remove on-demand JavaScript translation parsing and do everything on rebuild but there hasn't been progress on it.

The two systems actually look quite similar to me in Drupal 7 and Drupal 8 (only difference is that Drupal 8 moved some of the code out to a helper function):
https://api.drupal.org/api/drupal/modules!locale!locale.module/function/...
https://api.drupal.org/api/drupal/core!modules!locale!locale.module/func...

So I don't know why we wouldn't want to get this issue fixed in Drupal 8 first and backport it afterwards.

catch’s picture

Thanks for the review David.

#38-1:
That's a good point. The patch has been running in production on a site with several non-English languages, but it's very likely that this site has enough invalidations from elsewhere in the code base that any missing invalidation here is covered.

We could potentially leave _locale_invalidate_js() in, and have it run the same logic as it currently does of looping over all languages, but just calling out to _locale_rebuild_js()?

#38-3: pretty sure that line is just moved around by the patch and not introduced.

#39: The difference between the versions is that in 8.x we'd 100% remove on-demand translation parsing, so this code would not run at all. This is possible because in 8.x every file is tracked in a library, whereas in 7.x there's no such option. So we could postpone this on the 8.x issue, but there would be no usable code to backport to this issue from there - it just becomes irrelevant in 8.x.