Implement Fluent DOMOverlays in C++

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
26 days ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 2 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

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

Updated

7 months ago
Blocks: 1441035
Component: Internationalization → DOM
Priority: -- → P3
Assignee

Comment 2

6 months ago
This is just a scaffolding
Assignee

Comment 4

3 months ago

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.

Assignee

Updated

2 months ago
Blocks: 1507008
Component: DOM → DOM: Core & HTML
Product: Core → Core
Assignee

Comment 5

2 months ago

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?

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika) → needinfo?(bobbyholley)

(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

Flags: needinfo?(bobbyholley)
Assignee

Comment 8

2 months ago

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

Attachment #9028779 - Attachment is obsolete: true

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

Assignee: nobody → gandalf
Status: NEW → ASSIGNED

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.

Updated

a month ago
See Also: → 1543493

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!

Attachment #9057439 - Attachment description: Bug 1503657 - Refactor Node::Localize to use DocumentL10n directly. → Bug 1503657 - Move Node::Localize to DocumentL10n::TranslateFragment. r?smaug

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.

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.

Attachment #9047258 - Attachment is obsolete: true
Attachment #9056061 - Attachment is obsolete: true
Attachment #9057439 - Attachment is obsolete: true

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

Flags: needinfo?(bugs)

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!

Flags: needinfo?(bugs)

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.

Assignee

Updated

a month ago
Blocks: 1544118

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.

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.

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.

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 31

27 days ago
Pushed by zbraniecki@mozilla.com:
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

27 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 27 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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?

Flags: needinfo?(gandalf)

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

Sorry for the noise.

Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.