1389384 - Consider batching l10n mutations to one per paint

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

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.

Comment 1

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

Attachment: Bug 1389384 - Batch l10n mutations to one per paint.

Review commit: https://reviewboard.mozilla.org/r/208018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208018/

Comment 2

[Zibi Braniecki [:zbraniecki][: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 https://github.com/projectfluent/fluent.js/pull/113

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8937306 [details]
Bug 1389384 - Batch l10n mutations to one per paint.

https://reviewboard.mozilla.org/r/208018/#review217900

::: 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?

Comment 4

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

Comment on attachment 8937306 [details]
Bug 1389384 - Batch l10n mutations to one per paint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208018/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67f5e518f575
Batch l10n mutations to one per paint. r=mossop

Comment 6

[Raul Gurzau (:RaulG)]

Backed out changeset 67f5e518f575 (bug 1389384) for failing xpcshell tests on intl/l10n/test/test_domlocalization.js on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/2ddc9ad2b919dffe71b7a71ef01eac2864d7c7b0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=67f5e518f5751d7d037db95d5d7e705d04d1752e

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=156046114&repo=autoland&lineNumber=2067

[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

Comment 7

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

Comment on attachment 8937306 [details]
Bug 1389384 - Batch l10n mutations to one per paint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208018/diff/2-3/

Comment 8

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

Thank you! Pushed with an update.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e140335f1237
Batch l10n mutations to one per paint. r=mossop

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e140335f1237