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

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

(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 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 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 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 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 on attachment 8659779 [details] [diff] [review] Part 5: Rename nsComputedDOMStyle::mStyleContextHolder to mStyleContext. r=me

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

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

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

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); } }

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

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

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

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

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

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

Just added a comment and assertion.

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

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

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

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.

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.

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

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 on attachment 8661661 [details] [diff] [review] Part 6.1: Clear cached style context on nsComputedDOMStyle when its element is moved. (v2) r=me

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

Thanks for the reviews, Boris.

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