1503657 - Implement Fluent DOMOverlays in C++

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

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

One of the core conclusions from the analysis in bug 1441035 is that in order to improve the performance experience of using Fluent, we need to reduce the amount of JS<-->C++ back and forth.

One of the first steps could be to reduce the API surface of DOMLocalization.jsm by moving more of the DOM localization logic to C++.

The biggest chunk of it is DOMOverlays so we should rewrite it into C++ to become part of `nsINode.localize` C++ API.

Comment 1

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

Attachment: Bug 1503657 - Implement Fluent DOMOverlays in C++

Comment 2

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

This is just a scaffolding

Comment 3

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

Attachment: Bug 1503657 - Implement Fluent DOMOverlays in C++

Comment 4

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

This is a much more mature implementation of the proposed 3rd revision of the API. It's still quite early, but it should work well as a POC for the final code once we settle the API shape.

Comment 5

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

Hi Nika,

I'm looking to write a pretty isolated API for Gecko DOM. I'd love to write it in Rust, but I'm not sure how far are we from the basic DOM APIs like Element, Node etc. to be available from Rust. Can you help me understand if that's possible, or if there's any effort to get us to the point where this might be possible?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

The XPCOM Rust APIs don't provide any mechanism for accessing that sort of information from Gecko's DOM, and aren't intended to provide those kinds of features. I believe that stylo has done some work with interacting with the DOM from Rust before, but IIRC it was pretty custom.

I'll forward the ni? to :bholley because I believe he'd know more about Rust/DOM work.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)

I'm looking to write a pretty isolated API for Gecko DOM. I'd love to write it in Rust, but I'm not sure how far are we from the basic DOM APIs like Element, Node etc. to be available from Rust. Can you help me understand if that's possible, or if there's any effort to get us to the point where this might be possible?

Yeah, the layer we have for stylo is pretty custom and scattershot. We do a lot of inline access via bindgen [1] and use FFI for the rest [2].

So I don't think we really have the necessary infrastructure in place for this. We could certainly build it, but it would take some resourcing to get it right.

[1] https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/servo/components/style/gecko/wrapper.rs#261
[2] https://searchfox.org/mozilla-central/source/layout/style/GeckoBindings.h

Comment 8

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

Thank you! I'll continue working on that in C++ then!

Comment 9

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

Attachment: Bug 1503657 - Migrate nsINode::localize and DOMLocalization.jsm to use DOMOverlays C++.

Depends on D21475

Comment 10

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

I got it this to work using the current (rev. 2) iteration of DOM Overlays.

First patch introduces the component, as a WebIDL exposed C++ module and brings all the DOM Overlays tests from JS.
Second patch moves nsINode::localize and DOMLocalization.jsm to use it and simplifies DOMLocalization.jsm pathways - removing almost half of DOMLocalization.jsm!

The code is not clean yet, and the errors are not reported back anywhere, but I was able to load Preferences and pass all the intl/l10n/test/dom tests!

Unfortunately, DOMOverlays tests operate on elements that get sanitized in chrome code, so I had to manually disable the sanitization to get the tests running.

Here's a talos with the current patchset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65db64e6ff1aa0657618520090e376df2f727a2f

I also ran talos with the sanitization turned back on to ensure that the sanitization switch doesn't impact the performance numbers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1567ab87fb1639298222d3bb383b8bb58f1184c

Comment 11

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

new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=270baab418490d988b5fe3784e912e0ff964db8d (sanitize on)

Comment 12

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

newer try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=053b3072c0db47207c85b1c9c529c65ce22094a6 - this time with builds!

Comment 13

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

There doesn't seem to be any noticable perf impact from this patch alone - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d6f849fde11757411d2f91b85f019cffdfc9038a&newProject=try&newRevision=3a9061c9137927044fb225b8deff016d7b8ca0ea

but it enables us to plug fluent-rs and perform the initial translation (or restoration from cache) without loading any JS.

Comment 14

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

Attachment: Bug 1503657 - Move Node::Localize to DocumentL10n::TranslateFragment. r?smaug

Depends on D26277

Comment 15

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

The latest patch is not yet fully tested, but it is the real reward I think. I was able to remove nsINode::Localize and all Fluent-related code from nsINode and shove it into DocumentL10n::TranslateFragment, where they belong.

This also makes TranslateFragment not have to go to JS at all!

Comment 16

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

This patchset is now ready to review.

There's one remaining bug that I need to dig into - the text-link on DOMOverlays elements with is="text-link" is not getting set (it was added in bug 1527495 via connectedCallback), but it shouldn't affect the patchset too much.

I'm generally really happy with how it shaped up. The last patch is not necessary, but cleans up a lot, and removes the workarounds from nsINode, leaving us with l10n code in DocumentL10n only, so I'm happy to push for landing it all together.

The rest of DOMLocalization is basically MutationObserver specific, and will go away with the next patchset that will migrate the observation logic to dom/l10n/MutationObserver or something similar.

Comment 17

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

Thank you all for the feedback.

I applied everything from the first patch (or responded to), and I'm going not to recreate the patchset to get rid of the annoying phabricator comment spam.

I'll also start with a patch that adds tests to separate it out (smaug's request), and stop at the first patch that creates a regression.

Comment 18

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

Attachment: Bug 1503657 - Land DOMOverlays tests prior to switch to C++. r?pike

Comment 19

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

Attachment: Bug 1503657 - Implement Fluent DOMOverlays in C++.

Depends on D27199

Comment 20

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

Attachment: Bug 1503657 - Migrate nsINode::localize and DOMLocalization.jsm to use DOMOverlays C++.

Depends on D27200

Comment 21

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

Olli - need a bit of your help here I think.

I applied all your feedback on namespaces from the abandoned revision, but I still see the CE connectedCallback regression.

I was able to create a minimized testcase which I attached in the first patch as test_domoverlays_bug.xul - it's a bit extracted from Preferences lazy loading of panels.

What happens is that the <label is="text-link"/> should trigger connectedCallback via [0], but after applying the third patch (switching DOMLocalization to use C++ API), it doesn't work.

What's surprising is that if instead I replace the JS with:

    let box = document.getElementById("template-paneGeneral");

    let frag = MozXULElement.parseXULToFragment(box.firstChild.data);

    box.replaceWith(frag);

    let label = document.getElementById("unsupportedLink");
    is(label.classList.contains("text-link"), true, "textLink conntectedCallback should have been called");

then it does work.

If I just skip localization, it also works:

    let frag = MozXULElement.parseXULToFragment(box.firstChild.data);

    document.l10n.pauseObserving();
    box.replaceWith(frag);
    document.l10n.resumeObserving();

    let label = document.getElementById("unsupportedLink");
    is(label.classList.contains("text-link"), true, "textLink conntectedCallback should have been called");

So both inserting, and localizing does work, but localizing prior to inserting makes it not work. I suspect that I do something to the namespaces still that messes up custom element, but I searched for couple hours scanning namespaces of every element in the fragment and its attributes and can't find any difference with and without the patch.

I don't know how to debug this further.

[0] https://searchfox.org/mozilla-central/source/toolkit/content/widgets/text.js#31

Comment 22

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

Okay, not sure why, but this fixed it:

--- a/dom/l10n/DOMOverlays.cpp
+++ b/dom/l10n/DOMOverlays.cpp
@@ -220,7 +220,7 @@ already_AddRefed<nsINode> DOMOverlays::G
   aTranslatedChild->GetAttr(kNameSpaceID_None, nsGkAtoms::datal10nname,
                             childName);
 
-  Element* sourceChild = nullptr;
+  RefPtr<Element> sourceChild = nullptr;
 
   nsINodeList* childNodes = aSourceElement->ChildNodes();
   for (uint32_t i = 0; i < childNodes->Length(); i++) {

Continuing to build up now!

Comment 23

[Boris Zbarsky [:bzbarsky]]

Okay, not sure why, but this fixed it:

Well... In the old code you stored a reference to the element in sourceChild, then the code does:

  aSourceElement->RemoveChild(*sourceChild, rv);

This can delete sourceChild, since now nothing is holding a reference to it, no? After that point, the fact that anything other than a crash happened is luck.

Comment 24

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

Yeah, makes sense!

I separated out bug 1544118 for the deeper refactor, and the patches in this bug should be now ready to be reviewed.

Comment 25

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

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa01b6c3b146ab726f685142d15881366271d0c

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

As explained on IRC, RemoveChild ends up dropping refcnt to 0. That doesn't delete the object but does call LastRelease, which drops internal slots - which among others keep custom element data alive.

Comment 27

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

Ok, I think this is now ready to be reviewed.

I asked Pike for reviews on the tests ported from fluent.js repo, and on the second patch to skim through how we match JS to C++.
I asked smaug for reviews on the C++ implementation and the refactor of nsINode::localize.

In bug 1544118 I'll get rid of nsINode::localize completely.

Here's a try:

For performance, I verified that talos numbers are in check, but I don't see any noticable perf difference yet. I expect that it likely comes from the fact that Preferences are lazy-loaded so about_preferences_basic talos test won't show many l10n related perf improvements.

I still expect to get some noticable perf wins from bug 1544118 tho.

Comment 28

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

Here's the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c2cbb4b889316bbe823f335828457ce5c67d974

Comment 29

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

I also ran some microbenchmark on:

{

let count = 10000;
let l10n = {
  value: `Click on <img data-l10n-name="picture"/> to go to <a data-l10n-name="link"/>the website</a>.`
};
let elems = [];
for(let i = 0; i < count; i++) {
  let elem = document.createElement("description");
  let frag = MozXULElement.parseXULToFragment(`<img src="logo.png" data-l10n-name="picture"/><a href="www.mozilla.org" data-l10n-name="link"></a>`);
  elem.appendChild(frag);
  elems.push(elem);
}
let start = performance.now();
for(let i = 0; i < count; i++) {
  DOMLocalization.jsTranslateElement(elems[i], l10n);
  //DOMOverlays.translateElement(elems[i], l10n);
}
let end = performance.now();
console.log(`time: ${end - start}`);
}

and got the following results (Carbon X1):

JS implementation: 602.32ms
C++ implementation: 159.8ms

Comment 30

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

talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=843ce8172bef5f383fc321e02fd8949bcb9f0272&newProject=try&newRevision=716d769dbaab3c2a212026f98ccfe2dac2f9652e

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8607ae75fc82
Land DOMOverlays tests prior to switch to C++. r=Pike
https://hg.mozilla.org/integration/autoland/rev/cae9cd8bbdea
Implement Fluent DOMOverlays in C++. r=smaug,Pike
https://hg.mozilla.org/integration/autoland/rev/87829648b0e5
Migrate nsINode::localize and DOMLocalization.jsm to use DOMOverlays C++. r=smaug

Comment 32

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8607ae75fc82
https://hg.mozilla.org/mozilla-central/rev/cae9cd8bbdea
https://hg.mozilla.org/mozilla-central/rev/87829648b0e5

Comment 33

[Johann Hofmann [:johannh]]

Attachment: Screenshot 2019-04-24 at 12.32.48.png

This broke lots of things in my local central build, for example about pages (see screenshot).

Gandalf, can you confirm? Should we back this out before it hits Nightly?

Comment 34

[Johann Hofmann [:johannh]]

Ah, nevermind, wasn't building with the latest artifacts... :/

Sorry for the noise.