1483038 - Adding a third FileSource to l10n causes preferences to take 20 seconds to open

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[Dave Townsend [:mossop]]

Test with this revision: https://hg.mozilla.org/users/dtownsend_mozilla.com/working/trunk/rev/c718caf63856

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1483038 - Optimize L10nRegistry generator to early exit on missing resources.

Optimize L10nRegistry generator to early exit on missing resources.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

The source of the problem was a microptimization we did when we originally landed L10nRegistry.

We optimized it to only skip the iterator if the *last* resource was known to be missing, which means that if the *first* is known to be missing, L10nRegistry will still happily attempt to constructor a valid context out of all permutations.

I took the liberty to throw in several optimizations that were lingering for a while now:

 - I turned FileSource return values for failure to be `false`, rather than a rejected Promise.
 - I brought back the async down to the core, to allow myself to block on each iteration so that following iteration can already know if any of the fetch attempts failed and not bother
 - I applied a a cached-iterable optimization to prevent iterator duplication in race condition ( https://github.com/projectfluent/cached-iterable/pull/4 )
 - I reduced the prefetch for when there's only one locale (most common case!) to 1
 - I removed prefetching in `onChange` under the assumption that there's no need to prefetch, since we'll trigger new CachedIterable with `translateRoots`.

The result is a drastic improvement in the error scenario that Dave identified without any impact on tests and no impact on performance metrics (which should not be affected because on central we only operate in single-locale, always-match optimistic case so far).

But the first result out of the talos run is already seeing some modest wins and generally all the improvements cut out lots of permutations that are known to end up failing.

I'll fine tune and document this patch tomorrow before requesting r?.

Comment 3

[Axel Hecht [:Pike]]

Trying to understand what we're doing here.

Say we want resources A, B, C, and they're in FileSource α, β, γ, resp.

Say German has only A and C, but not B. We do want to generate a Context(A, C), to resolve all the messages we can.

Say English has A, B, and C. We don't want Context(A), Context(B), Context(C), Context(A,B), Context(A,C), Context(B,C), Context(A,B,C). Instead, we just want Context(A,B,C) right away.

Right?

And say, we have A, B, C in each of α, β, γ. Then we'd want

Context(Aα, Ba, Cα)

Context(Aβ, Ba, Cα)
Context(Aα, Bβ, Cα)
Context(Aα, Ba, Cβ)

Context(Aβ, Bβ, Cα)
Context(Aβ, Ba, Cβ)
Context(Aβ, Bβ, Cβ)

and then γ? Did we properly define the permutation order? Does it matter? 'Cause some conditions might never be reached.

Trying to read the code, it seems that we're doing Context(Aα, Ba, Cα) first, speculatively?
I wonder if we can swap that, and do speculative Resource loading first, i.e., a lazy iterator over FileSource per locale/resource, and then peek into those iterators, and generate as complete as possible bundles?
Sorry if I misread the code, and that's what we're already doing.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Supporting partial contexts is a separate bug. Here I'm adding more logic to exit early from the generator.

The representation of what we do before the patch is:

```
1. generateContexts(["en-US"], ["A", "B", "C"]);
2.   generateResourceSetsForLocale("en-US", sourcesOrder, resIds, []);
3.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α"]);
4.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "α"]);
5.         generateResourceSet("en-US", ["α", "α", "α"], resourceids);
6.         generateResourceSet("en-US", ["α", "α", "β"], resourceids);
7.         generateResourceSet("en-US", ["α", "α", "γ"], resourceids);
8.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "β"]);
9.         generateResourceSet("en-US", ["α", "β", "γ"], resourceids);
10. ...
```

Notice that in step 5. we `Promise.all` fetch all three resources, so by the time we get to 6. we *know* (in the conservative scenario) that B is not in α.

Yet, due to the code in [0], we only will skip 3. and 4. the next time we get on that level (in step 9.), because then we know that the *last* item from the order is marked as not available.

My change turns it into:

```
1. generateContexts(["en-US"], ["A", "B", "C"]);
2.   generateResourceSetsForLocale("en-US", sourcesOrder, resIds, []);
3.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α"]);
4.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "α"]);
5.         generateResourceSet("en-US", ["α", "α", "α"], resourceids);
6.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "β"]);
7.         generateResourceSet("en-US", ["α", "β", "β"], resourceids);
9.         generateResourceSet("en-US", ["α", "β", "γ"], resourceids);
10. ...
```

Here, we exited early twice:
 - First, after step 5, we know that B is not in α, so we can skip ["α", "α", *], and jump right into testing ["α", "β"].
 - Then, we also know that in step 7. we don't need to test ["α", "β", "α"] because C is not in α.



A more conservative scenario which is what Mossop reported is when resources A is in γ, B is in β and C is in α.

Without the patch:

```
1. generateContexts(["en-US"], ["A", "B", "C"]);
2.   generateResourceSetsForLocale("en-US", sourcesOrder, resIds, []);
3.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α"]);
4.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "α"]);
5.         generateResourceSet("en-US", ["α", "α", "α"], resourceids);
6.         generateResourceSet("en-US", ["α", "α", "β"], resourceids);
7.         generateResourceSet("en-US", ["α", "α", "γ"], resourceids);
8.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "β"]);
9.         generateResourceSet("en-US", ["α", "β", "α"], resourceids);
11.      generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "γ"]);
12.        generateResourceSet("en-US", ["α", "γ", "α"], resourceids);
15.    generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["β"]);
16.      generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["β", "β"]);
17.        generateResourceSet("en-US", ["β", "β", "α"], resourceids);
18.      generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["γ", "β"]);
19.        generateResourceSet("en-US", ["γ", "β", "α"], resourceids);
```

With the patch:

```
1. generateContexts(["en-US"], ["A", "B", "C"]);
2.   generateResourceSetsForLocale("en-US", sourcesOrder, resIds, []);
3.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α"]);
4.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["α", "α"]);
5.         generateResourceSet("en-US", ["α", "α", "α"], resourceids);
6.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["β"]);
7.       generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["β", "β"]);
8.         generateResourceSet("en-US", ["β", "β", "α"], resourceids);
9.     generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["γ"]);
10.      generateResourceSetsForLocale("en-US", sourcesOrder, resIds, ["γ", "β"]);
11.        generateResourceSet("en-US", ["γ", "β", "α"], resourceids);
```

This of course scales with 13 resouces instead of 3 :)

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

[0] https://searchfox.org/mozilla-central/source/intl/l10n/L10nRegistry.jsm#207-209

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 9000160 [details]
Bug 1483038 - Optimize L10nRegistry generator to early exit on missing resources.

Dave Townsend [:mossop] has approved the revision.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cd626758796
Optimize L10nRegistry generator to early exit on missing resources. r=mossop

Comment 8

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 6cd626758796 (Bug 1483038) for ES lint failure in L10nRegistry

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=33c7b0ea5caaa654c72cce43124db1ebc052dead&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=197405969

https://treeherder.mozilla.org/logviewer.html#?job_id=197405969&repo=autoland&lineNumber=262

[vcs 2018-09-04T16:36:32.542Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "clone", "shouldAlert": false, "subtests": [], "value": 129.52659106254578}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "pull", "shouldAlert": false, "subtests": [], "value": 8.629196882247925}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 87.36154699325562}, {"extraOptions": ["m3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 226.73440408706665}]}
[vcs 2018-09-04T16:36:33.028Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/6cd626758796faee3caa64cbccd86e2695f3d810 title='Built from autoland revision 6cd626758796faee3caa64cbccd86e2695f3d810'>6cd626758796faee3caa64cbccd86e2695f3d810</a>
[task 2018-09-04T16:36:33.028Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2018-09-04T16:36:33.031Z] + cd /builds/worker/checkouts/gecko/
[task 2018-09-04T16:36:33.031Z] + cp -r /build/node_modules_eslint node_modules
[task 2018-09-04T16:36:34.502Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2018-09-04T16:36:34.503Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2018-09-04T16:36:34.504Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2018-09-04T16:36:35.273Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-09-04T16:36:35.273Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-09-04T16:36:37.001Z] Installing setuptools, pip, wheel...done.
[task 2018-09-04T16:36:38.186Z] running build_ext
[task 2018-09-04T16:36:38.186Z] building 'psutil._psutil_linux' extension
[task 2018-09-04T16:36:38.186Z] creating build
[task 2018-09-04T16:36:38.186Z] creating build/temp.linux-x86_64-2.7
[task 2018-09-04T16:36:38.186Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-09-04T16:36:38.186Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-09-04T16:36:38.186Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-09-04T16:36:38.186Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-09-04T16:36:38.186Z] creating build/lib.linux-x86_64-2.7
[task 2018-09-04T16:36:38.186Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-09-04T16:36:38.187Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-09-04T16:36:38.187Z] building 'psutil._psutil_posix' extension
[task 2018-09-04T16:36:38.187Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-09-04T16:36:38.187Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-09-04T16:36:38.187Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-09-04T16:36:38.187Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-09-04T16:36:38.187Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-09-04T16:36:38.187Z] 
[task 2018-09-04T16:36:38.187Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-09-04T16:42:27.327Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/intl/l10n/L10nRegistry.jsm:91:10 | Expected to return a value at the end of async generator method 'generateContexts'. (consistent-return)
[taskcluster 2018-09-04 16:42:27.664Z] === Task Finished ===
[taskcluster 2018-09-04 16:42:27.665Z] Unsuccessful task run with exit code: 1 completed in 614.219 seconds

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31d611a28db2
Backed out changeset 6cd626758796 for ES lint failure in L10nRegistry

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d4fd146e0b1
Optimize L10nRegistry generator to early exit on missing resources. r=mossop

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0d4fd146e0b1