Closed
Bug 1389384
Opened 7 years ago
Closed 7 years ago
Consider batching l10n mutations to one per paint
Categories
(Core :: Internationalization, enhancement, P3)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(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.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years 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•7 years ago
|
Comment 3•7 years 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) |
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67f5e518f575
Batch l10n mutations to one per paint. r=mossop
Comment 6•7 years ago
|
||
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) |
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e140335f1237
Batch l10n mutations to one per paint. r=mossop
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•