1613705 - Migrate Localization API to Rust

Status: RESOLVED FIXED

(Core :: Internationalization, task, P2)

Description

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

Once we have bug 1560038 fixed, the next step will be to remove mozILocalization, and Localization.jsm.

Comment 1

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

Here's the upper bound of the wins - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=dba043e0c81a6b8422d69873b28de38fc8ad336e&newProject=try&newRevision=df8beab44d49776375e3ad24c6d4ea2711363c9b&framework=1

I removed L10nRegistry.jsm and Localization.jsm and basically perform no localization. I was able to launch the browser and use it in limited capability without l10n :)

Comment 2

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

The results are very preliminary. There's still a lot of potential improvements on all levels, but here are talos numbers from a fairly complete build that replaces L10nRegistry.jsm and Localization.jsm with Rust/C++: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9bdda7f8c5ace35705e0961b55fba99f99fbe77c&newProject=try&newRevision=9b4e419e79518947b597079390e3cd3adfba760a&framework=1

sessionrestore, startup_about_home, ts_paint, twinopen wins!

This is just based on a very raw POC, with a number of limitations:

• arguments are not yet passed
• I/O is synchronous even for preferences (in m-c sync is only used for startup)
• No fallback
• L10nRegistry with its hot-plugging is yet to be added - https://github.com/zbraniecki/l10nregistry-rs

Despite that, I believe the results should remain similar when missing features get added for all startup path tests, while non-startup path is a matter of sync/async as the only likely significant factor. The rest of the missing bits are either affecting small number of strings (arguments) or fallback scenarios.

Further optimization avenues are:

• fluent-syntax (Parser)
  • Use of unsafe slicing shows up to 20% win on parsing https://github.com/projectfluent/fluent-rs/issues/82
  • Lexer branch shows up to 66% win on parsing https://github.com/projectfluent/fluent-rs/pull/161
  • Pushing parsing off the main thread may also help
• fluent-bundle (resolver)
  • Writer pattern can give up to 10% perf win on resolution https://github.com/projectfluent/fluent-rs/pull/127
• Gecko bindings
  • A number of suboptimal allocations and UTF8<-->UTF16 conversions that we can still optimize
  • Some of the API calls are unnecessary slow because we needed it for the JSMs
  • IntlMemoizer is currently created per-bundle, but could be shared between all bundles in a single process/locale

It's hard to evaluate how those improvements would translate to actual talos numbers, but the numbers we get already are often fairly close to the upper bounds listed in the previous comment, so my hope is that they would mostly help us get wins in areas which the upper bound shown as potential wins but this talos compare doesn't show any significant wins for yet (quite often showing wins but below significance level).

What's nice is that basically all of the things I listed here are already reasonable to assume as achievable, not just a potential research, but as a clear forward path.

Comment 3

[Axel Hecht [:Pike]]

Just to make sure, is https://searchfox.org/mozilla-central/rev/cc0f9906505d0ffa0d3a29a80142379e21cb5fb2/browser/components/newtab/lib/RemoteL10n.jsm#65 with explicit creation of DOMLocalization on your list of things to track?

Comment 4

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

Attachment: Bug 1613705 - [WIP] Migrate Localization API to Rust.

Comment 5

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

Just to make sure, is https://searchfox.org/mozilla-central/rev/cc0f9906505d0ffa0d3a29a80142379e21cb5fb2/browser/components/newtab/lib/RemoteL10n.jsm#65 with explicit creation of DOMLocalization on your list of things to track?

Yep! All IDLs will retain functionality! (I'm adding two options that are in mozILocalization to Localization.webidl).

Comment 6

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

Arraching WIP patch!

The WIP patch is extremely simple, reduces the whole l10nregistry and localization to the bare minimum, but it does work and Firefox with it is completely usable!

Similarly to bug 1560038 comment 57 I did some initial performance measurements.

This time, I used a much simpler test on the preferences.xhtml page:

{
async function testResolving() {
  let l10nElements = Array.from(document.querySelectorAll("[data-l10n-id]")).map(elem => document.l10n.getAttributes(elem));
  performance.mark("fluent-resolve-start");
  let resolveStart = performance.now();
  let values = await document.l10n.formatMessages(l10nElements);
  let resolveEnd = performance.now();
  performance.mark("fluent-resolve-end");
  console.log(`perf(resolve): ${resolveEnd-resolveStart}`);
}

testResolving();
}

and generated profiles:

Variant	Profile Fast	Profile Detailed
Localization.jsm+Fluent.jsm	https://perfht.ml/2w973QU	https://perfht.ml/2PpKi1L
localization-rs+fluent-rs	https://perfht.ml/3caOyMo	https://perfht.ml/2T0GGW6

All profiles have markers for fluent-resolving start/end.

1. Fast

This one collects minimal amount of data, uses 1ms sampling, and is meant to keep the overhead low to give us a good approximation of the actual performance difference.

Results

Test	JS	Rust	Decrease	Percent
Resolve Time	9.2ms	1.2ms	-8.0 ms	-86.95%
Resolve Memory	1080 KB	226 KB	-854 KB	-79.07%

Hooray! Seems like my worry from bug 1560038 comment 57 is alleviated with the move of Localization+L10nRegistry classes to Rust!

As mentioned there, I still expect a Writer trait to yield some performance/memory gains, but even without that, we're seeing a pretty substantial drop in both!

2. Detailed Resolver

The overhead makes reading the profiles a bit tricky, but core characteristics are preserved. JS FormatPattern takes ~8ms, while Rust format_pattern takes 0.7ms.

The whole profile is still a bit noisy, and I won't dive too deep into it yet.

Next, I'd like to clean up the bindings between Localization.cpp and Localization-rs and in particular figure out how to handle async I/O.

Comment 7

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

This could be useful for an IDL-backed I/O - https://phabricator.services.mozilla.com/D76743

Comment 8

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

Attachment: Bug 1613705 - WIP Localization in Rust

Comment 9

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

Attachment: test.js

With the latest version of the WIP I get to compare Localization (JS) and Localization2 (Rust) classes.

I tested loading a main preferences.ftl file as if it was loaded for Preferences UI and formatting them all. This scenario should give us some approximation of how this work may affect Preferences UI Localization performance/memory cost.

This is an early evaluation, because the Rust side is using sync I/O, as we wait to integrate async, but except of that, the results are from a complete translation with the same behavior of fallbacking and analogous L10nRegistry in Rust.

Variant	Profile	Time (ms)	Memory (Mb)
JS (1)	https://share.firefox.dev/37ayRok	12.19	8.58
JS (2)	https://share.firefox.dev/31bovAU	12.05	8.64
JS (3)	https://share.firefox.dev/3lR1UkU	13.14	8.47
Rust (1)	https://share.firefox.dev/37evJYy	5.38	1.55
Rust (2)	https://share.firefox.dev/3lUA8Ec	5.62	1.79
Rust (3)	https://share.firefox.dev/3dA4F73	6.85	1.73

Steps to Reproduce:

1. Apply the WIP patch
2. Take https://github.com/projectfluent/fluent-rs/blob/master/fluent-bundle/benches/preferences.ftl
3. Put it in `browser/locales/en-US/browser/preferences/preferences2.ftl
4. Rebuild Firefox
5. Open Firefox with Profiler
6. Wait 5s for startup to settle
7. Open Browser Console
8. Paste the example script using either Localization or Localization2)
9. Start Profiler (ctrl+shift+1)
10. Execute the script in browser console
11. Capture Profile (ctrl+shift+2)
12. Main Process, Markers, select localization.

Comment 10

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

Attachment: Bug 1613705 - Swap Localization with Localization2

Depends on D91709

Comment 11

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

Updated the patches to @djg's work which is now merged to master of l10nregistry-rs and fluent-rs repos.

The first PR allows for easy testing of the Localization in isolation. Steps to work with it are in the previous comment
Next steps for it are:

1. Plug the async logic so that we can call the test sync or async using Localization2
2. Clean up the code to minimize allocations and I/O

The second patch swaps Localization to use the new Localization2 allowing us to remove Localization.jsm and L10nRegistry.jsm. I got it to run startup and preferences, so this is what I'm running against Talos at the moment.

Comment 12

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

I folded preferences2.ftl sample file into the first patch, so the new steps to reproduce are:

• Apply the WIP patch
• Rebuild Firefox
• Open Firefox with Profiler
• Wait 5s for startup to settle
• Open Browser Console
• Paste the example script using either Localization or Localization2)
• Start Profiler (ctrl+shift+1)
• Execute the script in browser console
• Capture Profile (ctrl+shift+2)
• in the profile, select Main Process, Markers, and filter localization.

Comment 13

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

Attachment: test2.js - Heavy I/O

Update on the performance front.

The benchmarks above show the performance of loading 1 file and formatting a lot of messages in it.

When testing the performance of the whole new system we noticed that those performance wins don't translate to real talos numbers.

I profiled it and found that the reason is that l10nregistry-rs core algorithm for source orders was not using any optimization techniques. It is a very complex algorithm and the JS version of it [0] has been hard to write, hard to maintain and not very well understood.

I spent last two weeks writing a new version of it named ProblemSolver.
In the process I learned that the problem scope is actually not well solved by generic algorithms and the class of problems we're solving here requires quite fine-tuned heuristics.

I wrote two new algorithms - serial for synchronous I/O and parallel for asynchronous, and documented the algorithm and optimizations extensively [1].

The good news is that the new test (attaching) is showing a really promising perf wins! This test measures loading of a large localization context based on Preferences UI with 16 resources from 2 sources.

I separated "cold" which is the first call that actually performs I/O and caches the result, and "warm" which are all consequitive loads that re-use the cache.

Here are the results as of today:

Variant	Cold	Warm
L10nRegistry.jsm	10.2	4.1
l10nregistry-rs (master)	341.6	329.1
l10nregistry-rs (pruning)	6.6	1.5

There's still more work to be done to get the asynchronous code hooked up into L10nRegistry, but the results look very promising!

My next step is to test it against Talos in hopes that the perf wins on resolution and on I/O will result in end-to-end neutral or wins.

[0] https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/intl/l10n/L10nRegistry.jsm#346-393
[1] https://github.com/zbraniecki/l10nregistry-rs/blob/pruning2/src/solver/README.md

Comment 14

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

The improvements lead to at least comparable talos run! https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=6dec23a0ddbf46cc40d26a1f66ef83cd24abba2f&newProject=try&newRevision=328221e959a3c7cb917a37c0beeacd639212425b&framework=1

There's still a number of performance improvements that should lead to wins:

• We compare async (JSM) to sync (rust) for now. Async should lead to improvements
• We allocate unnecessairly, we'll clean it up on the way
• The async optimizations are not complete. I'll work on that next

Comment 15

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

Attachment: Test for formatting of many strings at once

Comment 16

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

Attachment: Test for heavy I/O

Comment 17

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

New numbers based on the latest advancements in the L10nRegistry pruning3 problem solver algorithmic branch:

Formatting heavy test:

Localization	Sync	Cold	Warm
JavaScript	false	21.0	15.1
Rust	false	13.3	7.5
JavaScript	true	19.8	15.2
Rust	true	8.3	6.9

I/O heavy test:

Localization	Sync	Cold	Warm
JavaScript	false	25.4	4.3
Rust	false	13.4	3.1
JavaScript	true	11.4	4.0
Rust	true	6.4	1.5

Comment 18

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

Attachment: Bug 1613705 - Introduce a WIP Localization2 class to run side by side with JS backed Localization.

Comment 19

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

Attachment: browser-context.js

Third test - this one replicates loading all messages necessary for browser.xhtml localization.

Comment 20

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

I released fluent-bundle 0.14 and fluent-fallback 0.1.0 and am almost ready to release initial l10nregistry-rs!

I believe at this point all the features in Rust are complete, although the code would benefit from cleanups and documentation.

I vendored in those versions into class Localization2 on D100643.

Using that PR I got the following results:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	18.6	14.6
JavaScript	true	18.9	14.8
Rust	false	12.0	8.5
Rust	true	8.1	7.1

I/O heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	17.3	4.5
JavaScript	true	8.7	3.3
Rust	false	15.7	3.5
Rust	true	5.9	1.7

I also added a third test that replicates very closely what browser.xhtml localization requires.

This test is also available in https://github.com/zbraniecki/l10nregistry-rs crate as cargo bench --all-features registry/scenarios/browser and cargo bench --all-features localization/scenarios/browser.

The results are as follows:

Localization	Sync	Cold	Warm
Rust (standalone)	false		414 us
Rust (standalone)	true		394 us
JavaScript	false	9.8 ms	7.1 ms
JavaScript	true	8.3 ms	7.9 ms
Rust (gecko)	false	15.0 ms	4.7 ms
Rust (gecko)	true	6.6 ms	3.2 ms

Things to notice:

• I the browser.xhtml test async Rust in Gecko takes a long time on cold load. No idea why.
• Standalone Rust completes the same test in ~10% of the time it takes when launched from JS in Gecko. I assume some of that is inevitable, but it seems like the Gecko bits wrangling costs ~90% of the time. Likely we clone a lot of Strings, where fluent-fallback returns Cow's and we need nsCString. Wondering if we can clean that up.

Comment 21

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

Attachment: Bug 1613705 - Swap Localization with Localization2

Comment 22

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

I plugged the new chain into the DOM bindings and ran talos test:

./mach talos-test --activeTests about_preferences_basic --tpcycles 10

The result's I got:

Test	JS	Rust
Preferences	203	687.40
blank	25.96	452.45
#search	384.95	143.56
#privacy	460.77	202.98
#sync	431.44	129.13
#home	187.09	162.91

The subpages seems to work much faster, but there's something slow going on with the main load and switching to blank. I'm wondering if it's something related to destructors, and costly drops.

I'll try to debug it, but :djg - maybe you'll have an idea on what may be the cause of it?

Comment 23

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

I'm getting shutdown leaks and crashes with those two patches in debug mode - https://paste.mozilla.org/RFJXgwdg - likely related to the perf issue above.

Comment 24

[u480271]

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

Test	JS	Rust
Preferences	203	687.40
blank	25.96	452.45

Wow that is a lot slower.

Comment 25

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

I applied latest patches from Dan, and rebased the second patchset on top, but I'm still seeing shutdown crashes and "leaking the world". Trying to debug.

Comment 26

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

:emilio spotted that the new Localization path wasn't GC'ing mGlobal! That fixed the leak.

New talos numbers:

./mach talos-test --activeTests about_preferences_basic --tpcycles 10

The result's I got:

Test	JS	Rust
Preferences	203	555.01
blank	25.96	387.77
#search	384.95	148.31
#privacy	460.77	186.58
#sync	431.44	117.44
#home	187.09	150.73

The blank and main Preferences are still significantly slower, while the rest is either on par of faster.

Comment 27

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

With the fixes in place and on top of fluent-rs 0.14, the test benchmarks listed here start showing consistent wins:

Format heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	22.1	14.2
Rust	false	11.8	7.3
JavaScript	true	20.0	16.3
Rust	true	8.6	6.8

I/O heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	21.9	3.9
Rust	false	14.3	1.9
JavaScript	true	12.0	2.8
Rust	true	5.5	1.3

browser.xhtml test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	11.5	7.1
Rust	false	11.9	3.5
JavaScript	true	8.4	7.3
Rust	true	7.5	3.1

I'm going to focus on talos tests now.

Comment 28

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

First positive results from talos!

https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=9e4b189f6fa6e2440f03d6241ef150b9a54b2860&newProject=try&newRevision=862afb946815126e187f2fe9f3af1b426bfc2aa0&framework=1

The about_preferences wins are suspiciously large, but I am able to run Preferences views and the talos test locally and they seem to work. I can also run most of the mochitest for Preferences UI. I'm going to debug further.

The other wins are below noise level, but at most show some wins.

I'm going to debug the ts_paint a little bit more to understand where the performance goes, but I think we're getting to the point where we can start separating out patches for full review and landing!

Comment 30

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

Spinning off L10nRegistry part that is gearing up for landing first in bug 1660392.

Comment 31

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

Attachment: Bug 1613705 - Swap Localization to use localization-rs.

Depends on D102490

Comment 32

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

Attachment: Bug 1613705 - Swap Localization to use localization-rs.

Depends on D102096

Comment 33

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

Since I separated out the fluent-fallback FFI to localization-ffi crate, I can't link this last PR:

 1:01.81     Finished release [optimized] target(s) in 2.23s
 1:27.07    Compiling geckoservo v0.0.1 (/projects/mozilla-unified/servo/ports/geckolib)
 1:27.69    Compiling gkrust v0.1.0 (/projects/mozilla-unified/toolkit/library/rust)
 1:31.85     Finished release [optimized] target(s) in 1m 13s
 1:32.04 toolkit/library/build/libxul.so
 1:33.60 ld.lld: error: undefined hidden symbol: localization_new
 1:33.60 >>> referenced by Localization.cpp:107 (/projects/mozilla-unified/intl/l10n/Localization.cpp:107)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::Create(nsTArray<nsTString<char> > const&, bool))
 1:33.60 >>> referenced by Localization.cpp:107 (/projects/mozilla-unified/intl/l10n/Localization.cpp:107)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::Localization(nsTArray<nsTString<char> > const&, bool))
 1:33.60 >>> referenced by Localization.cpp:117 (/projects/mozilla-unified/intl/l10n/Localization.cpp:117)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::Localization(nsIGlobalObject*, nsTArray<nsTString<char> > const&, bool))
 1:33.60 >>> referenced 3 more times
 1:33.60 ld.lld: error: undefined hidden symbol: localization_release
 1:33.60 >>> referenced by LocalizationBindings.h:20 (/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/dist/include/mozilla/intl/LocalizationBindings.h:20)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::~Localization())
 1:33.60 >>> referenced by LocalizationBindings.h:20 (/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/dist/include/mozilla/intl/LocalizationBindings.h:20)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::~Localization())
 1:33.60 ld.lld: error: undefined hidden symbol: localization_upgrade
 1:33.60 >>> referenced by Localization.cpp:406 (/projects/mozilla-unified/intl/l10n/Localization.cpp:406)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::SetIsSync(bool))
 1:33.60 >>> referenced by Localization.cpp:406 (/projects/mozilla-unified/intl/l10n/Localization.cpp:406)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::Upgrade())
 1:33.60 ld.lld: error: undefined hidden symbol: localization_add_res_id
 1:33.60 >>> referenced by Localization.cpp:197 (/projects/mozilla-unified/intl/l10n/Localization.cpp:197)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::AddResourceId(nsTSubstring<char16_t> const&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_remove_res_id
 1:33.60 >>> referenced by Localization.cpp:202 (/projects/mozilla-unified/intl/l10n/Localization.cpp:202)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::RemoveResourceId(nsTSubstring<char16_t> const&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_add_res_ids
 1:33.60 >>> referenced by Localization.cpp:210 (/projects/mozilla-unified/intl/l10n/Localization.cpp:210)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::AddResourceIds(nsTArray<nsTString<char16_t> > const&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_remove_res_ids
 1:33.60 >>> referenced by Localization.cpp:219 (/projects/mozilla-unified/intl/l10n/Localization.cpp:219)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::RemoveResourceIds(nsTArray<nsTString<char16_t> > const&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_value
 1:33.60 >>> referenced by Localization.cpp:232 (/projects/mozilla-unified/intl/l10n/Localization.cpp:232)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatValue(nsTSubstring<char> const&, mozilla::dom::Optional<mozilla::dom::Record<nsTString<char>, mozilla::dom::Nullable<mozilla::dom::OwningUTF8StringOrDouble> > > const&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_values
 1:33.60 >>> referenced by Localization.cpp:246 (/projects/mozilla-unified/intl/l10n/Localization.cpp:246)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatValues(mozilla::dom::Sequence<mozilla::dom::OwningUTF8StringOrL10nIdArgs> const&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_messages
 1:33.60 >>> referenced by Localization.cpp:278 (/projects/mozilla-unified/intl/l10n/Localization.cpp:278)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatMessages(mozilla::dom::Sequence<mozilla::dom::OwningUTF8StringOrL10nIdArgs> const&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_value_sync
 1:33.60 >>> referenced by Localization.cpp:317 (/projects/mozilla-unified/intl/l10n/Localization.cpp:317)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatValueSync(nsTSubstring<char> const&, mozilla::dom::Optional<mozilla::dom::Record<nsTString<char>, mozilla::dom::Nullable<mozilla::dom::OwningUTF8StringOrDouble> > > const&, nsTSubstring<char>&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_values_sync
 1:33.60 >>> referenced by Localization.cpp:342 (/projects/mozilla-unified/intl/l10n/Localization.cpp:342)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatValuesSync(mozilla::dom::Sequence<mozilla::dom::OwningUTF8StringOrL10nIdArgs> const&, nsTArray<nsTString<char> >&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_format_messages_sync
 1:33.60 >>> referenced by Localization.cpp:367 (/projects/mozilla-unified/intl/l10n/Localization.cpp:367)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::FormatMessagesSync(mozilla::dom::Sequence<mozilla::dom::OwningUTF8StringOrL10nIdArgs> const&, nsTArray<RefPtr<mozIL10nMessage> >&, mozilla::ErrorResult&))
 1:33.60 ld.lld: error: undefined hidden symbol: localization_prefetch
 1:33.60 >>> referenced by Localization.cpp:405 (/projects/mozilla-unified/intl/l10n/Localization.cpp:405)
 1:33.60 >>>               /projects/mozilla-unified/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(mozilla::intl::Localization::Prefetch())
 1:33.75 clang-11: error: linker command failed with exit code 1 (use -v to see invocation)
 1:33.75 make[4]: *** [/projects/mozilla-unified/config/rules.mk:545: libxul.so] Error 1
 1:33.75 make[3]: *** [/projects/mozilla-unified/config/recurse.mk:72: toolkit/library/build/target] Error 2
 1:33.75 make[2]: *** [/projects/mozilla-unified/config/recurse.mk:34: compile] Error 2
 1:33.75 make[1]: *** [/projects/mozilla-unified/config/rules.mk:355: default] Error 2
 1:33.75 make: *** [client.mk:89: build] Error 2
 1:33.75 274 compiler warnings present.
 1:33.79 Failed to parse ccache stats output: secondary config (readonly)         /etc/ccache.conf

I may have made some trivial mistake, but I compared this to other -ffi crates I introduced and tested that structs exposed via cbindgen out of localization-ffi are available, so my suspicion is that if I'm not making some trivial mistake I can't find, then maybe I'm hitting some implicit limit of functions exposed via cbindgen?

Nika, Emilio, can you help unblock me here?

Comment 34

[Emilio Cobos Álvarez (:emilio)]

diff --git a/toolkit/library/rust/shared/lib.rs b/toolkit/library/rust/shared/lib.rs
index 658446ae9c41..de834e0fa621 100644
--- a/toolkit/library/rust/shared/lib.rs
+++ b/toolkit/library/rust/shared/lib.rs
@@ -34,6 +34,7 @@ extern crate http_sfv;
 extern crate jsrust_shared;
 extern crate kvstore;
 extern crate l10nregistry_ffi;
+extern crate localization_ffi;
 extern crate mapped_hyph;
 extern crate mozurl;
 extern crate mp4parse_capi;

Comment 35

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

Attachment: Test for formatting of many strings at once

Comment 36

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

Attachment: browser-context.js

Comment 37

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

Final part of the patchset is coming together. I requested initial pass review from :emilio, to align on the general direction.

Perf numbers look good:

Format heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	12.8	8.5
Rust	false	5.5	3.4
JavaScript	true	9.9	8.2
Rust	true	3.7	3.4

I/O heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	16.9	3.3
Rust	false	6.1	0.7
JavaScript	true	4.9	2.5
Rust	true	1.7	0.3

browser.xhtml test:

Localization	Sync	Cold (ms)	Warm (ms)
JavaScript	false	3.8	5.6
Rust	false	1.4	1.4
JavaScript	true	6.6	4.7
Rust	true	1.2	1.2

Comment 38

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

Attachment: Bug 1613705 - [localization] part1: Introduce localization-ffi bindings for fluent-fallback. r?emilio,nika

Depends on D102096

Comment 39

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

Attachment: Bug 1613705 - [localization] part2: Switch Localization class to use localization-ffi. r?nika,emilio

Depends on D104788

Comment 40

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

Attachment: Bug 1613705 - [localization] part3: Refactor callers to use Localization API.

Depends on D104789

Comment 41

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

I separated the patch here into three pieces:

• localization-ffi
• swapping Localization.cpp to use localization-ffi
• Updating callers to use the new API in Localization class

I hope it'll make it easier to review, although first and second are deeply correlated.

Comment 42

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

Attachment: Bug 1613705 - [localization] part10: Remove Localization.jsm.

Depends on D104790

Comment 43

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

Attachment: browser-context.js

test updated to current m-c

Comment 44

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

Comment on attachment 9202225 [details]
Test for formatting of many strings at once

retiring the formatting benchmark. It mostly is covered by browser-context bench which is more representative.

Comment 45

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

With most of the underlying code - FileSource and L10nRegistry now close to r+, I retested against today's m-c. This time I tested three scenarios (sync and async):

• m-c
• filesource-rs + l10nregistry-rs, but Localization in JSM
• full patchset without JS

I/O heavy test:

Localization	Sync	Cold (ms)	Warm (ms)
m-c	false	13.6	2.4
FileSource + L10nRegistry	false	8.8	1.1
Rust	false	4.8	0.7
m-c	true	6.4	1.9
FileSource + L10nRegistry	true	2.0	0.6
JavaScript	true	1.9	0.4

browser.xhtml test:

Localization	Sync	Cold (ms)	Warm (ms)
m-c	false	4.8	4.1
FileSource + L10nRegistry	false	3.1	3.0
Rust	false	1.8	1.7
m-c	true	6.0	4.5
FileSource + L10nRegistry	true	4.3	4.0
JavaScript	true	1.3	1.3

Comment 46

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

Attachment: Bug 1613705 - [localization] part3: Introduce passing Registry to Localization to replace BundleGenerator argument. r?nika,emilio

Comment 47

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

Attachment: Bug 1613705 - [localization] part4: Enable observer on Localization. r?nika,emilio

Depends on D111178

Comment 48

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

Attachment: Bug 1613705 - [localization] part5: Refactor tests to use new API model.

Depends on D113247

Comment 49

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

I was able to resume finishing this patchset.

The core functionality works and the performance is satisfactory, so I'm focused now on completing edge cases, and passing tests.

In part6 I am trying to reinstate preferences observation to reset Localization on prefs change, and for that I need to make Localization implement nsISupportsWeakObserver. I did clean it up in part2 because I didn't need to hold references to JS objects anymore, so I hope it should be simpler, but at the moment with part6 in it fails to link with:

 0:51.33 ld.lld: error: undefined hidden symbol: mozilla::intl::Localization::GetWeakReference(nsIWeakReference**)
 0:51.33 >>> referenced by Unified_cpp_intl_l10n0.cpp
 0:51.33 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(vtable for mozilla::intl::Localization)
 0:51.33 >>> referenced by Unified_cpp_dom_l10n0.cpp
 0:51.33 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../dom/l10n/Unified_cpp_dom_l10n0.o:(vtable for mozilla::dom::DOMLocalization)
 0:51.33 >>> referenced by Unified_cpp_dom_l10n0.cpp
 0:51.33 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../dom/l10n/Unified_cpp_dom_l10n0.o:(vtable for mozilla::dom::DocumentL10n)
 0:51.34 ld.lld: error: undefined hidden symbol: non-virtual thunk to mozilla::intl::Localization::GetWeakReference(nsIWeakReference**)
 0:51.34 >>> referenced by Unified_cpp_intl_l10n0.cpp
 0:51.34 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../intl/l10n/Unified_cpp_intl_l10n0.o:(vtable for mozilla::intl::Localization)
 0:51.34 >>> referenced by Unified_cpp_dom_l10n0.cpp
 0:51.34 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../dom/l10n/Unified_cpp_dom_l10n0.o:(vtable for mozilla::dom::DOMLocalization)
 0:51.34 >>> referenced by Unified_cpp_dom_l10n0.cpp
 0:51.34 >>>               /home/mozilla/projects/mozilla-central/obj-x86_64-pc-linux-gnu-opt/toolkit/library/build/../../../dom/l10n/Unified_cpp_dom_l10n0.o:(vtable for mozilla::dom::DocumentL10n)

What's the right magic to get WrapperCache + SupportsWeakReference + Observer DECL/IMPL macros?

Comment 50

[Nika Layzell [:nika] (ni? for response)]

Make the type inherit like this:

// header file
class Localization : public nsIObserver, public nsSupportsWeakReference, public nsWrapperCache {
  public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(Localization, nsIObserver)
   NS_DECL_NSIOBSERVER

  // ... etc
};

// cpp file
NS_IMPL_CYCLE_COLLECTING_ADDREF(Localization)
NS_IMPL_CYCLE_COLLECTING_RELEASE(Localization)

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Localization)
  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
  NS_INTERFACE_MAP_ENTRY(nsIObserver)
  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver)
NS_INTERFACE_MAP_END

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_WEAK(Localization, mFieldsHere)

note that the base class is nsSupportsWeakReference not nsISupportsWeakReference. That type will implement the interface for you (https://searchfox.org/mozilla-central/rev/6143c3b83436184644a55afad83cd4d4396f7fec/xpcom/base/nsWeakReference.h#18).

Comment 51

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

Attachment: Bug 1613705 - [localization] part6: Fine tune error reporting to only report to automation and nightly.

Depends on D113248

Comment 52

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

Attachment: Bug 1613705 - [localization] part7: Re-enable custom locales argument to Localization. r?emilio,nika

Depends on D113570

Comment 53

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

Attachment: Bug 1613705 - [localization] part8: Refactor SetIsSync to be SetAsync.

Depends on D113571

Comment 54

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

Attachment: Bug 1613705 - [localization] part9: Update DOMLocalization to match Localization constructor.

Depends on D113691

Comment 55

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

Status update - the full patchset is now passing all tests in intl/l10n/test and all-but-one in dom/l10n/test!

Next steps:

• Fix the final test
• Run a try build
• Take another look at the prefetch
• Ask for reviews

Comment 56

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

It took me a bit, but I was able to narrow down the problem scope, but I don't think I understand what is going on.

The issue boils down to:

    pub fn format_messages(
        &self,
        keys: &ThinVec<L10nKey>,
        promise: &xpcom::Promise,
        callback: extern "C" fn(&xpcom::Promise, Option<&ThinVec<OptionalL10nMessage>>, &ThinVec<nsCString>),
    ) {
        let keys: Vec<FluentL10nKey> = keys.into_iter().map(|k| k.into()).collect();

        let this = RefPtr::new(self);
        let strong_promise = RefPtr::new(promise);

        moz_task::spawn_current_thread(async move {
            let mut errors = vec![];
            let ret_val = this
                .inner
                .borrow()
                .format_messages(&keys, &mut errors)
                .await
                .into_iter()
                .map(|msg| {
                    if let Some(msg) = msg {
                        OptionalL10nMessage {
                            is_present: true,
                            message: msg.into(),
                        }
                    } else {
                        OptionalL10nMessage {
                            is_present: false,
                            message: L10nMessage::default(),
                        }
                    }
                })
                .collect::<ThinVec<_>>();

            assert_eq!(keys.len(), ret_val.len());

            let errors = errors.into_iter().map(|err| err.to_string().into()).collect();

            callback(&strong_promise, Some(&ret_val), &errors);
        })
        .expect("Failed to spawn future");
    }
}

I think what is going on is that if this method is called twice concurrently the first call thread closure is never executed, and only the second is.

To reproduce, you need the whole (yes 27 patches) stack and then:

      var loc = new Localization(["browser/preferences/preferences.ftl"], false);
      var value = loc.formatValue("do-not-track-learn-more");
      var value2 = loc.formatValue("do-not-track-learn-more");
      console.log(await value);
      console.log(await value2);

in a fresh browser (if the localization resources are cached, the problem is not reproducible).

On the other hand those two work well:

      var loc = new Localization(["browser/preferences/preferences.ftl"], false);
      var value = loc.formatValue("do-not-track-learn-more");
      var value2 = loc.formatValue("do-not-track-learn-more");
      //console.log(await value);
      console.log(await value2);

      var loc = new Localization(["browser/preferences/preferences.ftl"], false);
      var value = await loc.formatValue("do-not-track-learn-more");
      var value2 = await loc.formatValue("do-not-track-learn-more");
      console.log(value);
      console.log(value2);

My debugging got to the point where if I call moz_task::spawn_current_thread , and then again before the first one is resolved, then the first one never finishes.
I'm not sure if that's the right interpretation of what I see, but I'm hoping maybe Nika or Emilio can recognize if I'm on the right track and what may be causing it.

Comment 57

[Emilio Cobos Álvarez (:emilio)]

If the two closures are executing but it's not resolving, that means that this.inner().borrow().format_messages(...).await is not returning, right? If so you need to dig into why that might be...

I'm happy to look into it a bit further, but if you want me to, mind pasting a try push with all the needed patches etc? Thanks.

Comment 58

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

If the two closures are executing but it's not resolving, that means that this.inner().borrow().format_messages(...).await is not returning, right?

I don't think so.

The order of printf's that I see is:

 pub fn format_messages(
        &self,
        keys: &ThinVec<L10nKey>,
        promise: &xpcom::Promise,
        callback: extern "C" fn(&xpcom::Promise, Option<&ThinVec<OptionalL10nMessage>>, &ThinVec<nsCString>),
    ) {
        let keys: Vec<FluentL10nKey> = keys.into_iter().map(|k| k.into()).collect();

        let this = RefPtr::new(self);
        let strong_promise = RefPtr::new(promise);

        println!("format_messages before: {:?}", keys.len());
        moz_task::spawn_current_thread(async move {
            println!("format_messages inside: {:?}", keys.len());
            // (...)
        })
        .expect("Failed to spawn future");
    }
}

and if I call: format_messages with first 2 keys, and then 4 keys, I see:

1. Calling format_messages with 2 keys
2. format_messages before: 2
3. Calling format_messages with 4 keys
4. format_messages before: 4
5. format_messages inside: 4

while if I call it just once, I see:

1. Calling format_messages with 2 keys
2. format_messages before: 2
3. format_messages inside: 2

It looks as if the second call discarded the first thread?

Comment 59

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

try build: https://treeherder.mozilla.org/jobs?repo=try&revision=0a120a2884f504bf500096c585af75073a8641a2

Comment 60

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

I'll try to catch that separately, but it seems like I'm leaking the World - https://treeherder.mozilla.org/jobs?repo=try&revision=0a120a2884f504bf500096c585af75073a8641a2&selectedTaskRun=VO1Azw3aR1SzOWke8DuhPA.0

Nika, Emilio - if you're debugging this, maybe you'll stumble upon where (or maybe its correlated?)

Comment 61

[Emilio Cobos Álvarez (:emilio)]

So I'm confused, how is format_messages involved with the snippet you provided to repro?

      var loc = new Localization(["browser/preferences/preferences.ftl"], false);
      var value = loc.formatValue("do-not-track-learn-more");
      var value2 = loc.formatValue("do-not-track-learn-more");
      console.log(await value);
      console.log(await value2);

What I do when I run that is:

format_value out: "do-not-track-learn-more", None
format_value out: "do-not-track-learn-more", None
format_value in: "do-not-track-learn-more", None
format_value in: "do-not-track-learn-more", None
(no console log)

Comment 62

[Emilio Cobos Álvarez (:emilio)]

So you have two tasks listening to two AsyncCacheStreams backed by the same Cache object, and what I see is that one of the tasks get stuck in the await and never gets waken up:

format_value out: "do-not-track-learn-more", None
format_value out: "do-not-track-learn-more", None
format_value in: "do-not-track-learn-more", None
AsyncCacheStream::poll_next(0x7fd13be90c78, 0, 0)
poll_next: false
format_value in: "do-not-track-learn-more", None
AsyncCacheStream::poll_next(0x7fd13be90598, 0, 0)
poll_next: false
AsyncCacheStream::poll_next(0x7fd13be90598, 0, 0)
poll_next: true
format_value after: "do-not-track-learn-more", None, "Learn more"

I'm not sure who's responsibility is it to awake the task, and whether it's an AsyncCacheStream bug (and it should keep track of pending wakers and such), or it's a moz_task bug.

Comment 63

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think it is a bug in AsyncCacheStream and https://github.com/projectfluent/fluent-rs/pull/224 should fix it. But note the disclaimer there.

Comment 64

[Nika Layzell [:nika] (ni? for response)]

It seems like emilio has figured the bug out, clearing ni?

Comment 65

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

Fixed the leak and intl tests are passing!

We still seem to have some leaks, but they're weird - "negative leaks".

Here are talos numbers from today - https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=bc8ebc5ba34ff5812479228f1fa5f3a8e8a01903&newProject=try&newRevision=33cd25ff3d083a677275b124313ca71ad7816b4e&framework=1

Here is a try auto build with the negative leaks - https://treeherder.mozilla.org/jobs?repo=try&revision=88fdd620c265ea2634776e44732717d1416ae1dc

Next steps:

• Debug the negative leaks
• Identify other errors from try build and fix them
• Get review
• Land!

Comment 66

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

There is a small number of talos tests crashing with:

[task 2021-05-05T21:09:05.024Z] 21:09:05     INFO -  PROCESS-CRASH | about_newtab_with_snippets | application crashed [@ RustMozCrash(char const*, int, char const*)]
[task 2021-05-05T21:09:05.024Z] 21:09:05     INFO -  Crash dump filename: C:\Users\task_1620244936\AppData\Local\Temp\tmpj5ydi9sc\profile\minidumps\4d5e2a47-da00-45f4-aa9c-14efc8d21c96.dmp
[task 2021-05-05T21:09:05.024Z] 21:09:05     INFO -  Mozilla crash reason: assertion failed: self.state.is(POLL)
[task 2021-05-05T21:09:05.024Z] 21:09:05     INFO -  Operating system: Windows NT
[task 2021-05-05T21:09:05.025Z] 21:09:05     INFO -                    10.0.17134
[task 2021-05-05T21:09:05.025Z] 21:09:05     INFO -  CPU: amd64
[task 2021-05-05T21:09:05.025Z] 21:09:05     INFO -       family 6 model 94 stepping 3
[task 2021-05-05T21:09:05.025Z] 21:09:05     INFO -       8 CPUs
[task 2021-05-05T21:09:05.026Z] 21:09:05     INFO -  GPU: UNKNOWN
[task 2021-05-05T21:09:05.026Z] 21:09:05     INFO -  Crash reason:  EXCEPTION_BREAKPOINT
[task 2021-05-05T21:09:05.026Z] 21:09:05     INFO -  Crash address: 0x5f50f680
[task 2021-05-05T21:09:05.027Z] 21:09:05     INFO -  Assertion: Unknown assertion type 0x00000000
[task 2021-05-05T21:09:05.027Z] 21:09:05     INFO -  Process uptime: 0 seconds
[task 2021-05-05T21:09:05.027Z] 21:09:05     INFO -  Thread 0 (crashed)
[task 2021-05-05T21:09:05.028Z] 21:09:05     INFO -   0  xul.dll!RustMozCrash(char const*, int, char const*) [wrappers.cpp:33cd25ff3d083a677275b124313ca71ad7816b4e : 16 + 0x10]

Comment 67

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

Attachment: Bug 1613705 - [localization] part11: Fix XPCOM future_task polling assert. r?nika

Comment 68

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

talos starts looking green - https://treeherder.mozilla.org/jobs?repo=try&revision=da03228c46f311ee1373be193715cf817a7697b0

perf comparison: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=bc8ebc5ba34ff5812479228f1fa5f3a8e8a01903&newProject=try&newRevision=da03228c46f311ee1373be193715cf817a7697b0&framework=1

Comment 69

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

Attachment: Bug 1613705 - [localization] part12: Fix browser_autocomplete_a11y_label test to not be racy.

Depends on D114508

Comment 70

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

Attachment: Bug 1613705 - [localization] part13: Fix newtab asrouter RemoteL10n test.

Depends on D116738

Comment 71

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

Attachment: Bug 1613705 - [localization] part14: Fix browser_topsites_contextMenu_options test to not be racy.

Depends on D116739

Comment 72

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

Attachment: Bug 1613705 - [localization] part15: Fix aboutprocesses to not be racy against l10n.

Comment 73

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

Attachment: Bug 1613705 - [localization] part16: Fix preferences language menu selection test.

Depends on D116755

Comment 74

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

Attachment: Bug 1613705 - [localization] part17: Fix racy tests to wait for l10n frame.

Depends on D116791

Comment 75

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

I'm crunching through the remaining test failures and I got from 68 to 28 this week: https://treeherder.mozilla.org/jobs?repo=try&revision=382db0aee1b94ca7a02a364df175a98286d6b6d0&selectedTaskRun=Lhiybl-4Ty-OhOxlC4LR_w.0

Most of the remaining issues seems to be racy tests that are affected by bug 1672317 comment 10 change in how we handle microtasks.
Fixing them is tricky but I think worth it as we're mostly fixing what should already be async but relies on particular microtask order, so hopefully those fixes will result in fewer intermittents.

Comment 76

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

Harry - can I get your help debugging https://treeherder.mozilla.org/logviewer?job_id=341789731&repo=try&lineNumber=4892 ?

I can reproduce it locally with my patchstack (34 patches, sorry!) in debug mode using ./mach test browser/components/urlbar/tests/browser/browser_keyword.js --headless --verify (won't hit it without --verify).

The issue is confusing because if I console.log(element.innerHTML) in [0] I can see that urlbarView-action has data-l10n-args and textContent, but not data-l10n-id. So it did hit some path that did removeAttribute('data-l10n-id') but not textContent = "", and my only guess is that it is some race that my patchset changes from winning to losing.

The logic that checks if actions is empty is here: https://searchfox.org/mozilla-central/source/browser/components/urlbar/tests/UrlbarTestUtils.jsm#227

but the code that sets/unsets/changes urlbarView-action id/args and textContent is spread around and in some cases instead of setting textContent ="" when data-l10n-id is removed, a callback is set, like here: https://searchfox.org/mozilla-central/source/browser/components/urlbar/UrlbarView.jsm#1453-1455 and I'm not sure why.

In my local --verify + debug test I'm hitting the error in line 115 and line 175 of the test. Can you advice on what may be going on assuming it's just some race?

[0] https://searchfox.org/mozilla-central/rev/bf8d5de8528036c09590009720bc172882845b80/browser/components/urlbar/tests/browser/browser_keyword.js#115

Comment 77

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

I think I have all remaining test failures under control.

The general outline is that we're now green + one test browser/components/urlbar/tests/browser/browser_keyword*.js which harry is looking at and a number of intermittents that this patchset changes intermittentness off.

My general sense is that in most cases we are turning those intermittents into either permaorganges or at least more likely to reproduce which is good, but it leaves a question whether we want to chase fixing all of them before we merge this.

My plan is to continue looking into the intermittents while the PRs are being reviewed, but once we have the code approach, I'd like to talk to sherrifs about landing the patchset and filing follow ups for intermittens. Fixing them should be easier once maintainers of the respective areas have a m-c build that reproduces them rather than having to apply this massive patchstack to repro.

In all cases we found so far the failures are test-specific (not affecting users) and are caused by tests firing rapidly many operations and assuming that some condition is met in a given microtask, while in fact it is now done in the next one. As you can see from part 18, the fixes are almost always just BrowserTestUtils.waitForCondition, and I'm ok continuing with that.

The side benefit is that I believe that this patchset will generally reduce the number of intermittents that are related to l10n being async, but I'm concerned about 9 months, 33 part patchset, being eventually blocked on those oranges, so I hope the sherrifs and Mozilla developer community will be open to unblock landing of that patchset soon.

Comment 78

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

:nika - when you're reviewing this, the amount of oranges indicates to me that we're inherently affecting some task/microtask in this move from jsm->native as it flares a lot of racy tests.

The previous two parts (FileSource and L10nRegistry) don't trigger it, so I assume something changes about it in this patchset around FormatMessages (which is used by the Fluent DOM under the hood) timings which extenuates the races.

I'm happy to see that we get a chance to identify and fix those races, but I'd like to ask you to pay attention to when reviewing to if we may be accidentally pushing something that used to be a microtask to a new task and if so, should we be concerned about perf impact?

I tested talos and telemetry and it seems like this should improve the perf quite nicely, but it also may be that some new event loop cycle is not captured by the talos tests.

Comment 79

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

Full test coverage on try on linux opt/debug:

• central - https://treeherder.mozilla.org/jobs?repo=try&revision=4e3a04e49f076bb4455413891d602cef5b3ad502
• filesource - https://treeherder.mozilla.org/jobs?repo=try&revision=65bdb1f374787b762e31dc0707a901cdb1860be9
• l10nregistry - https://treeherder.mozilla.org/jobs?repo=try&revision=bb6bc06f347767463e4a63748a029cdec3b10dd6
• localization - https://treeherder.mozilla.org/jobs?repo=try&revision=1a22f2f6fcdc417547abbaab707fda41f7c66806

All oranges seem to be known intermittent. As I stated in previous comments - we definitely affect the frequency of many of them which may help debug them and fix the raciness.

I'll keep working on fixing the tests but I'd like to talk about landing this next week assuming we can finish the reviews even if we still have some intermittents. None of them seem to be affecting users and I struggle to get help from code authors when I have to ask them to apply 38 patch patchset and rebuild Firefox to reproduce their intermittents.

One functional area to validate is the microtask shift that I flagged in comment 38. I'll be looking into it this week.

NI on :nika to bring attention to comment 38 :)

Comment 80

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

I spun off bug 1717241 to handle prefetch as a follow up to this work.

Performance without prefetch looks good: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=ca904ed8b066e7c8e1953aa3ea1ad7b4006c271e&newProject=try&newRevision=2896e14a12f069b66b1d3657dcbb7cf0c47b590e&framework=1

Comment 81

[Nika Layzell [:nika] (ni? for response)]

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

:nika - when you're reviewing this, the amount of oranges indicates to me that we're inherently affecting some task/microtask in this move from jsm->native as it flares a lot of racy tests.

The previous two parts (FileSource and L10nRegistry) don't trigger it, so I assume something changes about it in this patchset around FormatMessages (which is used by the Fluent DOM under the hood) timings which extenuates the races.

I'm happy to see that we get a chance to identify and fix those races, but I'd like to ask you to pay attention to when reviewing to if we may be accidentally pushing something that used to be a microtask to a new task and if so, should we be concerned about perf impact?

I tested talos and telemetry and it seems like this should improve the perf quite nicely, but it also may be that some new event loop cycle is not captured by the talos tests.

The use of spawn_current_thread will always cause at least one spin of the event loop to occur after the task is spawned before the code for the task is run, so any function which uses that will always spin the event loop at least once, even if no async work needs to be done. We probably don't need to worry too much about the perf impact, but that might be what you're observing.

Avoiding this extra pass to the event loop would technically be possible, but would take some work and wouldn't be something we'd want to happen in all cases (as it would mean that the spawn call could evaluate synchronously, which would be quite surprising).

Comment 82

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

Attachment: Bug 1613705 - [localization] part18: Fix AuditParsingOfHTMLXMLFragments to work without JS context. r?emilio

Comment 83

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

Green try: https://treeherder.mozilla.org/jobs?repo=try&revision=26605e722071f894e06eace21a2825092e984bcc

My plan is to land FileSource and L10nRegistry first, and iron out the part 17 with :mixedpuppy before landing this bug next week.

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f542b20eaa7b
Fix failures in devtools/client/performance-new/test/browser/browser_aboutprofiling-features-disabled.js CLOSED TREE

Comment 85

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b399ae149f6
lint fix. a=lint-fix. CLOSED TREE

Comment 86

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f542b20eaa7b
https://hg.mozilla.org/mozilla-central/rev/5b399ae149f6

Comment 87

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

Green try: https://treeherder.mozilla.org/jobs?repo=try&revision=4c783098ddfcbb0ef7658443e59be8844859d6b0

Comment 88

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9cfbba15d90
[localization] part1: Introduce localization-ffi bindings for fluent-fallback. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/3404fd6725f1
[localization] part2: Switch Localization class to use localization-ffi. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/8d0c087b9896
[localization] part3: Introduce passing Registry to Localization to replace BundleGenerator argument. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/6c188c4b38bf
[localization] part4: Enable observer on Localization. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/6466027c5767
[localization] part5: Refactor tests to use new API model. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/c89aa0375690
[localization] part6: Fine tune error reporting to only report to automation and nightly. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/2213917abb30
[localization] part7: Re-enable custom locales argument to Localization. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/1cbab987bb7d
[localization] part8: Refactor SetIsSync to be SetAsync. r=platform-i18n-reviewers,dminor,emilio
https://hg.mozilla.org/integration/autoland/rev/626b40e3aad1
[localization] part9: Update DOMLocalization to match Localization constructor. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/302e7a75affc
[localization] part10: Remove Localization.jsm. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/4e690882b6af
[localization] part11: Fix XPCOM future_task polling assert. r=nika

Comment 89

[Cosmin Sabou [:CosminS]]

Backed out for several test failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=4e690882b6af39e8990b687e6657c08badf93157&selectedTaskRun=RlK0xXCuR-S3FgxpuuWVCA.0

Failure logs:

• newtab: https://treeherder.mozilla.org/logviewer?job_id=347099969&repo=autoland

• non-unified: https://treeherder.mozilla.org/logviewer?job_id=347097644&repo=autoland

• browser chrome:
  https://treeherder.mozilla.org/logviewer?job_id=347099957&repo=autoland
  https://treeherder.mozilla.org/logviewer?job_id=347099966&repo=autoland
  https://treeherder.mozilla.org/logviewer?job_id=347099752&repo=autoland
  https://treeherder.mozilla.org/logviewer?job_id=347100618&repo=autoland
  https://treeherder.mozilla.org/logviewer?job_id=347100843&repo=autoland

  overview for bc's: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=4e690882b6af39e8990b687e6657c08badf93157&searchStr=bc&failure_classification_id=2

Backout link: https://hg.mozilla.org/integration/autoland/rev/87e93141e0869f6da5b08f3ba1fd133bfe7d4433

Comment 90

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

Hmm, seems like this landed only part1-part11.

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6bebff87544
[localization] part1: Introduce localization-ffi bindings for fluent-fallback. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/d88b294e0a5e
[localization] part2: Switch Localization class to use localization-ffi. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/6ef0aafd2db0
[localization] part3: Introduce passing Registry to Localization to replace BundleGenerator argument. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/08b637fc3fcd
[localization] part4: Enable observer on Localization. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/408983c64f7f
[localization] part5: Refactor tests to use new API model. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/fc3b9acb0e81
[localization] part6: Fine tune error reporting to only report to automation and nightly. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/442289058933
[localization] part7: Re-enable custom locales argument to Localization. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/9f3e2963d925
[localization] part8: Refactor SetIsSync to be SetAsync. r=platform-i18n-reviewers,dminor,emilio
https://hg.mozilla.org/integration/autoland/rev/595529cd906f
[localization] part9: Update DOMLocalization to match Localization constructor. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/93fcb23d7898
[localization] part10: Remove Localization.jsm. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/175af8a1b8c2
[localization] part11: Fix XPCOM future_task polling assert. r=nika
https://hg.mozilla.org/integration/autoland/rev/85bf98573899
[localization] part12: Fix browser_autocomplete_a11y_label test to not be racy. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/5d61617a5402
[localization] part13: Fix newtab asrouter RemoteL10n test. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/a7aeae7afd49
[localization] part14: Fix browser_topsites_contextMenu_options test to not be racy. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/5fc5918e5905
[localization] part15: Fix aboutprocesses to not be racy against l10n. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/9843372abb6e
[localization] part16: Fix preferences language menu selection test. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/de9d4378f0ac
[localization] part17: Fix racy tests to wait for l10n frame. r=platform-i18n-reviewers,dminor,application-update-reviewers,nalexander
https://hg.mozilla.org/integration/autoland/rev/d377afc0b09f
[localization] part18: Fix AuditParsingOfHTMLXMLFragments to work without JS context. r=emilio

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ee1091dd20d
Fix for failures in browser_checkNonEmptyFields.js. CLOSED TREE

Comment 93

[Atila Butkovits]

Backed out for causing build bustages complaining about Document.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/4d5846f47a99e2302b8d6f792d0af824e7c68848

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=TE1M6FT2QECHjL6jdFWbPg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=d377afc0b09f5b823bbcab7c81248df18f5c855e

Failure log:
-for bustage: https://treeherder.mozilla.org/logviewer?job_id=347225349&repo=autoland&lineNumber=5359
-for bc failures: https://treeherder.mozilla.org/logviewer?job_id=347232795&repo=autoland&lineNumber=3819
https://treeherder.mozilla.org/logviewer?job_id=347232708&repo=autoland&lineNumber=13214

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/878b052ebeaa
[localization] part1: Introduce localization-ffi bindings for fluent-fallback. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/92b87b0f776c
[localization] part2: Switch Localization class to use localization-ffi. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/7e901027262a
[localization] part3: Introduce passing Registry to Localization to replace BundleGenerator argument. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/1873a25dc33d
[localization] part4: Enable observer on Localization. r=emilio,nika
https://hg.mozilla.org/integration/autoland/rev/7e85ba836ad5
[localization] part5: Refactor tests to use new API model. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/cd03ec17b66c
[localization] part6: Fine tune error reporting to only report to automation and nightly. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/ec4e0ef9955d
[localization] part7: Re-enable custom locales argument to Localization. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/181487bdf6c0
[localization] part8: Refactor SetIsSync to be SetAsync. r=platform-i18n-reviewers,dminor,emilio
https://hg.mozilla.org/integration/autoland/rev/85194e5b7a9e
[localization] part9: Update DOMLocalization to match Localization constructor. r=nika,emilio
https://hg.mozilla.org/integration/autoland/rev/41ff3770502d
[localization] part10: Remove Localization.jsm. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/0d942291a203
[localization] part11: Fix XPCOM future_task polling assert. r=nika
https://hg.mozilla.org/integration/autoland/rev/c85e6e320576
[localization] part12: Fix browser_autocomplete_a11y_label test to not be racy. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/48fa4e836ca1
[localization] part13: Fix newtab asrouter RemoteL10n test. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/0950a9e56ceb
[localization] part14: Fix browser_topsites_contextMenu_options test to not be racy. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/e030a54c907f
[localization] part15: Fix aboutprocesses to not be racy against l10n. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/064a935a55ce
[localization] part16: Fix preferences language menu selection test. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/bce61215b4d0
[localization] part17: Fix racy tests to wait for l10n frame. r=platform-i18n-reviewers,dminor,application-update-reviewers,nalexander
https://hg.mozilla.org/integration/autoland/rev/120f83c79d5c
[localization] part18: Fix AuditParsingOfHTMLXMLFragments to work without JS context. r=emilio

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/343aef4f4fe4
Fix failures in browser_checkNonEmptyFields.js r=fix

Comment 96

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/878b052ebeaa
https://hg.mozilla.org/mozilla-central/rev/92b87b0f776c
https://hg.mozilla.org/mozilla-central/rev/7e901027262a
https://hg.mozilla.org/mozilla-central/rev/1873a25dc33d
https://hg.mozilla.org/mozilla-central/rev/7e85ba836ad5
https://hg.mozilla.org/mozilla-central/rev/cd03ec17b66c
https://hg.mozilla.org/mozilla-central/rev/ec4e0ef9955d
https://hg.mozilla.org/mozilla-central/rev/181487bdf6c0
https://hg.mozilla.org/mozilla-central/rev/85194e5b7a9e
https://hg.mozilla.org/mozilla-central/rev/41ff3770502d
https://hg.mozilla.org/mozilla-central/rev/0d942291a203
https://hg.mozilla.org/mozilla-central/rev/c85e6e320576
https://hg.mozilla.org/mozilla-central/rev/48fa4e836ca1
https://hg.mozilla.org/mozilla-central/rev/0950a9e56ceb
https://hg.mozilla.org/mozilla-central/rev/e030a54c907f
https://hg.mozilla.org/mozilla-central/rev/064a935a55ce
https://hg.mozilla.org/mozilla-central/rev/bce61215b4d0
https://hg.mozilla.org/mozilla-central/rev/120f83c79d5c
https://hg.mozilla.org/mozilla-central/rev/343aef4f4fe4

Comment 97

[Andra Esanu (needinfo me)]

== Change summary for alert #30856 (as of Tue, 10 Aug 2021 10:40:10 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
3%	welcome	ContentfulSpeedIndex	linux1804-64-shippable-qr	cold webrender	1,650.75 -> 1,608.92
2%	welcome	ContentfulSpeedIndex	linux1804-64-shippable-qr	cold webrender	1,642.88 -> 1,616.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30856

Comment 98

[Dave Hunt [:davehunt] [he/him] ⌚BST]

== Change summary for alert #30870 (as of Tue, 10 Aug 2021 18:35:23 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
9%	about_preferences_basic		linux1804-64-shippable-qr	e10s stylo webrender	139.37 -> 126.31

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30870