Closed Bug 1389384 Opened 3 years ago Closed 3 years ago

Consider batching l10n mutations to one per paint


(Core :: Internationalization, enhancement, P3)




Tracking Status
firefox59 --- fixed


(Reporter: zbraniecki, Assigned: zbraniecki)




(1 file)

Currently, DOMLocalization API performs all translations in response to MutationObserver callback.

MutationObserver does some batching, but according to :smaug (see bug 1214026) there could be a value in scheduling a single localization per frame.

Basically, in the onMutations callback, we would check if there's already a pending rAF scheduled.
If there is, we would only add our modified elements to the list of elements to be localized.
If there isn't we would schedule one.

Once rAF is reached, we would translate in batch all elements collected from all mutation observer callbacks since the previous rAF.

My only concern is that recently in bug 1381988 comment 26 :smaug pointed out that rAF forces early layout/paint which maybe something we want here, or not.

Either way, worth investigating.
Priority: -- → P3
Assignee: nobody → gandalf
I investigate this more with my new profiling powers against the Preferences and checked back what we did for Firefox OS back in a day (when :julienw reported similar issue against Firefox OS preferences!).

Fortunately, at the moment Preferences only really trigger several mutations on startup and all of them are lightweight and synchronously triggered in the same microtask, so this is not very urgent, but a nice cleanup.

I wrote my own mock up that triggered setting `data-l10n-id` in various intervals and sometimes in seprate microtasks and was able to produce scenarios where we triggered separate `translateMutations` and in result separate `applyTranslations` all within a single paint frame.

This patch fixes that coalescing all elements into a single batch that gets resolved in the upcoming rAF.

The corresponding fluent-dom patch is in
Blocks: 1424682
No longer blocks: 1365426
Comment on attachment 8937306 [details]
Bug 1389384 - Batch l10n mutations to one per paint.

::: intl/l10n/DOMLocalization.jsm:482
(Diff revision 1)
>                if (addedNode.childElementCount) {
> -                this.translateFragment(addedNode);
> +                for (let elem of this.getTranslatables(addedNode)) {
> +                  this.pendingElements.add(elem);
> +                }
>                } else if (addedNode.hasAttribute(L10NID_ATTR_NAME)) {
> -                this.translateElement(addedNode);
> +                this.pendingElements(addedNode);

Shouldn't this be this.pendingElements.add?
Attachment #8937306 - Flags: review?(dtownsend) → review+
Pushed by
Batch l10n mutations to one per paint. r=mossop
Backed out changeset 67f5e518f575 (bug 1389384) for failing xpcshell tests on intl/l10n/test/test_domlocalization.js on a CLOSED TREE


Push with failures:


[task 2018-01-12T22:58:08.062Z] 22:58:08     INFO -  TEST-START | intl/l10n/test/test_domlocalization.js
[task 2018-01-12T22:58:14.870Z] 22:58:14  WARNING -  TEST-UNEXPECTED-FAIL | intl/l10n/test/test_domlocalization.js | xpcshell return code: 0
[task 2018-01-12T22:58:14.875Z] 22:58:14     INFO -  TEST-INFO took 6809ms
Flags: needinfo?(gandalf)
Thank you! Pushed with an update.
Flags: needinfo?(gandalf)
Pushed by
Batch l10n mutations to one per paint. r=mossop
Depends on: 1430697
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.