Open
Bug 1400915
Opened 7 years ago
Updated 2 years ago
stylo: Still have a lot of Display, Position, Border structs on the HTML5 page even with STYLO_THREADS=1
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
NEW
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
Per the about:memory output I just posted in bug 1397380, STYLO_THREADS=1 on the HTML5 page has, under servo-style-structs
2.46 MB (16.10%) ── Display
1.13 MB (07.41%) ── Position
0.85 MB (05.55%) ── Border
The Gecko numbers for the no-stylo output are:
0.86 MB (-5.65%) ── Display
0.21 MB (-1.34%) ── Position
0.02 MB (-0.13%) ── Border
The other reset structs are also much bigger on the stylo side, but those are the ones that are biggest in absolute terms.
Is that still expected with our TLS sharing patches?
Reporter | ||
Updated•7 years ago
|
Summary: stylo: Still have a lot of Display, Position, Border structs on the HTML5 page event with STYLO_THREADS=1 → stylo: Still have a lot of Display, Position, Border structs on the HTML5 page even with STYLO_THREADS=1
Comment 1•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #0)
> Per the about:memory output I just posted in bug 1397380, STYLO_THREADS=1 on
> the HTML5 page has, under servo-style-structs
>
> 2.46 MB (16.10%) ── Display
> 1.13 MB (07.41%) ── Position
> 0.85 MB (05.55%) ── Border
>
> The Gecko numbers for the no-stylo output are:
>
> 0.86 MB (-5.65%) ── Display
> 0.21 MB (-1.34%) ── Position
> 0.02 MB (-0.13%) ── Border
>
> The other reset structs are also much bigger on the stylo side, but those
> are the ones that are biggest in absolute terms.
>
> Is that still expected with our TLS sharing patches?
I do know that, for STYLO_THREADS=1, the two-tiered cache (effectively a larger cache) made a big difference on the html5 spec. I decided not to land that though, since it didn't seem to move the needle on AWSY and the aforementioned testing conditions aren't very realistic.
That said, it would be good to verify that the cache is working properly. Is there some eviction parameter we can tune to see if the difference goes away with a larger cache?
Flags: needinfo?(emilio)
Flags: needinfo?(cam)
Priority: -- → P3
Comment 2•7 years ago
|
||
The TLS struct cache doesn't evict anything during a traversal. It's only cleared at the end of a traversal.
The rule node cache patches I had added a few additional conditions beyond font-size/WritingMode that helped with some of the struct overhead that came from lack of some anonymous box sharing. I can try adding those to the TLS cache.
Flags: needinfo?(cam)
Comment 3•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #2)
> The TLS struct cache doesn't evict anything during a traversal. It's only
> cleared at the end of a traversal.
>
> The rule node cache patches I had added a few additional conditions beyond
> font-size/WritingMode that helped with some of the struct overhead that came
> from lack of some anonymous box sharing. I can try adding those to the TLS
> cache.
OK sounds good - probably best to postpone that to next week though.
Comment 4•7 years ago
|
||
Yeah, also note that we don't share per struct, but per whole ComputedValues, which can affect sharing negatively.
Flags: needinfo?(emilio)
Reporter | ||
Comment 5•7 years ago
|
||
Oh, I didn't realize that. I thought we had sharing per-struct, because we already had per-ComputedValues sharing via the existing sharing mechanism...
Comment 6•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #5)
> Oh, I didn't realize that. I thought we had sharing per-struct, because we
> already had per-ComputedValues sharing via the existing sharing mechanism...
Nope, we share all reset structs at the same time or none.
I suspect this may actually explain this, but I haven't investigated it.
Comment 7•7 years ago
|
||
That's a great point. I did see some increase in sharing in my patches when I made it share per struct. So that definitely sounds like worth trying, in addition to the extra conditions.
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•