1480146 - Can we do some sort of sharing across <style> in different instantiations of a shadow DOM component?

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

https://wpt.fyi/results/ has a shadow DOM component with a <style> inside for each link.

If I load that page with shadow DOM disabled, I see in about:memory:

│   │  │   ├──0.29 MB (00.25%) ++ style-structs
│   │  │   ├──0.24 MB (00.20%) ++ computed-values
│   │  │   ├──0.05 MB (00.04%) ── style-sheets
│   │  │   ├──0.05 MB (00.04%) ++ style-sets

If I enable shadow DOM I see:

│      │  │   ├──0.37 MB (00.24%) ++ computed-values
│      │  │   ├──0.26 MB (00.17%) ++ style-structs
│      │  │   ├──0.09 MB (00.06%) ++ style-sets
│      │  │   ├──0.04 MB (00.02%) ── style-sheets

So maybe there isn't really an issue here after all, in that whatever they do to work around shadow DOM not being present is pretty bloaty too?

Comment 1

[Cameron McCormack (:heycam)]

Re-reading my bug 1410579 comment 0 I'm not sure if this is a dupe of that or not.

I can only see people using <style> in shadow trees for style encapsulation more and more, so I think we should consider doing something.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

(In reply to Cameron McCormack (:heycam) from comment #1)
> Re-reading my bug 1410579 comment 0 I'm not sure if this is a dupe of that
> or not.
> 
> I can only see people using <style> in shadow trees for style encapsulation
> more and more, so I think we should consider doing something.

Sharing on Shadow Trees is much easier than sharing across documents, fwiw, which is what that bug was about.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Plus I do hope people use <link> instead of <style> for big sheets on Shadow DOM...

Comment 4

[Chris Mills [:cmills]]

Might be worth adding a best practice somewhere to the web components docs about https://bugzilla.mozilla.org/show_bug.cgi?id=1480146#c3?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

That'd be great indeed.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

A bit of research shows that Blink and WebKit do have such a cache, and sites are probably slower than they need in Firefox because of it.

So I'll do something here...

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1480146 - Add a cache for inline stylesheets without @import rules. r=heycam

Supporting @import was much more annoying (I had a patch but it became much more
complex than this), and seems other browsers don't do it either:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/style_sheet_contents.cc?l=149&rcl=1999822baf4dc673088e65997fa07ad158f2e166
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/css/StyleSheetContents.cpp?rev=246490#L111

Also added a singleton perf test for <link> caching.

Maybe we should add some cache eviction here, but I'm not sure what'd be the
best policy here.

Comment 8

[Cameron McCormack (:heycam)]

Maybe we should evict entries that never get a second user, after some timer fires?

Comment 9

[Emilio Cobos Álvarez [:emilio]]

(In reply to Cameron McCormack (:heycam) from comment #8)

Maybe we should evict entries that never get a second user, after some timer fires?

I'll add some code to measure memory usage of this cache and if we see it show up we can think about eviction harder. A timer seems tricky, because it seems plausible for a page to create multiple components across time...

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1480146 - Add memory reporting for the inline style cache.

Comment 11

[Boris Zbarsky [:bzbarsky]]

You could use an expiration tracker, maybe. Though guessing its tick interval would be nontrivial.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f47314ec33ff
Add a cache for inline stylesheets without @import rules. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ff3ba7a16c21
Add memory reporting for the inline style cache. r=heycam

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f47314ec33ff
https://hg.mozilla.org/mozilla-central/rev/ff3ba7a16c21

Comment 14

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing talos perf reftest to timeout in link-style-cache-1.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/a232c8fd289f6a26bce04fdcf82e25e763098722

Push with failures:
(central, where it first appeared): https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&group_state=expanded&searchStr=%28ps%29%2Cwindows%2C7&fromchange=5d4cbfe103bbc517599231eb33d4f3ebbbcede40&selectedJob=262103626

(autoland): https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C7%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-windows7-32%2Fopt-talos-perf-reftest-singletons-e10s%2Ct%28ps%29&tochange=ff3ba7a16c214c01e270483ba42dc37e5bcd22f6&group_state=expanded&fromchange=a61a256c7eafbbbc11b57ed0d8f0e1a1e0144cae

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=262115079&repo=autoland&lineNumber=1236
[task 2019-08-17T00:56:41.568Z] 00:56:41 INFO - PID 4896 | Cycle 1(3): loaded http://127.0.0.1:49777/tests/perf-reftest-singletons/inline-style-cache-1.html (next: http://127.0.0.1:49777/tests/perf-reftest-singletons/link-style-cache-1.html)
[task 2019-08-17T00:56:46.822Z] 00:56:46 INFO - PID 4896 | Cycle 1(4): loaded http://127.0.0.1:49777/tests/perf-reftest-singletons/inline-style-cache-1.html (next: http://127.0.0.1:49777/tests/perf-reftest-singletons/link-style-cache-1.html)
[task 2019-08-17T00:57:17.374Z] 00:57:17 INFO - PID 4896 | __WARNTimeout (1/3) exceeded on http://127.0.0.1:49777/tests/perf-reftest-singletons/inline-style-cache-1.html__WARN
[task 2019-08-17T00:57:47.629Z] 00:57:47 INFO - PID 4896 | __WARNTimeout (2/3) exceeded on http://127.0.0.1:49777/tests/perf-reftest-singletons/inline-style-cache-1.html__WARN
[task 2019-08-17T00:58:17.880Z] 00:58:17 INFO - PID 4896 | __FAILTimeout in perf-reftest-singletons__FAIL
[task 2019-08-17T00:58:17.880Z] 00:58:17 INFO - PID 4896 | __FAILTimeout (3/3) exceeded on http://127.0.0.1:49777/tests/perf-reftest-singletons/inline-style-cache-1.html__FAIL
[task 2019-08-17T00:58:17.880Z] 00:58:17 INFO - PID 4896 | console.error: "You should not call finishTest without having first initted the Profiler"
[taskcluster:error] Aborting task...

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bcd5ea241cd
Add a cache for inline stylesheets without @import rules. r=heycam
https://hg.mozilla.org/integration/autoland/rev/8e3a8dd3189d
Add memory reporting for the inline style cache. r=heycam

Comment 16

[Andrei Ciure[:aciure]]

Backed out 2 changesets (bug 1480146) for causing inline-style-cache-1.html timeouts

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=262233537&fromchange=38459b4f9d63f583a89a618cf73a62cc7e777d31&searchStr=windows%2C7%2Cshippable%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-windows7-32-shippable%2Fopt-talos-perf-reftest-singletons-e10s%2Ct%28ps%29

backout: https://hg.mozilla.org/integration/autoland/rev/99fd896a682f5d4ce4dc1766f157d22eb6f1e94e

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7dea9580758
Add a cache for inline stylesheets without @import rules. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a0d4d643bce2
Add memory reporting for the inline style cache. r=heycam

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/455228c96302
Talos tests for stylesheet caching. r=heycam

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f7dea9580758
https://hg.mozilla.org/mozilla-central/rev/a0d4d643bce2
https://hg.mozilla.org/mozilla-central/rev/455228c96302

Comment 20

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Andrei Ciure[:andrei_ciure] from comment #16)

Backed out 2 changesets (bug 1480146) for causing inline-style-cache-1.html timeouts

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=262233537&fromchange=38459b4f9d63f583a89a618cf73a62cc7e777d31&searchStr=windows%2C7%2Cshippable%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-windows7-32-shippable%2Fopt-talos-perf-reftest-singletons-e10s%2Ct%28ps%29

backout: https://hg.mozilla.org/integration/autoland/rev/99fd896a682f5d4ce4dc1766f157d22eb6f1e94e

== Change summary for alert #22522 (as of Sun, 18 Aug 2019 19:12:45 GMT) ==

Improvements:

29% perf_reftest_singletons windows10-64-shippable opt e10s stylo 66.62 -> 47.58
28% perf_reftest_singletons windows10-64-shippable-qr opt e10s stylo 67.04 -> 48.43
27% perf_reftest_singletons linux64-shippable opt e10s stylo 63.13 -> 46.05
27% perf_reftest_singletons linux64-shippable-qr opt e10s stylo 65.99 -> 48.35

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=22522

Comment 21

[Cameron McCormack (:heycam)]

Alexandru, are these performance improvements as a result of backing out the initial landing? Were there corresponding regressions in those tests when they initially landed (and when they re-landed)?

Comment 22

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Yes, they are, Cameron.

Comment 23

[Chris Mills [:cmills]]

I've documented this one; see https://github.com/mdn/sprints/issues/2117#issuecomment-530808053 for the full details.

I'm not 100% sure my wording is correct, so a review would be really appreciated, thanks!