If locale finds a new string, it immediately clears the locale cache so the next request will rebuild it and cache the fact that the string has been found (with or without a translation).

If you have several strings being found on a page, this can lead to the locale cache being cleared over and over. This is reasonably expensive with memcache at the moment since it does several operations during a wildcard clear, DELETE FROM {cache} WHERE cid LIKE 'foo%'; isn't free either.

Instead of doing it every time, we can just record the fact that a string has been found (in $conf or drupal_static() or somewhere), then add a hook_exit() to locale module to clear the cache once for the end of the request.

No patch yet, I came across this issue with the i18nstrings module, but it looks like the code in there was adapted from core. #1080954: Clear textgroup caches once per request is that issue (with a D6 patch).

CommentFileSizeAuthor
#66 locale-lookup-1080964-63.patch6.51 KBDamienMcKenna
#63 locale-lookup-1080964-63.patch6.51 KBBerdir
#63 locale-lookup-1080964-63-interdiff.txt1.25 KBBerdir
#60 drupal-1080964-60.patch6.4 KBtim.plunkett
#58 drupal-1080964-locale-58-d7-do-not-test.patch6.4 KBdisasm
#58 interdiff.txt4.42 KBdisasm
#56 drupal-1080964-locale-d7-do-not-test.patch6.08 KBnerdstein
#49 locale_D6_do-not-test.patch5.69 KBcatch
#46 locale_lookup.patch7.4 KBcatch
#43 locale_cache.patch7.4 KBcatch
#41 1080964-41-locale-lookup.patch8.17 KBAnonymous (not verified)
#37 1080964-37-locale-lookup.patch8.74 KBAnonymous (not verified)
#35 1080964-35-locale-cache.patch8.18 KBAnonymous (not verified)
#33 1080964-33-locale-cache.patch8.51 KBAnonymous (not verified)
#32 locale-before.png123.54 KBcatch
#32 locale-after.png68.59 KBcatch
#32 locale-after-resolveCacheMiss.png109.35 KBcatch
#31 locale.patch7.17 KBcatch
#29 locale.patch7.16 KBcatch
#26 Contexts.png120.48 KBGábor Hojtsy
#25 locale.patch7.08 KBcatch
#22 Desk 1_046.png87.03 KBcatch
#21 locale_cache-D6.patch3.48 KBcatch
#20 Desk 1_047.png99.36 KBcatch
#20 Desk 1_048.png94.29 KBcatch
#19 locale_cache-D6.patch3.47 KBcatch
#17 locale_1080964.patch5.37 KBcatch
#13 locale_1080964.patch4.56 KBdrewish
#9 locale_1080964.patch4.23 KBSteven Jones
#5 locale_1080964.patch4.23 KBSteven Jones
#4 locale_1080964.patch4.25 KBSteven Jones
#2 locale_1080964.patch4.22 KBcatch
#1 locale_1080964.patch4.22 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs review
FileSize
4.22 KB

Here's a patch.

Instead of clearing the cache every time we find a string, it collects a list of a newly found cacheable strings in the locale static. Then in locale_exit(), if any new strings are found, adds these to the existing cached strings that are already there.

I'm not set up to profile this , but it should mean requests look like:

db_query()
db_query()
db_query()
db_query()
[... potentially dozens of these ...]
cache_get()
cache_set()

instead of

db_query()
cache_clear_all()
db_query()
cache_clear_all()
db_query()
cache_clear_all()
db_query()
cache_clear_all()
[... potentially dozens of these ...]
-- next request --
lock_acquire()
Huge db_query()
cache_set()
[...]

Didn't run tests locally so see what the bot says.

catch’s picture

FileSize
4.22 KB

Fix that notice.

Status: Needs review » Needs work

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

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
+++ modules/locale/locale.module
@@ -694,6 +694,31 @@ function locale($string = NULL, $context = NULL, $langcode = NULL) {
+      foreach ($strings['#write_cache'] as $context) {
+        foreach ($context as $string) {
+          $data[$context][$string] = $locale_t[$langcode][$context][$string];
+        }
+      }

Looks to me like that foreach is just iterating over the arrays incorrectly. Patch attached that should fix that.

I've not reviewed if this patch actually works mind you, are their tests for that already?

Powered by Dreditor.

Steven Jones’s picture

FileSize
4.23 KB

Minor change to use a $cid variable we went to the trouble of creating.

Status: Needs review » Needs work
Issue tags: -Performance

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

Steven Jones’s picture

Status: Needs work » Needs review
Issue tags: +Performance

#5: locale_1080964.patch queued for re-testing.

Steven Jones’s picture

Looks like pirf broke testing that patch.

Steven Jones’s picture

FileSize
4.23 KB

Let's try this again shall we testbot?

Steven Jones’s picture

If you follow the link through then the tests for #9 are actually passing. Testbot is a little fragile it would seem.

drewish’s picture

This comment doesn't make much sense:

+        // This is the first use of this string under current Drupal version. The
+        // saved version also a string-history information for later pruning of the tables.
catch’s picture

That's true, but it's the same as the current comment minus what felt like a redundant sentence, I wasn't sure what it said so didn't try to rewrite it completely, but very open to suggestions :)

drewish’s picture

FileSize
4.56 KB

I cleaned up some of the comments.

Does it really make sense to write this out at the end of every session? Why not just use register_shutdown_function() to set the callback when we notice it's changed?

drewish’s picture

Status: Needs review » Needs work

I'm also now concerned that the write out needs to use the same lock that we use when doing the initial cache loading to avoid race conditions.

DanPir’s picture

subscribe

DanPir’s picture

subscribe

catch’s picture

Status: Needs work » Needs review
FileSize
5.37 KB

I like using register_shutdown_function for this, there are other places this pattern is used in core that could use the same thing.

Also added the lock - if we fail to acquire the lock, we just don't write to the cache - the next request can do that.

Haven't run tests.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D6, +Needs backport to D7

Tags.

catch’s picture

FileSize
3.47 KB

The 8.x patch should apply to 7.x, here's one for D6 as well. I should hopefully be able to test the D6 patch on a client code base in the next day or so.

catch’s picture

FileSize
94.29 KB
99.36 KB

Here's before and after screenshots from Drupal 6. You can see 246 calls to cache_clear_all() (this was done on the french version of the front page of a client site, after running TRUNCATE locales_source), compared to zero with the patch.

The way I'm profiling doesn't pick up the _locale_write_cache() but this is just a cache_get(), foreach and cache_set() so shouldn't be much.

catch’s picture

FileSize
3.48 KB

Fixes a bug in the D6 patch.

catch’s picture

FileSize
87.03 KB

umm, both screenshots in #20 were 'before', here's the actual after.

catch’s picture

#17: locale_1080964.patch queued for re-testing.

catch’s picture

Issue tags: +DrupalCacheArray

Adding DrupalCacheArray tag, this is worth looking at for converting to that.

catch’s picture

FileSize
7.08 KB

Here's a DrupalCacheArray version.

Current situation in 8.x.

- each time you upgrade core to a minor release, each translated string on the site that is found will trigger a clear of the locale cache in that language, if you have 15,000 rows in {locales_source} (I'm looking at a 6.x sites with 18,000), that's 15,000 cache clears assuming they're all found at some point.

- each time the cache is cleared, the next request will get a cache miss and acquire a lock, then front load the cache with all previously found strings under a certain length. All other requests in that language until the cache item is set / lock released will need to individually query each string, however they're all capable of clearing the cache again if they find new strings.

- strings are cached based on current usage, and also whether the string is shorter than 75 chars to save memory.

With the patch:
- finding strings in new versions will not trigger a cache clear, it'll just trigger a locale cache write at the end of the request
- there's a write lock to prevent the criss/cross issue of different paths overwriting the cache with different sets of strings built into CacheArray. However the locking operates differently from current 8.x - with CacheArray the first request to try to cache some strings wins, rather than the first request to get a cache miss.
- all strings get cached regardless of length. This would mean a larger cache item when lots of strings have been found. However, the patch also segments the cache by system path root (i.e. 'node', 'admin', 'user'), so for example strings that are only ever found on admin pages won't get into the cache for other path roots and vice versa.

One drawback at the moment is that CacheArray doesn't handle multi-dimensional arrays at all, so I'm currently creating a new one per-context - if there were lots of contexts on a page, then you could end up with lots of cache items. Currently we're barely using msgctxt in core so it's in practice not going to make a lot of difference, but it could if it starts to be used more widely. If that's a problem we'd likely need to go for a custom class that has a language in the constructor, then something like function get($context, $string); instead of CacheArray.

beejeebus suggested caching only the keys in irc (i.e. the lookup string, not the translation), then we could the translations in __construct() or somewhere with an IN query each request, with the idea we'd not clear the cache when saving new translations at all.

If we want the IN() query to only look for strings that we know have translations though, then we still need to store whether there's a translation or not, which means that saving translated strings will still need to clear the cache or force an update, so I didn't implement this for now but that's probably worth discussing more.

Patch passed a couple of locale tests locally but I didn't run the whole suite or anything.

Gábor Hojtsy’s picture

FileSize
120.48 KB

You can get a list of contexts used by all of contrib on localize.drupal.org, just go to a translation team's Translate tab and hit the context dropdown. There are not a lot of contexts (and not very consistent):

Contexts.png

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review

OK given that list I think it's OK to have a cache entry per context so we can use CacheArray as is as opposed to adding extra custom logic. I'll look at fixing up the test failures.

catch’s picture

FileSize
7.16 KB

This should fix at least some of those test failures.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

One more then.

catch’s picture

Here's xhprof output comparing current HEAD vs. the patch. To get this I did the following:

- added a new language
- visited 8.localhost/fr
- cleared the cache
- upped the version in bootstrap.inc

The main change is that 69 calls to cache()-deletePrefix() are now gone from that page.

The other change would be that the next request to that page would have to rebuild the cache from scratch, whereas with the patch the cache has already been primed (since we remove the need to clear the cache each time a new string is found). Once the version stuff is settled down it's the usual trade-off: HEAD will rebuild the whole cache the first request that gets a miss, the patch does the usual CacheArray pattern of adding stuff as it finds it.

There's two after screenshots, that's because a lot of the code in resolveCacheMiss() used to be in locale() itself, so I've taken the function detail from there too for easier comparison.

Anonymous’s picture

here's another approach, based on talking to catch in IRC.

Status: Needs review » Needs work

The last submitted patch, 1080964-33-locale-cache.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.18 KB

fixed dumb dup locale function.

changed LocalCache to LocaleLookup.

implemented catch's suggestion to keep less data in memory in LocaleLookup.

Status: Needs review » Needs work

The last submitted patch, 1080964-35-locale-cache.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.74 KB

fixed typos in LocaleLookup.

Status: Needs review » Needs work
Issue tags: -Performance, -Needs backport to D6, -Needs backport to D7, -DrupalCacheArray

The last submitted patch, 1080964-37-locale-lookup.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

#37: 1080964-37-locale-lookup.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +Needs backport to D6, +Needs backport to D7, +DrupalCacheArray

The last submitted patch, 1080964-37-locale-lookup.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.17 KB

trying that again.

Status: Needs review » Needs work

The last submitted patch, 1080964-41-locale-lookup.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
7.4 KB

I like the lack of locking in beejeebus' patch and the fact it's not necessary to clear caches when adding/updating a translation, but I'm concerned about the trade-off from the extra cache sets (one for each string) as well as the cache()->get() plus cache()->getMultiple() compared to a single cache()->get(). Would be good to get some more opinions on this, but I'm starting to also think we should start a new issue to look at 1. introducing an interface which can cleanly handle both approaches 2. deciding on the preferred approach.

Here's an update based on #31 that switches from per path-root caching to per-role caching - we realised when discussing this in irc that per-path-root caching doesn't help if for example you have admin_menu installed showing the full admin tree on every page, whereas per-role should mean not too many potential combinations as well as anonymous and low level authenticated users getting a much smaller cache item than they would otherwise.

aspilicious’s picture

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -0,0 +1,88 @@
+    $rids = implode(':', array_keys($GLOBALS['user']->roles)); ¶

Trailing whitespace

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -0,0 +1,88 @@
+    if (variable_get('locale_cache_strings', 1)) { ¶

Same

15 days to next Drupal core point release.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
7.4 KB

Thanks! Fixed the whitespace issues, I had vim set up to show those, but apparently no longer.

Also updated the use statement and some stale docs with the class rename.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

#46 looks good to go to me, and those benchmarks show it

i agree that my approach is more experimental, so it's probably worth moving forward with this and creating follow ups for that.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint

Putting on D8MI sprint for tracking.

catch’s picture

FileSize
5.69 KB

Here's a D6 backport. It can't be committed due to PHP 5.2 requirements, but I'm (very) slowly trying to get these into Pressflow.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/modules/locale/locale.moduleundefined
@@ -343,59 +343,68 @@ function locale($string = NULL, $langcode = NULL, $reset = FALSE) {
+  } ¶

Check your environment again, it doesn't show you trailing whitespaces anymore ;)

13 days to next Drupal core point release.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Didn't ment to unset the rtbc of #46.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I confess to not totally understanding everything in this patch, however it has sign-off from the right people and doesn't seem to do anything too contentious.

Committed and pushed to 8.x, with a small comment tweak per catch:

change

  // Strings are cached by langcode, context and system path root, using instances of the
 // LocaleCache class to handle string lookup and caching.

to:

 // Strings are cached by langcode, context and roles, using instances of the
 // LocaleLookup ...

Marking 7.x and patch (to be ported) as a result, and also since David will probably want a closer look at this for 7.x.

Gábor Hojtsy’s picture

Should we have a changelog entry for this?

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing off Drupal 8 multilingual sprint issues. Work applies to Drupal 7 now.

nerdstein’s picture

Assigned: Unassigned » nerdstein

I am adding in the backport to D7

nerdstein’s picture

The patch did not apply cleanly. I manually went through the patch and made edits to locale.module. When running the D7 tests, it fails. I wanted to upload it for feedback.

aspilicious’s picture

Status: Patch (to be ported) » Needs review
disasm’s picture

Status: Needs review » Patch (to be ported)
FileSize
4.42 KB
6.4 KB

attached is a patch fixing the spacing in nerdstein's patch as well as fixing some issues between d7 and d6.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review

.

tim.plunkett’s picture

FileSize
6.4 KB

Just renamed, no credit.

Status: Needs review » Needs work

The last submitted patch, drupal-1080964-60.patch, failed testing.

mgifford’s picture

Just missing this one test from the looks of it:
Default context is working. Other locale.test 1046 LocaleImportFunctionalTest->testLanguageContext()

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
6.51 KB

This should work better, the patch did not correctly consider the context stuff. That should have resulted in more than just one fail :)

mgifford’s picture

63: locale-lookup-1080964-63.patch queued for re-testing.

mgifford’s picture

Assigned: nerdstein » Unassigned
Issue summary: View changes

Patch from @Berdir still applies nicely.

DamienMcKenna’s picture

  • webchick committed 7f6f467 on 8.3.x
    Issue #1080964 by catch, beejeebus, Steven Jones, drewish: Be more...

  • webchick committed 7f6f467 on 8.3.x
    Issue #1080964 by catch, beejeebus, Steven Jones, drewish: Be more...

  • webchick committed 7f6f467 on 8.4.x
    Issue #1080964 by catch, beejeebus, Steven Jones, drewish: Be more...

  • webchick committed 7f6f467 on 8.4.x
    Issue #1080964 by catch, beejeebus, Steven Jones, drewish: Be more...
Silicon.Valet’s picture

reviving the zombie thread, would be interested in seeing this merged. Hi @DamienMcKenna from the past!

Silicon.Valet’s picture

We've been running this code in production without any issues now for 5 months. Patch also looks good, but I'm lost on some of the subtleties.

joseph.olstad’s picture

A diamond in the rough here, reminder, lets make a seperate issue for D7 so that we do not have to ping the D8 people every time we update this ticket. Put a link to the new D7 issue here, and then set this issue back to 8.x and mark this issue as 'Fixed'

  • webchick committed 7f6f467 on 9.1.x
    Issue #1080964 by catch, beejeebus, Steven Jones, drewish: Be more...
alexdmccabe’s picture

FWIW: We're still using the patch from #66 in production.