1367863 - stylo: figure out how to mitigate the memory hit from disabling Gecko style context sharing

Status: RESOLVED DUPLICATE of bug 1367904

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Stylo disables Gecko style context sharing.

On the Obama wikipedia page that means an extra 20k style context allocations (see numbers in bug 1367862 comment 0). Each style context is about 20 words, so figure 160 bytes on 64-bit and 80 bytes on 32-bit.  That means 3.2MB of RAM overhead on 64-bit and half that on 32-bit.

Once we move to "thin" style contexts this will be much less of a problem, but that relies on removing the Gecko style system, which is not happening yet.  So we might want a plan for mitigating the memory impact here.

Comment 1

[Bobby Holley (:bholley)]

How about moving earlier to "thin" style contexts by using subclasses? We can have GeckoStyleContext and ServoStyleContext, both of which inherit nsStyleContext, and the style struct pointers and tree state can live in GeckoStyleContext. The style struct getters can just cast on the Gecko path, which should make the indirection free.

That would shrink style contexts down to about 6 words, which is a substantial mitigation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, that seems like a good idea.  So ServoStyleContext would have:

  mParent, mStyleIfVisited, mPseudoTag, mSource, mBits, mRefCnt

which is indeed 6 words.

Comment 3

[Cameron McCormack (:heycam)]

If we really wanted to squeeze it, we could get rid of mPseudoTag, and start storing an enum value that can represent all of the pseudo-element and anonymous box types.  I think there is plenty of space in mBits to replace the pseudo enum with this broader enum.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I believe the set of allowed tree pseudos is completely open-ended right now.  We could change that, of course...

Comment 5

[Cameron McCormack (:heycam)]

Ugh, forgot about those.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #2)
> Yes, that seems like a good idea.  So ServoStyleContext would have:
> 
>   mParent, mStyleIfVisited, mPseudoTag, mSource, mBits, mRefCnt
> 
> which is indeed 6 words.

Why do we need mParent?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Also we don't need mStyleIfVisited AFAICT, because that's stored in the ComputedValues now with jryans' patch. Also it'd be nice to move mBits to ComputedValues, we're going to need something similar for something like https://github.com/servo/servo/issues/16825

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Why do we need mParent?

Because we still have GetParentAllowServo() consumers.  Once we get rid of them, we could kill off mParent.

> Also we don't need mStyleIfVisited AFAICT, because that's stored in the ComputedValues now with jryans' patch. 

That needs more work, then, because right now jryans' code in fact synthesizes the mStyleIfVisited.  We would need to change the implementation of GetVisitedDependentStyle to not use mStyleIfVisited in the servo case.  Doable, of course.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The other option is to add a backpointer from a ComputedValues to a corresponding nsStyleContext.  We'd have to be a bit careful to make sure the nsStyleContext can't die before the ComputedValues or something...  Then we could share on the Gecko side inasmuch as we're sharing on the servo side.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) (if a patch has no decent message, automatic r-) from comment #9)
> The other option is to add a backpointer from a ComputedValues to a
> corresponding nsStyleContext.  We'd have to be a bit careful to make sure
> the nsStyleContext can't die before the ComputedValues or something...  Then
> we could share on the Gecko side inasmuch as we're sharing on the servo side.

That's a nice idea.

We would just have a weak AtomicPtr<nStyleContext> inside of ComputedValues. Then, in NS_NewStyleContext, we check whether there's already a backpointer for the given ComputedValues, and if so, assert that the parents match and then use that. Otherwise, we mint a new one and stash the pointer in the ComputedValues. The nsStyleContext destructor can then null out the backpointer, just to be safe.

I think we should do all the other optimizations we've discussed in these bugs first (especially thin style contexts), and then be clear about which situations we actually need the backpointer for, and whether the savings outweigh the cost of the additional word in ComputedValues.

Comment 11

[Bobby Holley (:bholley)]

Note that in particular, I'd like to eventually merge nsStyleContext with ServoComputedValues, so the problem of duplicated nsStyleContexts for a single ServoComputedValues will be a temporary problem. That's why I want to be clear about what we're gaining before doing it.

Comment 12

[Bobby Holley (:bholley)]

Bug 1367904 should solve this.