1400915 - stylo: Still have a lot of Display, Position, Border structs on the HTML5 page even with STYLO_THREADS=1

Status: NEW

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Bobby Holley (:bholley)]

(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

[Emilio Cobos Álvarez (:emilio)]

Yeah, also note that we don't share per struct, but per whole ComputedValues, which can affect sharing negatively.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Emilio Cobos Álvarez (:emilio)]

(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

[Cameron McCormack (:heycam)]

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.