Closed Bug 1480146 Opened 6 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Bug 1480146 - Add a cache for inline stylesheets without @import rules. r=heycam
47 bytes, text/x-phabricator-request
Details | Review
Bug 1480146 - Add memory reporting for the inline style cache.
47 bytes, text/x-phabricator-request
Details | Review 
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?
Priority: -- → P3
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.
(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.
Plus I do hope people use <link> instead of <style> for big sheets on Shadow DOM...
Might be worth adding a best practice somewhere to the web components docs about https://bugzilla.mozilla.org/show_bug.cgi?id=1480146#c3?
Keywords: dev-doc-needed
That'd be great indeed.

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

Assignee: nobody → emilio
Flags: needinfo?(emilio)
Depends on: 1571530

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.

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

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

Flags: needinfo?(emilio)

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

Pushed by ealvarez@mozilla.com:
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

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Flags: needinfo?(emilio)
Status: RESOLVED → REOPENED
status-firefox70: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla70 → ---
Attachment #9084713 - Attachment description: Bug 1480146 - Add a cache for inline stylesheets without @import rules. → Bug 1480146 - Add a cache for inline stylesheets without @import rules. r=heycam
Pushed by ealvarez@mozilla.com:
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
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
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
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/455228c96302
Talos tests for stylesheet caching. r=heycam

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

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

(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

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)?

Flags: needinfo?(alexandru.ionescu)

Yes, they are, Cameron.

Flags: needinfo?(alexandru.ionescu)
Regressions: 1574718

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!

Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1696447
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: