1660392 - Migrate L10nRegistry to Rust

Status: RESOLVED FIXED

(Core :: Internationalization, task, P3)

Description

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

As part of bug 1660391, we want to move L10nRegistry from JS to Rust

Comment 1

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

We may need bug 1231711, but I'm not yet sure how and if.

Comment 2

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

We're going to introduce 3 classes:

• L10nFileFetcher - which in JS will be a simple class with two methods fetch and fetchSync
• FileSource which will be an equivalent of Rust FileSource and may accept a custom L10nFileFetcher or use the default one
• L10nRegistry which will be a singleton per process

Comment 4

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

Attachment: Bug 1660392 - [l10nregistry] part2: Add L10nRegistry WebIDL backed by l10nregistry-rs.

Comment 5

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

Attachment: Bug 1660392 - Vendor in l10nregistry-rs and fluent_fallback.

Comment 6

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

Attachment: reg-io-heavy-test.js

Test analogous to bug 1613705 I/O heavy for L10nRegistry.

Comment 7

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

Attachment: reg-io-heavy-test.js

this is a more accurate test, with all files not primed by the main UI.

Comment 8

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

Here are initial perf results from this test (just sync for now). (the Rust from Rust is an instrumentation in Rust using std::time::Instant):

When calling generateBundlesSync and then next to fetch the first bundle:

Variant	Cold (ms)	Warm (ms)
JSM (from JS)	10.1	2.5
Rust (from JS)	5.1	1.3
Rust (from Rust)	5.0	1.27

When calling only generateBundlesSync (can't test cold for JS because it's already instantiated):

Variant	Cold (ms)	Warm (ms)
JSM (from JS)		0.014
Rust (from JS)	0.092	0.054
Rust (from Rust)	0.0046	0.0031

It seems that overall we're getting a nice perf boost of ~2x, but just creation of the the iterator is more costly from Rust/C++ than from JS.
Initially I thought that maybe Rust l10nregistry is performing some operations more eagerly and creation of the Iterator is more costly in result.
But:
a) 5x slower may be also indicating some imperfection in the way I wrote the C++ code.
b) the "Rust from Rust" shows that the whole operation in Rust takes only 1/20th of the time it takes from JS, so I suspect I'm doing something wrong in C++

Overall I think this PR is ready for initial review pass from Nika and Emilio on sanity and use of WebIDL, FFI and XPCOM before I dive to add asynchronous variant.
I know I'm low on documentation of that code in both in l10nregistry-rs, and in Gecko FFI. I didn't have time yet to go through the cleanup phase.
I hope the patch is in good enough shape to spot major mistakes.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #8)

It seems that overall we're getting a nice perf boost of ~2x, but just creation of the the iterator is more costly from Rust/C++ than from JS.
Initially I thought that maybe Rust l10nregistry is performing some operations more eagerly and creation of the Iterator is more costly in result.
But:
a) 5x slower may be also indicating some imperfection in the way I wrote the C++ code.
b) the "Rust from Rust" shows that the whole operation in Rust takes only 1/20th of the time it takes from JS, so I suspect I'm doing something wrong in C++

Hard to say without a profile, but my guess would be at all the allocation overhead of creating a bunch of new objects for each iteration. JS has the nursery to optimize this kind of usage pattern of "lots of temporary objects".

I'd take a profile. If that is indeed the case, there are potential ways to fix it. The simplest of course is to just return simpler types rather than interfaces...

My next guess would just be webidl overhead... That one might or might not be easy to address, depending on the stuff involved.

Comment 10

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

All right... I think I got the async iterator to work as well. I moved my NI to be r? on the PR. It's early draft, but it seems to give us L10nRegistryService producing sync or async iterator and I was able to retrieve a formatted localization out of it.

Nika, Emilio - can you take a look at the PR WIP for whether what I'm doing makes any sense? My use of XPCOM/FFI is questionable to me.

Comment 11

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

Got the async working!

Variant	Cold (ms)	Warm (ms)
JSM (from JS)	18.2	3.3
Rust (from JS)	12.2	1.9

Comment 12

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

The same test against resources needed for Preferences UI:

Variant	Cold (ms)	Warm (ms)
JSM (from JS)	21.6	3.8
Rust (from JS)	11.7	2.0

Comment 13

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

The patches are now going through talos, and I'm looking at what's left to complete this PR. I evaluated uses of L10nRegistry.jsm around our codebase and missing methods that we need to expose:

• browser/components/newtab/lib/RemoteL10n.jsm: FileSource
• browser/components/preferences/main.js: generateBundlesForLocales
• devtools/client/accessibility/panel.js: nothing
• devtools/client/shared/fluent-l10n/fluent-l10n.js: nothing
• intl/l10n/Localization.jsm: nothing
• toolkit/components/extensions/Extension.jsm: FileSource, registerSources / removeSources
• toolkit/components/processsingleton/MainProcessSingleton.jsm: eager/lazy initialization
• toolkit/mozapps/extensions/internal/AddonTestUtils.jsm: clearSources

I'll work on closing the feature gap now and the final patch!

Comment 14

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

Attachment: Bug 1660392 - [l10nregistry] part5: Move tests to use Rust L10nRegistry.

Depends on D102096

Comment 15

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

Attachment: Bug 1660392 - Apply djg's updates to L10nRegistry loading.

Depends on D102096

Comment 16

[Phabricator Automation]

Comment on attachment 9197610 [details]
Bug 1660392 - Vendor in l10nregistry-rs and fluent_fallback.

Revision D102102 was moved to bug 1672317. Setting attachment 9197610 [details] to obsolete.

Comment 17

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

Attachment: reg-io-heavy-test.js

Updated test to latest API:

Variant	Cold (ms)	Warm (ms)
JSM (sync)	4.9	2.0
Rust (sync)	1.8	0.3

Variant	Cold (ms)	Warm (ms)
JSM (async)	12.0	2.6
Rust (async)	6.2	0.9

Comment 18

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

Updated results with today's feedback applied:

Sync	Cold (ms)	Warm (ms)
JSM	5.9	1.6
Rust	1.4	0.3

Async	Cold (ms)	Warm (ms)
JSM	17.8	2.3
Rust	8.1	1.0

Comment 19

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

Quick update with the status:

The L10nFileSource bug (bug 1672317) has now r+ from :emilio and I'm getting comfortable with the code both in Gecko and in l10nregistry-rs for it. The performance & memory is good and I got a green try today. I'm waiting for a review from Nika nad maybe someone from Necko on the I/O use, but generally seems like we're unblocked for this PR which is next.

Here, the core PR is ready, most of the Intl specific tests pass, and I'm couple rounds of reviews from :emilio in, but:

• There's a potentially tricky to resolve problem with Async Iterator being passed by reference to a moz_task which requires 'static. I'm not sure how to fix that.
• Performance/Memory is good
• I need to implement the IPC filesource broadcasting to match what JSM is doing
• All Intl tests except of the one relying on that IPC pass
• I still need to clean up the code, replace unwraps and document both FFI side and l10nregistry-rs side

I'll focus on fixing the IPC and then cleaning up the code and documenting it. Assuming the 'static issue won't be too complex, I expect to get a full try run this week.

Comment 20

[Fabian Cohen]

Hello

• IPC is enable dom.ipc.tabs.createKillHardCrashReports true
  I upgrade to version 78.7.1 (32-bit), but in debug exists errors

[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 658" data: no] 2 L10nRegistry.jsm:658:19

[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 658" data: no] 2 L10nRegistry.jsm:658:19

[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 658" data: no]

Best Regards

Comment 21

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

Hi @Fabian. The error you noticed is covered by bug 1659239.

Comment 22

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

Attachment: Bug 1660392 - [l10nregistry] part1: Add support for AsyncIterator to Codegen.py.

Depends on D105394

Comment 23

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

Attachment: Bug 1660392 - [l10nregistry] part6: Move Localization to use WebIDL L10nRegistry.

Depends on D102372

Comment 24

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

Attachment: Bug 1660392 - [l10nregistry] part3: Enable L10nRegistry IPC.

Depends on D102096

Comment 25

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

Attachment: Bug 1660392 - [l10nregistry] part4: Broadcast registry updates to IPC and LocaleService.

Depends on D105572

Comment 26

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

Attachment: Bug 1660392 - [l10nregistry] part7: Move L10nRegistry callees to use WebIDL one.

Depends on D105416

Comment 27

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

Attachment: Bug 1660392 - [l10nregistry] part8: Remove L10nRegistry.jsm.

Depends on D105584

Comment 28

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

Updated results with today's feedback applied:

Sync	Cold (ms)	Warm (ms)
JSM	6.1	1.7
Rust	1.6	0.3

Async	Cold (ms)	Warm (ms)
JSM	11.2	2.0
Rust	4.9	0.6

Comment 29

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

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=664ec974001454cdd0ac4599b1aaa7c1b59875cc

Comment 30

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

Attachment: Bug 1660392 - [l10nregistry] part9: Fix racy l10n checks in browser_history_navigation.js.

Comment 31

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

Full try run on linux opd/debug - https://treeherder.mozilla.org/jobs?repo=try&revision=bb6bc06f347767463e4a63748a029cdec3b10dd6&selectedTaskRun=aAGLiMRJSKqVWcuLyF5VSQ.0

All oranges seems to be known intermittents

Comment 32

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

Attachment: Bug 1660392 - [l10nregistry] part10: Fix mozapps update test to await for gAppUpdater.

Comment 33

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

green try: https://treeherder.mozilla.org/jobs?repo=try&revision=af7593d9afcbc3837a460708913d5c3b392bff5b

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5ed9d5764a4
[l10nregistry] part1: Add support for AsyncIterator to Codegen.py. r=edgar
https://hg.mozilla.org/integration/autoland/rev/ba23990f6fcd
[l10nregistry] part2: Add L10nRegistry WebIDL backed by l10nregistry-rs. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/347ff021ea9a
[l10nregistry] part3: Enable L10nRegistry IPC. r=nika,emilio,platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/10e661d4f2f8
[l10nregistry] part4: Broadcast registry updates to IPC and LocaleService. r=nika,emilio,platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/31de0887ee42
[l10nregistry] part5: Move tests to use Rust L10nRegistry. r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/e1c2fcc0e0d5
[l10nregistry] part6: Move Localization to use WebIDL L10nRegistry. r=platform-i18n-reviewers,nordzilla
https://hg.mozilla.org/integration/autoland/rev/412640392ffd
[l10nregistry] part7: Move L10nRegistry callees to use WebIDL one. r=preferences-reviewers,Gijs,platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/5bc160109e45
[l10nregistry] part8: Remove L10nRegistry.jsm. r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/844ae4352aa7
[l10nregistry] part9: Fix racy l10n checks in browser_history_navigation.js. r=robwu
https://hg.mozilla.org/integration/autoland/rev/230c4f19d141
[l10nregistry] part10: Fix mozapps update test to await for gAppUpdater. r=application-update-reviewers,bytesized

Comment 35

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c5ed9d5764a4
https://hg.mozilla.org/mozilla-central/rev/ba23990f6fcd
https://hg.mozilla.org/mozilla-central/rev/347ff021ea9a
https://hg.mozilla.org/mozilla-central/rev/10e661d4f2f8
https://hg.mozilla.org/mozilla-central/rev/31de0887ee42
https://hg.mozilla.org/mozilla-central/rev/e1c2fcc0e0d5
https://hg.mozilla.org/mozilla-central/rev/412640392ffd
https://hg.mozilla.org/mozilla-central/rev/5bc160109e45
https://hg.mozilla.org/mozilla-central/rev/844ae4352aa7
https://hg.mozilla.org/mozilla-central/rev/230c4f19d141

Comment 36

[Worcester12345]

I get the following in my Thunderbird "error console":

[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 658" data: no] L10nRegistry.jsm:658:19
loadSync resource://gre/modules/L10nRegistry.jsm:658
fetchFile resource://gre/modules/L10nRegistry.jsm:573
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:478
map self-hosted:240
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:473
generateResourceSetsForLocaleSync resource://gre/modules/L10nRegistry.jsm:415
InterpretGeneratorResume self-hosted:1151
next self-hosted:1099
generateBundlesSync resource://gre/modules/L10nRegistry.jsm:177
InterpretGeneratorResume self-hosted:1151
next self-hosted:1099
touchNext resource://gre/modules/Localization.jsm:167
regenerateBundles resource://gre/modules/Localization.jsm:552
activate resource://gre/modules/Localization.jsm:243
<anonymous> chrome://openpgp/content/modules/trust.jsm:11
<anonymous> chrome://openpgp/content/modules/keyRing.jsm:17
<anonymous> chrome://openpgp/content/modules/windows.jsm:19
loader chrome://openpgp/content/modules/lazy.jsm:20
getService chrome://openpgp/content/modules/core.jsm:427
getService chrome://openpgp/content/modules/core.jsm:184
init chrome://openpgp/content/BondOpenPGP.jsm:104
InterpretGeneratorResume self-hosted:1151
AsyncFunctionNext self-hosted:693

Not sure if this is related or not. I think I have another similar bug open on this, but cannot find it.

I would love to some day have no errors in my "error console".

This is on Thunderbird 78.12.0 (64-bit). Will this be fixed in a future Thunderbird?