Note that this is totally non-prioritary for now, since lazy frame construction works. The following shouldn't block shipping unless lazy frame construction appears in the profiles. Lazy frame construction is a top-down traversal looking for nodes for which we need to recreate frames using the NODE_DESCENDANT_NEEDS_FRAME and NODE_NEEDS_FRAME flags, that ends up calling ContentRangeInserted or ContentAppended in the frame constructor. After going through that code, I couldn't but notice the parallelism with our RecreateStyleContexts traversal, where we effectively do a similar DOM traversal (driven by other two flags). David and Timothy commented about merging restyling and lazy frame construction in Gecko in bug 827239, so seems like this isn't done just because nobody has taken the time to do so. It would be nice to traverse the DOM just once, probably collecting change hints instead of just recreating the frames in-place for the lazy frame construction case (to avoid both duplicate frame construction and keep the concerns separated). I'd have to look into all the hacks that we have in the frame constructor's methods to see if any of them would be a roadblocker. Also we should check what are the implications of using the plain flat tree traversal we use for lazy frame construction vs the StyleChildrenIterator we use for style contexts (though it seems we disable lazy frame construction for children of native anonymous roots, so my intuition is that using StyleChildrenIterator should just work).

In bug 1331047, I'm running into issues where the current setup causes us to try to style doomed native-anonymous elements (NAC elements generated by a frame which will be reconstructed). This is problematic because we rely on _not_ styling doomed elements in order to guarantee that we have a rule node for NAC elements corresponding to eagerly-cascaded pseudos (since modulo this bug, the removal of the pseudo from the originating element's pseudo-set would have triggered a reconstruct, causing us to avoid traversing the element). I have a patch for this - let's see what try says.

MozReview-Commit-ID: FSXKAiyZDzt

Comment on attachment 8842514 [details] [diff] [review] Handle lazy frame construction in the regular post-servo pass. v1 Review of attachment 8842514 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Now the hard question though... Have you verified the NODE_NEEDS_FRAME flag goes away once we enter the frame constructor? I think it _should_, because the ReconstructFrame should do a ContentRemoved and then a ContentInserted, which ends up in ContentRangeInserted, which removes the flags in a fair amount of situations. I don't see us having a lot of strong assertions there, but would be nice to MOZ_ASSERT(!aContent->HasFlag(NODE_NEEDS_FRAME)) under the ReconstructFrame handling. r=emilio with that. ::: layout/base/ServoRestyleManager.cpp @@ +184,1 @@ > changeHint); Given you've renamed the change list, let's keep aligned the arguments here. @@ -397,3 @@ > PresContext()->EffectCompositor()->ClearElementsToRestyle(); > > - // First do any queued-up frame creation. (see bugs 827239 and 997506). Perhaps a comment pointing out that we do lazy frame construction during the post-traversal instead of here may be helpful if someone is going through the two restyle managers side by side. No big deal though. ::: layout/base/nsStyleChangeList.cpp @@ +30,5 @@ > (aFrame && aContent->GetParent() && > aFrame->PresContext()->FrameManager()-> > + GetDisplayContentsStyleFor(aContent->GetParent())) || > + (aContent->HasFlag(NODE_NEEDS_FRAME) && > + aHint & nsChangeHint_ReconstructFrame), Perhaps aHint == nsChangeHint_ReconstructFrame is in this case a better assertion, though maybe we don't have operator== there? Also, maybe assert aContent->IsNodeOfType(eText) && aContent->IsStyledByServo()? Though this assertion is already quite big.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5) > Comment on attachment 8842514 [details] [diff] [review] > Handle lazy frame construction in the regular post-servo pass. v1 > > Review of attachment 8842514 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall looks good. Now the hard question though... Have you verified the > NODE_NEEDS_FRAME flag goes away once we enter the frame constructor? I think > it _should_, because the ReconstructFrame should do a ContentRemoved and > then a ContentInserted, which ends up in ContentRangeInserted, which removes > the flags in a fair amount of situations. > > I don't see us having a lot of strong assertions there, but would be nice to > MOZ_ASSERT(!aContent->HasFlag(NODE_NEEDS_FRAME)) under the ReconstructFrame > handling. I don't think we can assert this. There are various places where the frame constructor bails out right now without creating frames and clearing the bits (i.e. [1]), which means that (I'm pretty sure) we just leave NODE_NEEDS_FRAME set on the element. Ideally long-term, we'd get rid of NODE_DESCENDANTS_NEED_FRAMES, and only set NODE_NEEDS_FRAME on the roots of lazily-constructed subtrees. But that's out of scope for this patch I think. [1] http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/base/nsCSSFrameConstructor.cpp#7858

In bug 1343449, dbaron suggested that we may want/need to coalesce the insertion of adjacent siblings during lazy frame construction, like CreateNeededFrames does. In local testing, this seems to do the right thing. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da53af17afc030df83ddba349b9ffd749a4517cd

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6) > I don't think we can assert this. There are various places where the frame > constructor bails out right now without creating frames and clearing the > bits (i.e. [1]), which means that (I'm pretty sure) we just leave > NODE_NEEDS_FRAME set on the element. That looks like a bug to me. In theory, we just set the lazy-construction bits when the parents have a frame (or are display contents, in which case a insertion point exists anyway), so I don't think that specific case is reached. If it is, we should just clear the bits there I think. It really feels like we should assert that. Also, some unexpected performance impact that this has is that right now sending a ReconstructFrame hint will walk over all the change list (see nsStyleChangeList::AppendChange), which can get slow pretty fast. I tried to fix that yesterday (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b98e3b4e255e95264dd4ab642bc1e0d100d89fa&selectedJob=81344398), but it still needs a bunch of work to get it right. Certainly not so much in the Servo case (where we're pretty sure we only post change hints one child at a time), but in the Gecko case all the asserts that seemed logical and worked locally ended up in a big amount of orange :(

This was green on try without the NODE_NEEDS_FRAME assertion. Running the assertion through try now. MozReview-Commit-ID: EEjC0VgYHsC

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=18a4053209a271b4b8b26216baeeb07396b07687 This is green.

Comment on attachment 8843377 [details] [diff] [review] Part 0 - Collect and coalesce adjacent siblings for lazy frame construction. v1 Review of attachment 8843377 [details] [diff] [review]: ----------------------------------------------------------------- I wonder what the impact of this is in non-stylo builds? If you could do some measurements, I guess it would be useful. Probably it's not a big deal, but worth checking. Ideally we'd be able to do this at AppendChange, coalescing hints. That's definitely doable for stylo (it'd be a matter of keeping a reference to the end of the range too in nsChangeHintData, and looking whether it's our previous sibling), since we insert changes in DOM order. Assuming you're fine with the potential perf issue from comment 9 (which we can eliminate completely for stylo too, I believe), and that this has no noticeable perf impact for non-stylo build, this is fine by me. That being said, I think I'd prefer dbaron to sign it off too if he has the time? (feel free to not wait if you think it's not necessary). ::: layout/base/RestyleManager.cpp @@ +1394,5 @@ > + > + // Collect and coalesce adjacent siblings for lazy frame construction. > + // Eventually it would be even better to make RecreateFramesForContent > + // accept a range and coalesce all adjacent reconstructs (bug 1344139). > + size_t lazy_range_start = i; nit: Rusty variable name, but it's not common here ;) @@ +1399,5 @@ > + while (i < aChangeList.Length() && > + aChangeList[i].mContent && > + aChangeList[i].mContent->HasFlag(NODE_NEEDS_FRAME) && > + (i == lazy_range_start || > + aChangeList[i - 1].mContent->GetNextSibling() == aChangeList[i].mContent)) Well, this only looks at the light tree for coalescing, not at the flattened tree. I guess that's ok-ish, since it will catch most of the stuff? @@ +1417,5 @@ > + } else { > + frameConstructor->ContentRangeInserted(container, start, end, nullptr, false); > + } > + } > + for (size_t j = lazy_range_start; j < i; ++j) { Maybe #ifdef DEBUG the loop? Any sane compiler would optimize it out I guess, but still...

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13) > Comment on attachment 8843377 [details] [diff] [review] > Part 0 - Collect and coalesce adjacent siblings for lazy frame construction. > v1 > > Review of attachment 8843377 [details] [diff] [review]: > ----------------------------------------------------------------- > > I wonder what the impact of this is in non-stylo builds? If you could do > some measurements, I guess it would be useful. Probably it's not a big deal, > but worth checking. I don't think it should have any impact, since non-stylo builds generally won't have NEEDS_FRAME set on elements reached by ProcessRestyledFrames. Also, the nice thing about m-c is that we have Talos to tell us if we have any very-bad and very-unexpected performance regressions. :-) > > Ideally we'd be able to do this at AppendChange, coalescing hints. That's > definitely doable for stylo (it'd be a matter of keeping a reference to the > end of the range too in nsChangeHintData, and looking whether it's our > previous sibling), since we insert changes in DOM order. Yeah, I think the extra overhead of storing that information in the change list isn't worth it. Doing it this way is basically free. > > Assuming you're fine with the potential perf issue from comment 9 (which we > can eliminate completely for stylo too, I believe), and that this has no > noticeable perf impact for non-stylo build, this is fine by me. I filed bug 1344386 about eliminating that impact for stylo. I don't think we should worry about it for non-stylo, because the days of that are numbered. > That being > said, I think I'd prefer dbaron to sign it off too if he has the time? (feel > free to not wait if you think it's not necessary). I discussed all this pretty extensively with bz and dbaron last night, so I don't think we need sr at this point. > > ::: layout/base/RestyleManager.cpp > @@ +1394,5 @@ > > + > > + // Collect and coalesce adjacent siblings for lazy frame construction. > > + // Eventually it would be even better to make RecreateFramesForContent > > + // accept a range and coalesce all adjacent reconstructs (bug 1344139). > > + size_t lazy_range_start = i; > > nit: Rusty variable name, but it's not common here ;) Hah! Look what I've become. :-) > > @@ +1399,5 @@ > > + while (i < aChangeList.Length() && > > + aChangeList[i].mContent && > > + aChangeList[i].mContent->HasFlag(NODE_NEEDS_FRAME) && > > + (i == lazy_range_start || > > + aChangeList[i - 1].mContent->GetNextSibling() == aChangeList[i].mContent)) > > Well, this only looks at the light tree for coalescing, not at the flattened > tree. I guess that's ok-ish, since it will catch most of the stuff? Yeah. getting flattened siblings is kind a hard, and I hit some assertions in the frame constructor when passing in the flattened tree parent. > > @@ +1417,5 @@ > > + } else { > > + frameConstructor->ContentRangeInserted(container, start, end, nullptr, false); > > + } > > + } > > + for (size_t j = lazy_range_start; j < i; ++j) { > > Maybe #ifdef DEBUG the loop? Any sane compiler would optimize it out I > guess, but still... I trust the compiler on that one. :-) Thanks for the review!

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a57ff8701dc2 Collect and coalesce adjacent siblings for lazy frame construction. r=emilio https://hg.mozilla.org/integration/autoland/rev/4a2a58be8c6c Handle lazy frame construction in the regular post-servo pass. r=emilio

https://hg.mozilla.org/mozilla-central/rev/a57ff8701dc2 https://hg.mozilla.org/mozilla-central/rev/4a2a58be8c6c