Consider batching l10n mutations to one per paint

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

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.
(Assignee)

Updated

2 years ago
Blocks: 1365426
Priority: -- → P3
(Assignee)

Updated

a year ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1424682
No longer blocks: 1365426

Comment 3

a year ago
mozreview-review
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?
Attachment #8937306 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)

Comment 5

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67f5e518f575
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

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
Flags: needinfo?(gandalf)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
Thank you! Pushed with an update.
Flags: needinfo?(gandalf)

Comment 9

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e140335f1237
Batch l10n mutations to one per paint. r=mossop
(Assignee)

Updated

a year ago
Depends on: 1430697

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e140335f1237
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.