Closed Bug 1389384 Opened 7 years ago Closed 7 years ago

Consider batching l10n mutations to one per paint

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

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.
Priority: -- → P3
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
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
Blocks: 1424682
No longer blocks: 1365426
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+
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)
Thank you! Pushed with an update.
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e140335f1237 Batch l10n mutations to one per paint. r=mossop
Depends on: 1430697
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: