1203766 - cache style contexts resolved by nsComputedDOMStyle for display:none elements

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In bug 1200028 we found that a page could end up doing a lot of style context resolution when looking at computed style objects for display:none elements.  We can cache the style context we resolve (until it becomes invalid, due to a style flush) so we can re-use it each time we inspect a property on the object.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04fbd39d3cd6

Comment 1

[Cameron McCormack (:heycam)]

There are a bunch of assertions in that try run that need looking at.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 1: Generate nsPresArena::ObjectIDs with a preprocessor-included file.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add an ArenaRefPtr class that can have its pointer cleared out when an object's owning nsPresArena goes away.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add ArenaRefPtr support to nsStyleContext.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add a "restyle generation" counter, which increments whenever we process pending restyles.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Part 5: Rename nsComputedDOMStyle::mStyleContextHolder to mStyleContext.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Part 6: Cache resolved style contexts on nsComputedDOMStyle to avoid re-resolving if styles haven't changed.

Comment 8

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ff05624bca

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #3)
> Created attachment 8659775 [details] [diff] [review]
> Part 2: Add an ArenaRefPtr class that can have its pointer cleared out when
> an object's owning nsPresArena goes away.

If there's a good way to avoid the duplication in those assign() methods let me know...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659774 [details] [diff] [review]
Part 1: Generate nsPresArena::ObjectIDs with a preprocessor-included file.

So this is killing off First_nsStyleStruct_id, DummyBeforeStyleStructs_id, DummyAfterStyleStructs_id, Last_nsStyleStruct_id, right?

I guess that's ok if those aren't used anywhere...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659775 [details] [diff] [review]
Part 2: Add an ArenaRefPtr class that can have its pointer cleared out when an object's owning nsPresArena goes away.

>+   * Clears the pointer to the arena-allocated object but skips the usual
>+   * stop of deregistering the ArenaRefPtr from the nsPresArena.  This

"step", not "stop".

>+  void assign(already_AddRefed<I>& aSmartPtr)
>+  void assign(already_AddRefed<I>&& aSmartPtr)
>+  void assign(T* aPtr)

There's some nasty code duplication here.  Can you factor it out into a private method with this signature:

  template<typename I>
  void assignFromRawPtr(I* aPtr)

?  Seems like it would work to me.

>+#define PRES_ARENA_OBJECT(name_)                                              \
>+    case name_##_id:                                                          \
>+      /* do nothing */                                                        \

Really?  Seems to me like if someone uses ArenaRefPtr on something that's not PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR, we should assert.  At least here, though better somewhere earlier... (e.g. the ArenaRefPtr constructor might be good, since it knows the object id).

>+#define PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR(name_) \
>+  name_##_id,

You could also have nsPresArenaObjectList.h define PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR to PRES_ARENA_OBJECT if not otherwise defined... either way.

>+    MOZ_ASSERT(T::ArenaObjectID() == aObjectID);

Then why exactly do we need the aObjectID argument at all?

>+  //   (a) before mFrameArena's constructor runs so that our

s/constructor/destructor/?

r=me with those fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659776 [details] [diff] [review]
Part 3: Add ArenaRefPtr support to nsStyleContext.

>+   * This is called when we reconstruct the rule tree so that style contexts
>+   * aren't released afterwards, triggering an assertion in ~nsStyleContext.

"... so that style contexts pointing into the old ruletree aren't released afterwards ..."

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659778 [details] [diff] [review]
Part 4: Add a "restyle generation" counter, which increments whenever we process pending restyles.

You probably want a 64-bit counter, to avoid overflow issues.

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659779 [details] [diff] [review]
Part 5: Rename nsComputedDOMStyle::mStyleContextHolder to mStyleContext.

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8659780 [details] [diff] [review]
Part 6: Cache resolved style contexts on nsComputedDOMStyle to avoid re-resolving if styles haven't changed.

>+    mStyleContextGeneration = currentGeneration;

Don't you need to reget the generation?  GetStyleContextForElement flushes style, so a restyle could have happened since currentGeneration was set.

For that matter, don't we need to avoid using our cached mStyleContext if we have _pending_ restyles but haven't processed them yet?

I think there's also a problem if an element is added to the DOM and gets a frame.  This won't update the generation counter in the restyle manager, but it does mean that we need to not use the cached mStyleContext and instead get one from the frame.  I believe that checking for pending restyles in the "needs style flush" sense would also handle this case...

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Comment on attachment 8659775 [details] [diff] [review]
> Part 2: Add an ArenaRefPtr class that can have its pointer cleared out when
> an object's owning nsPresArena goes away.
...
> >+  void assign(already_AddRefed<I>& aSmartPtr)
> >+  void assign(already_AddRefed<I>&& aSmartPtr)
> >+  void assign(T* aPtr)
> 
> There's some nasty code duplication here.  Can you factor it out into a
> private method with this signature:
> 
>   template<typename I>
>   void assignFromRawPtr(I* aPtr)
> 
> ?  Seems like it would work to me.

I managed to do it with:

  template<typename I>
  void assign(already_AddRefed<I>& aSmartPtr)
  {
    nsRefPtr<T> newPtr(aSmartPtr);
    assignFrom(newPtr);
  }

  template<typename I>
  void assign(already_AddRefed<I>&& aSmartPtr)
  {
    nsRefPtr<T> newPtr(aSmartPtr);
    assignFrom(newPtr);
  }

  void assign(T* aPtr) { assignFrom(aPtr); }

  template<typename I>
  void assignFrom(I& aPtr)
  {
    ...
  }

> >+#define PRES_ARENA_OBJECT(name_)                                              \
> >+    case name_##_id:                                                          \
> >+      /* do nothing */                                                        \
> 
> Really?  Seems to me like if someone uses ArenaRefPtr on something that's
> not PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR, we should assert.  At least
> here, though better somewhere earlier... (e.g. the ArenaRefPtr constructor
> might be good, since it knows the object id).

Great idea.  (I need to split out the ObjectID enum into a separate header for this though.)

> >+#define PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR(name_) \
> >+  name_##_id,
> 
> You could also have nsPresArenaObjectList.h define
> PRES_ARENA_OBJECT_SUPPORTS_ARENAREFPTR to PRES_ARENA_OBJECT if not otherwise
> defined... either way.

Hmm, I guess I should really make it follow the pattern in nsCSSPropList.h, where CSS_PROP can be used to capture all properties or individual ones can be selected with CSS_PROP_FONT etc.  I'll make PRES_ARENA_OBJECT capture all objects and PRES_ARENA_OBJECT_{WITH,WITHOUT}_ARENAREFPTR_SUPPORT for selecting just those.

> >+    MOZ_ASSERT(T::ArenaObjectID() == aObjectID);
> 
> Then why exactly do we need the aObjectID argument at all?

It was because I was trying to avoid needing to #include the declaration of T (nsStyleContext) here, but I'll see if I can get rid of it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

>  template<typename I>
>  void assignFrom(I& aPtr)

Hmm.  Why does this need templating at all, come to think of it?  The callers all have a T* or nsRefPtr<T>, right?  So can't assignFrom just take T*?

Comment 18

[Cameron McCormack (:heycam)]

I think it helps avoid an extra AddRef/Release pair, if we pass in a reference to the nsRefPtr<T> local variable in the two assign() methods, by calling Move() on the argument.  This is what I have currently:

  template<typename I>
  void assign(already_AddRefed<I>& aSmartPtr)
  {
    nsRefPtr<T> newPtr(aSmartPtr);
    assignFrom(newPtr);
  }

  template<typename I>
  void assign(already_AddRefed<I>&& aSmartPtr)
  {
    nsRefPtr<T> newPtr(aSmartPtr);
    assignFrom(newPtr);
  }

  void assign(T* aPtr) { assignFrom(aPtr); }

  template<typename I>
  void assignFrom(I& aPtr)
  {
    if (aPtr == mPtr) {
      return;
    }
    bool sameArena = mPtr && aPtr && mPtr->Arena() == aPtr->Arena();
    if (mPtr && !sameArena) {
      MOZ_ASSERT(mPtr->Arena());
      mPtr->Arena()->DeregisterArenaRefPtr(this);
    }
    mPtr = Move(aPtr);
    if (mPtr && !sameArena) {
      MOZ_ASSERT(mPtr->Arena());
      mPtr->Arena()->RegisterArenaRefPtr(this);
    }
  }

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Comment on attachment 8659780 [details] [diff] [review]
> Part 6: Cache resolved style contexts on nsComputedDOMStyle to avoid
> re-resolving if styles haven't changed.
> 
> >+    mStyleContextGeneration = currentGeneration;
> 
> Don't you need to reget the generation?  GetStyleContextForElement flushes
> style, so a restyle could have happened since currentGeneration was set.

I guess we should, although since we already flushed style just before we first assigned to currentGeneration, and it doesn't look like anything between that and the GetStyleContextForElement call could post restyles, I'm not sure it'll ever be different.  Probably better to do it just in case something do get added in the middle of the function that can cause a restyle.

The comment above the flush in UpdateCurrentStyleSources mentions that the pres shell might change -- is that something to worry about on the second GetRestyleGeneration call too?

> For that matter, don't we need to avoid using our cached mStyleContext if we
> have _pending_ restyles but haven't processed them yet?

Given the flush towards the top of UpdateCurrentStyleSources, I don't think we need to check for pending restyles, since that would have processed them, wouldn't it?

> I think there's also a problem if an element is added to the DOM and gets a
> frame.  This won't update the generation counter in the restyle manager, but
> it does mean that we need to not use the cached mStyleContext and instead
> get one from the frame.  I believe that checking for pending restyles in the
> "needs style flush" sense would also handle this case...

Hmm, which "needs style flush" state would we be checking here?  If RestyleManager::HasPendingRestyles, what would actually cause a restyle to be posted after the content insertion (assuming we don't fall into a case in RestyleManager::RestyleForInsertOrChange that does post one)?

Comment 20

[Cameron McCormack (:heycam)]

And just to be clear, the case you're worried about it something like this, yes?

  var e = document.createElement("span");
  var cs = window.getComputedStyle(e);
  cs.length;  // cache the style context
  document.body.appendChild(e);
  cs.length;  // should discard the cached s.c., but restyle generation counter is the same

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I think it helps avoid an extra AddRef/Release pair

I guess so... Not sure that's worth the codesize increase if we had a bunch of instantiations, but since we only have one consumer it's not that big a deal.

> although since we already flushed style just before we first assigned to currentGeneration

Ah, so we did.  Maybe we don't need to worry about this...

> is that something to worry about on the second GetRestyleGeneration call too?

If we could actually have a non-no-op flush under GetStyleContextForElement, then yes.  Which is why I really hope we can't have that.

> Given the flush towards the top of UpdateCurrentStyleSources, I don't think we need to
> check for pending restyles

Yes, agreed.  We should have comments next to that flush explaining all the things that depend on it.

> Hmm, which "needs style flush" state would we be checking here?

mNeedStyleFlush on the document.  But it's not going to be an issue, given the FlisPendingNotifications call at the start of UpdateCurrentStyleSources.  That will trigger frame construction and then we will ignore our cached style context, right?

Though actually, if it's display:none in the new location, we could still have a problem in that its computed style should have changed, but it still has no frame, so we'd use our cached computed style.

For that matter, we may not need to append to the document tree at all; appending to a disconnected subtree can still change computed styles, right?

> And just to be clear, the case you're worried about it something like this, yes?

Yes.

Comment 22

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > I think it helps avoid an extra AddRef/Release pair
> 
> I guess so... Not sure that's worth the codesize increase if we had a bunch
> of instantiations, but since we only have one consumer it's not that big a
> deal.

True, although you'd hope that the linker/optimizer could fold them together.

> > although since we already flushed style just before we first assigned to currentGeneration
> 
> Ah, so we did.  Maybe we don't need to worry about this...
> 
> > is that something to worry about on the second GetRestyleGeneration call too?
> 
> If we could actually have a non-no-op flush under GetStyleContextForElement,
> then yes.  Which is why I really hope we can't have that.

OK.  I'm going to just add an assertion in here then that the restyle generation value is the same just after the GetStyleContextForElement call.

> > Given the flush towards the top of UpdateCurrentStyleSources, I don't think we need to
> > check for pending restyles
> 
> Yes, agreed.  We should have comments next to that flush explaining all the
> things that depend on it.

I'll add a comment saying that although GetStyleContextForElement also flushes, it allows us not to re-get the restyle generation.

> > Hmm, which "needs style flush" state would we be checking here?
> 
> mNeedStyleFlush on the document.  But it's not going to be an issue, given
> the FlisPendingNotifications call at the start of UpdateCurrentStyleSources.
> That will trigger frame construction and then we will ignore our cached
> style context, right?

Well, frame construction doesn't tick the restyle generation counter over, so maybe not?

> Though actually, if it's display:none in the new location, we could still
> have a problem in that its computed style should have changed, but it still
> has no frame, so we'd use our cached computed style.
> 
> For that matter, we may not need to append to the document tree at all;
> appending to a disconnected subtree can still change computed styles, right?

OK.  And another case is if we remove a display:none element from the document, which can change its styles and thus should also discard the cached style context.

So we could make RestyleManager::RestyleFor{InsertOrChange,Remove,Append} tick the restyle generation counter over, although in a lot of cases that seems like it could discard the cached style context when it doesn't need to.  Or we could rework this to store the ArenaRefPtr<nsStyleContext> on the Element somewhere (in a property?) so it could be cleared in BindToTree/UnbindFromTree and also when its frame is reconstructed.

Any better option?

Comment 23

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #22)
> So we could make RestyleManager::RestyleFor{InsertOrChange,Remove,Append}
> tick the restyle generation counter over, although in a lot of cases that
> seems like it could discard the cached style context when it doesn't need
> to.  Or we could rework this to store the ArenaRefPtr<nsStyleContext> on the
> Element somewhere (in a property?) so it could be cleared in
> BindToTree/UnbindFromTree and also when its frame is reconstructed.

I think incrementing the generation in BindToTree/UnbindToTree is sufficient, and simpler than the alternative, so I'm trying that.

Comment 24

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47da19ef1332

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add a "computed DOM style generation" counter, which increments whenever we process pending restyles or bind/unbind elements. (v2)

Renamed it to "computed DOM style generation" and incremented it from BindToTree/UnbindFromTree.

Comment 26

[Cameron McCormack (:heycam)]

Attachment: Part 6: Cache resolved style contexts on nsComputedDOMStyle to avoid re-resolving if styles haven't changed. (v2)

Just added a comment and assertion.

Comment 27

[Cameron McCormack (:heycam)]

Attachment: Part 7: Test.

Comment 28

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/try/pushloghtml?changeset=baea47f04d14 (which hopefully compiles this time)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8661070 [details] [diff] [review]
Part 4: Add a "computed DOM style generation" counter, which increments whenever we process pending restyles or bind/unbind elements. (v2)

I guess that works, but I worry about the performance in bind/unbind.  

Would it make sense to have computed style observe ParentChainChanged on the node it's targeting, or ask presshell to do so?  We really only need one observer per presshell, since it would just bump the generation counter, or we could have the computed style just clear its point (and un-observe, I guess) and not bump the generation counter at all.  Thoughts?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8661071 [details] [diff] [review]
Part 6: Cache resolved style contexts on nsComputedDOMStyle to avoid re-resolving if styles haven't changed. (v2)

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8661072 [details] [diff] [review]
Part 7: Test.

I'd like to see tests here for the following as well:

1)  Element going from not in document to in document and display:none
2)  Element going from not in document to in document and child of display:none
    element.
3)  Element being added to a disconnected subtree.

and all of those also with some ancestor of the element actually being the thing appended/inserted/removed.

r=me with that.

Comment 32

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Would it make sense to have computed style observe ParentChainChanged on the
> node it's targeting, or ask presshell to do so?  We really only need one
> observer per presshell, since it would just bump the generation counter, or
> we could have the computed style just clear its point (and un-observe, I
> guess) and not bump the generation counter at all.  Thoughts?

That sounds better than calling on every BindToTree/UnbindFromTree.  I think I prefer the latter option, having each nsComputedDOMStyle register itself as an observer, since it's about the same complexity as having a single one registered by PresShell, and will avoid invalidating unrelated nsComputedDOMStyles.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yeah, the big problem there is that we can get expensive notifications if someone calls getComputedStyle lots of times on a single node and then moves it around... I guess we can live with that, though; I don't expect it to be common.

Comment 34

[Cameron McCormack (:heycam)]

Yes, I see.  As you say, shouldn't be common.

So I will go back to the original part 4 (where it was named the "restyle generation") and attach a new patch for the observer stuff.

Comment 35

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6923b0458879

Comment 36

[Cameron McCormack (:heycam)]

Attachment: Part 6.1: Clear cached style context on nsComputedDOMStyle when its element is moved.

Comment 37

[Cameron McCormack (:heycam)]

Additional tests here: https://hg.mozilla.org/try/rev/400e6f0f5303

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8661601 [details] [diff] [review]
Part 6.1: Clear cached style context on nsComputedDOMStyle when its element is moved.

Better fix this first.

PROCESS-CRASH | dom/base/test/test_bug588990.html | application crashed [@ nsComputedDOMStyle::ClearStyleContext()]

Comment 39

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=377965fa2c56

Comment 40

[Cameron McCormack (:heycam)]

Attachment: Part 6.1: Clear cached style context on nsComputedDOMStyle when its element is moved. (v2)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8661661 [details] [diff] [review]
Part 6.1: Clear cached style context on nsComputedDOMStyle when its element is moved. (v2)

r=me

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76d24976824c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5efb8afff6c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd4992e71be
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa31a6af845a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8537b5ef948
https://hg.mozilla.org/integration/mozilla-inbound/rev/b66ac6fdf07c
https://hg.mozilla.org/integration/mozilla-inbound/rev/856b829f6b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd7255b1c5f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac81086dc267

Comment 43

[Cameron McCormack (:heycam)]

Thanks for the reviews, Boris.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76d24976824c
https://hg.mozilla.org/mozilla-central/rev/5efb8afff6c7
https://hg.mozilla.org/mozilla-central/rev/3dd4992e71be
https://hg.mozilla.org/mozilla-central/rev/aa31a6af845a
https://hg.mozilla.org/mozilla-central/rev/b8537b5ef948
https://hg.mozilla.org/mozilla-central/rev/b66ac6fdf07c
https://hg.mozilla.org/mozilla-central/rev/856b829f6b0a
https://hg.mozilla.org/mozilla-central/rev/fd7255b1c5f8
https://hg.mozilla.org/mozilla-central/rev/ac81086dc267