1558602 - Add an option to localize document synchronously

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P2)

Description

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

Currently, Fluent in DocumentL10n is fully asynchronous due to async I/O.

bz pointed out that in case of some documents such as browser.xhtml we will likely want to block I/O, at least until we refactor browser.xhtml to handle lazy localization.

Until then, we block layout on it, so first paint, so there's no benefit in using async I/O.

From my early talos tests it also seems like we may be able to improve performance for the startup scenario and unblock things like bug 1501886.

My approach is to add mIsSync flag to DOMLocalization and data-l10n-sync attribute on a document that we want to localize synchronously.

All API calls to document.l10n.* are still asynchronous, but the initial translation uses a new method FormatMessagesSync which should be faster because it doesn't have to go through microtasks.

Comment 1

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

Attachment: Bug 1558602 - Allow DocumentL10n to use LocalizationSync.

Comment 2

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

Olli - I think my use of JSAPI to juggle strings and L10nMessages between JS and C++ here is atrocious. I'm sorry for that :(

Comment 3

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

I also will want to refactor tests to be able to run them with sync and async and verify that both cases work, but first I'd like to get your initial feedback on the patch itself.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a enhancement, but please change it back in case of error.

Comment 5

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08d4a43c942a3db69b163bce441a02c189c4af21&selectedJob=251821684

Comment 6

[Axel Hecht [:Pike]]

I have some thoughts on the patch. Taking them here because they're more about "what" and less about "how".

First and foremost, I think it's the right thing to lift the ban of sync IO in our Fluent APIs. That sounds subtle, but it actually isn't.

In my book, we need some additional changes:

• Deprecation and then removal of LocalizationSync.
• Unification of sync and async generateBundles. Right now, you can give different locales and resources and even algorithm for each. That was OK when the two were completely distinct, but now that they're not, that's not how it should work.
• Documentation updates.

The first is relatively straightforward, I guess. It's just mozIntl right now?

The second is a bit hairy. I think that the tuple of generators should be replaced by an object that builds in the cache, and implement both async and sync iteration. This would unify and obsolete CachedIterator. This might make it easier to write testing for anything that currently modifies and resets the l10n registry?

The third is not just docs, but also policy and guidelines on when it's OK to use sync IO. That's even actual browser-arch work, I'd think?

Let's talk about how to get there?

Comment 7

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

Let's talk about how to get there?

The changes you propose seem to cover:

• Localization API (https://github.com/projectfluent/fluent.js/tree/master/fluent-dom)
• CachedIterable (https://github.com/projectfluent/cached-iterable)
• L10nRegistry

And in result

• fluent-react (https://github.com/projectfluent/fluent.js/tree/master/fluent-react)
• fluent-rs (https://github.com/projectfluent/fluent-rs)

I have a lot of questions about your proposal. I don't understand what signature of methods on Localization do you propose, how such object that builds in the cache should look like, etc.

But the bottom line is that if I'm not mistaken, this patch can land and enable me now, while we work on the proposed refactor. We're one r+ away from enabling removal of DTD on the startup path. Do you believe we should block that on your proposed changes?

Comment 8

[Axel Hecht [:Pike]]

FYI, I don't think that the l10n registry changes would not bleed into that much code paths. I'm still expecting the generateBundles to return an iterable, as it does now.

Zibi, looking at bug 1557713, should the sync API in the content process be blocked until we can work it out? How would mozIntl be affected?

Comment 9

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

How would mozIntl be affected?

getLanguageDisplayNames, getRegionDisplayNames and getLocaleDisplayNames would not work in content process.
They also don't work now I believe.

Comment 10

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

green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcdab5fefa62f944822373ac8d0344a39f576c4

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24e366d72108
Allow DocumentL10n to use LocalizationSync. r=smaug,Pike

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/24e366d72108