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.

Files: 
CommentFileSizeAuthor
#14 locale_rebuild_js.patch6.32 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 37,241 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#12 locale_rebuild_js.patch6.32 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.module.
[ View ]
#9 locale_rebuild_js.patch6.17 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 37,239 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#7 locale_rebuild_js.patch6.18 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 37,238 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#5 locale_rebuild_js.patch6.27 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 37,200 pass(es), 22 fail(s), and 42 exception(s).
[ View ]
#3 locale_rebuild_js.patch6.23 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 37,156 pass(es), 22 fail(s), and 322 exception(s).
[ View ]
#1 locale_rebuild_js.patch6.23 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.module.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new6.23 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.module.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.23 KB
FAILED: [[SimpleTest]]: [MySQL] 37,156 pass(es), 22 fail(s), and 322 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.27 KB
FAILED: [[SimpleTest]]: [MySQL] 37,200 pass(es), 22 fail(s), and 42 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.18 KB
FAILED: [[SimpleTest]]: [MySQL] 37,238 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.17 KB
FAILED: [[SimpleTest]]: [MySQL] 37,239 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

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.

Issue tags:+JavaScript

tag

Status:Needs work» Needs review
StatusFileSize
new6.32 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.module.
[ View ]

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: Consider using imagecache style generation of CSS and JS aggregates - 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.

Status:Needs work» Needs review
StatusFileSize
new6.32 KB
FAILED: [[SimpleTest]]: [MySQL] 37,241 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

Argh.

Status:Needs review» Needs work

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

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.

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.

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

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.

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