1318446 - Benchmark what caching StyleContext on elements

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P1)

Description

[Andrew Overholt [:overholt]]

My understanding is that l20n impacts startup because its goal is to do work before layout (presumably to avoid rework there?) but XUL doesn't have a layout tree at that point and therefore no access to StyleContexts. It is also my understanding that Stylo will have a StyleContext cache on each element.

What we'd like to do is have some sort of hacky patch to Gecko to create a StyleContext cache like Stylo will have to get a rough idea of the performance improvement l20n will get from this.

(I'm quite confident I'll wish I could edit this description because it'll show how little I know about this :)

Comment 1

[Andrew Overholt [:overholt]]

Olli, Bobby, William: can you help correct me here and give a rough estimate of how hard this would be? It'll help the l20n team prioritize their optimization work. Thanks!

Comment 2

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

So since this would be for testing only, I think it might reveal something useful to have (for this testing) a method in XULDocument to cache style context on each element and after that run the l20n code, and compare that to non-patched build.

Also, need to calculate how much time running the "cache style context" takes.

Comment 3

[Axel Hecht [:Pike]]

Some additional context for this experiment:

The best base-revision would be https://hg.mozilla.org/mozilla-central/rev/51750761f2c6. That's our anchor for https://pike.github.io/try-pushes/?user=[e-mail]&startdate=2016-11-16%2020:40&enddate=2016-11-17, i.e., we could perf test this experiment, and then merge against larch and each of the experiments in gandalf's tree.

Comment 4

[Bobby Holley (:bholley)]

I'll defer to smaug on the difficulty of the prototype. Once we have stylo, it should be relatively straightforward - we might even get it for free, depending on how things shake out.

Comment 5

[Axel Hecht [:Pike]]

I went from XULDocument to XULElement, and figured, I might be able to do something horrible. So I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=15857692283095dd7c4ff6bd0c4256a118befddf.

It hangs/crashes on shutdown, but it did start "OK", so I pushed it to try and we'll see if talos shows good numbers for just central+patch. If so, I'll push out the larch leafs.

Comment 6

[Axel Hecht [:Pike]]

The XULElement thing blew up badly on talos, the shutdown hangs break the tests. Trying document now, just for an hour or so.

Comment 7

[Axel Hecht [:Pike]]

Attachment: failed attempt on nsDocument

So this attempt also failed to shut down cleanly. Assertions at runtime are this:

[Parent 71226] ###!!! ASSERTION: must be in the same rule tree as parent: 'r1 == r2', file /src/l10n/mozilla-central/layout/style/nsStyleContext.cpp, line 127
[Parent 71226] ###!!! ASSERTION: Null pres shell: 'mShell', file /src/l10n/mozilla-central/layout/base/nsPresContext.h, line 180

Attaching it here so that others can try something different :-).

smaug, can you help out here next week?

Comment 8

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

I wouldn't expect this kind of test to pass anything on try. Just measure the stuff Gandalf has been testing locally.

And the patch doesn't do what I suggested, it doesn't help with the case being slow.
You want some explicit "create style context for all xul elements" method on XULDocument or so and after that do the l20n stuff.

Comment 9

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

Gandalf, do you have the perf testing patches available somewhere? Want to upload them here?

Comment 10

[Axel Hecht [:Pike]]

See my comment 3. I had a bug in that code that showed a lot more than it should, https://pike.github.io/try-pushes/?user=[e-mail]&startdate=2016-11-16%2020:40&enddate=2016-11-17 now just shows 

- central
- larch
- larch, but just one localized string(not important)
- larch, but with mocked code for l20n (https://hg.mozilla.org/try/rev/c0ac2b41e254882bacebc8fccebefce41b28b6bb)
- larch, with mocked code for all localizable elements (https://hg.mozilla.org/try/rev/2c254c68f1920711edee305babb6f55428415299)

Interesting is central against larch, and central against mocked 100, and mocked all.

That way, we can compare the difference between the real larch tree and the mock, and how the mock scales in number of elements.

Comment 11

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

Oh, wait, I do have Gandalf's perf testing patches on this machine.

Comment 12

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

Attachment: test patch

For some reason this doesn't help much, even when measuring the case when the caching is outside measurement. Need to reprofile.
(oh, and I changed the l20n.js to use MozBeforeLayout for this case)

  getTranslatables(element) {
    document.cacheStyleContext();
    let start = performance.now();
    dump("getTranslatables2 " + element + "\n");
    const nodes = Array.from(element.querySelectorAll(this.query));
    let end = performance.now();
    dump(`qSA: ${end - start}` + "\n");
    dump(nodes.length + "\n");
    document.clearStyleContextCache();

Comment 13

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

well, it does help 66% in the slow case (MozBeforeLayout), but that is still nowhere near the fast case (MozAfterInitialLayout).

Comment 14

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

And the issue seems to be that wrapping the XUL elements still needs to run bunch of JS from XBL.

Sounds like for XUL case, if we want to keep the l20n setup, we need to add some new API to localize using C++.

Comment 15

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

So, I wrote a minimized version of the l20n testcase that just translates 221 menuitems.

Here's the code that you can launch in browser.xul and test one of the 4 timing strategies and cache/nocache: https://pastebin.mozilla.org/8948755

Here are my results for windows8:

(m-c: 283)
+------------------------+----------+-----------+
| tpaint(e10s)           | no cache | cache     |
+------------------------+----------+-----------+
| MozBeforeLayout        | 318      | 391       |
| MozAfterInitialLayout  | 307      | 332       |
| documentReady          | 309      | 326       |
| requestIdleCallback    | 285      | 284       |
+------------------------+----------+-----------+

(m-c 903)
+---------------------------+----------+-------+
| ts_paint (e10s)           | no cache | cache |
+---------------------------+----------+-------+
| MozBeforeLayout           |      931 |  1010 |
| MozAfterInitialLayout     |      925 |   941 |
| documentReady             |      904 |   903 |
| requestIdleCallback       |      904 |   902 |
+---------------------------+----------+-------+

So, on this test, I failed to get the cache to produce any win any scenario :( Not sure if it's because of the difference between full l20n.js vs. this minimized testcase or what.

The other interesting outcome is that for this testcase documentReady and requestIdleCallback give me no penalty vs m-c on ts_paint and requestIdleCallback gives me no penalty vs. m-c on tpaint.

If you want to dig into the results for all platforms:

+-----------------------+------------------------+------------------------+
|                       |        no cache        |         cache          |
+-----------------------+------------------------+------------------------+
| MozBeforeLayout       | https://mzl.la/2gN2qCI | https://mzl.la/2fN0bj1 |
| MozAfterInitialLayout | https://mzl.la/2fYRTmD | https://mzl.la/2gOVMsX |
| documentReady         | https://mzl.la/2gN1rSS | https://mzl.la/2fN0f2f |
| requestIdleCallback   | https://mzl.la/2gfhhSE | https://mzl.la/2gGxQaI |
+-----------------------+------------------------+------------------------+

Comment 16

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

Updated code, sorry: https://pastebin.mozilla.org/8948757

And here's the patch for Gecko that adds the cache and MozBeforeLayout and MozAfterInitialLayout: https://pastebin.mozilla.org/8948759

So, STR:

1) Build gecko with this patch

2) Add test.js to browser.xul with the code from https://pastebin.mozilla.org/8948757

3) Pick cache/nocache and pick at which point to trigger the translation

4) Launch talos

Comment 17

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

Using requestIdleCallback feels wrong here. Those callbacks may get run way after the window is up and running and shown to the user.

Comment 18

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

FWIW, I'm leaning on to what wchen said, that C++ API is needed here. Touching anything which has XBL binding will be slow.

(And I think using Custom Elements + Shadow DOM wouldn't make this any better, probably worse, since it forces effectively synchronous custom element constructor run.)

What would the API need to do? Go through elements at some point and replace some data with localized data?

Comment 19

[Axel Hecht [:Pike]]

We're taking the L20n technology, and we'll talk about an implementation in C++ of that.

There might also be parts in JS.

If those parts in JS need APIs, and which, is something we should discuss once have the high-level architecture together.

The data gathered here in this bug has been crucial to actually be able to make that call, so thanks a lot for that.

As for this bug, I think we're good to resolve this WORKSFORME, as we have the data we need to make further decisions.

Comment 20

[(no longer active)]

Perhaps next week in Hawaii we can come up with a rough sketch of the implementation?

Comment 21

[Axel Hecht [:Pike]]

Yes, I think that should be our intent.