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.
Comment | File | Size | Author |
---|---|---|---|
#37 | 1677026-do-not-test.patch | 5.75 KB | catch |
#31 | 1677026_0.patch | 6.31 KB | catch |
Comments
Comment #1
catchOK 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.
Comment #3
catchLet's at least run php -l on it...
Comment #5
catchLooks 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.
Comment #7
catchThis 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.
Comment #9
catchCan't reproduce the failure locally, trying a patch (with a couple of other minor fixes).
Comment #11
nod_tag
Comment #12
catchThis 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.
Comment #14
catchArgh.
Comment #16
catchStill 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.
Comment #17
Gábor HojtsyI 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:
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.
Comment #18
Gábor HojtsyA 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
Comment #19
Jose Reyero CreditAttribution: Jose Reyero commentedThat 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.
Comment #20
rares CreditAttribution: rares commentedPatch, any chance you could post or send me the D6 backport of the patch you posted in #16?
Comment #21
kenorb CreditAttribution: kenorb commentedComment #22
catch#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.
Comment #23
catchDrupal 7 version of the patch, passed the locale js tests locally but didn't run any more than that.
Comment #25
catchMIssed one call to _locale_invalidate_js().
Comment #26
catchAvoiding building the translation file each time if there's no translations for a language
Comment #27
Fabianx CreditAttribution: Fabianx commentedThis seems unrelated?
--
Overall looks great to me.
Comment #28
catchWhoops yes that's only related in the sense of variable_set() stampedes.
Comment #29
catchComment #30
catchComment #31
catchAnd fixing that typo.
Comment #32
Fabianx CreditAttribution: Fabianx commentedRTBC, but lets ping Gabor on this.
Comment #33
Gábor HojtsyLooks fine with me. Good find!
Comment #34
Gábor HojtsyLooks fine with me. Good find!
Comment #35
andypostremoval of this could affect contrib modules, at least that requires change record
Comment #36
Fabianx CreditAttribution: Fabianx commented#35: A CR is likely fine, but _ functions are always @internal. Else they would not be prefixed with an underscore.
Comment #37
catch#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.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis (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?
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?
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.
Relatively minor, but I can't find this variable used anywhere else in the codebase?
Comment #39
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt 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.
Comment #40
catchThanks 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.