Closed Bug 1344386 Opened 3 years ago Closed 3 years ago

stylo: nsStyleChangeList::AppendElement does too much work

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The stylo traversal visits each element and frame at most once, and so the coalescing has already happened.

There's some possibility this assumption might break with mReentrantChanges, but try will tell.
MozReview-Commit-ID: 9wK8TTXolPM
Attachment #8843492 - Flags: review?(emilio+bugs)
Forgot to null-check aContent before calling IsStyledByServo():

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a77ef2b2d2fcc35536c8d4dfd041f7e014b7fee
MozReview-Commit-ID: 9wK8TTXolPM
Attachment #8843538 - Flags: review?(emilio+bugs)
Attachment #8843492 - Attachment is obsolete: true
Attachment #8843492 - Flags: review?(emilio+bugs)
Assignee: nobody → bobbyholley
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c067295c39b92b81ff1aeda0973398c47be66731

This is now green.
Comment on attachment 8843538 [details] [diff] [review]
Don't look for overlapping change hints in stylo documents. v4

Review of attachment 8843538 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review based on my comment below, feel free to re-request if you think that doesn't make sense.

Thanks for doing this btw! :)

::: layout/base/nsStyleChangeList.cpp
@@ +36,5 @@
>    MOZ_ASSERT(!(aHint & nsChangeHint_AllReflowHints) ||
>               (aHint & nsChangeHint_NeedReflow),
>               "Reflow hint bits set without actually asking for a reflow");
>  
> +  if (!aAllowPreexistingForServo && aContent && aContent->IsStyledByServo()) {

nit: Since we know statically at nsStyleChangeList construction time what kind of document we're styling, perhaps we should just store a mStyleBackendType member here instead of looking up the doc all the time?

@@ +48,5 @@
> +    for (size_t i = 0; i < Length(); ++i) {
> +      MOZ_ASSERT_IF(aContent && ((aHint | (*this)[i].mHint) & nsChangeHint_ReconstructFrame),
> +                    (*this)[i].mContent != aContent);
> +      MOZ_ASSERT_IF(aFrame, (*this)[i].mFrame != aFrame);
> +    }

I think these aren't the right assertions, and aAllowPreexistingForServo looks quite hacky.

The only problem with sending the same content twice into the frame constructor is when we send two ReconstructFrame for the same content (mainly because that means that we do twice that work).

That doesn't happen with anonymous boxes (we would have reframed the owner content anyway, and we shouldn't had restyled the anon box). I don't think that happens either from the reentrant changes?

So I think the assertions should be:

 * A frame is only once in the change list (if this somehow happens from the reentrant changes sent from frame construction we could relax this, but I don't think it can happen?).
 * If this is a ReconstructFrame change, then we haven't any other change hint with the same content. This one should be unconditional, since the only way a ReconstructFrame hint could reach here is when we allow lazy insertion (see aAsyncInsert in the frame constructor), and that's not the case when we're processing change hints (we don't allow lazy insertion there). If that is reached, that's a bug we should fix IMO.
Attachment #8843538 - Flags: review?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Comment on attachment 8843538 [details] [diff] [review]
> Don't look for overlapping change hints in stylo documents. v4
> 
> Review of attachment 8843538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing review based on my comment below, feel free to re-request if you
> think that doesn't make sense.
> 
> Thanks for doing this btw! :)
> 
> ::: layout/base/nsStyleChangeList.cpp
> @@ +36,5 @@
> >    MOZ_ASSERT(!(aHint & nsChangeHint_AllReflowHints) ||
> >               (aHint & nsChangeHint_NeedReflow),
> >               "Reflow hint bits set without actually asking for a reflow");
> >  
> > +  if (!aAllowPreexistingForServo && aContent && aContent->IsStyledByServo()) {
> 
> nit: Since we know statically at nsStyleChangeList construction time what
> kind of document we're styling, perhaps we should just store a
> mStyleBackendType member here instead of looking up the doc all the time?

I can do that, sure.

> 
> @@ +48,5 @@
>  * A frame is only once in the change list (if this somehow happens from the
> reentrant changes sent from frame construction we could relax this, but I
> don't think it can happen?).

This definitely does happen when processing change hints. Thankfully, the existing handling we do for repeated frames is pretty cheap, and I think it's fine to leave in.

>  * If this is a ReconstructFrame change, then we haven't any other change
> hint with the same content. This one should be unconditional, since the only
> way a ReconstructFrame hint could reach here is when we allow lazy insertion
> (see aAsyncInsert in the frame constructor), and that's not the case when
> we're processing change hints (we don't allow lazy insertion there). If that
> is reached, that's a bug we should fix IMO.

As the try push at [1] demonstrates, UpdateStyleOfChildAnonBox can generate a reconstruct when there wasn't one on the parent.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1568e6df953d69c9ac3cf02d4d830ff42a28a2a7&selectedJob=81979209
Flags: needinfo?(emilio+bugs)
I guess we should just manually traverse the list in that case. I'll write a patch.
Flags: needinfo?(emilio+bugs)
MozReview-Commit-ID: 9wK8TTXolPM
Attachment #8844286 - Flags: review?(emilio+bugs)
Comment on attachment 8844286 [details] [diff] [review]
Don't look for overlapping change hints in stylo documents. v6

Review of attachment 8844286 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that, thanks for fixing this! :)

::: layout/generic/nsFrame.cpp
@@ +10089,5 @@
>    if (childHint) {
> +    if (childHint & nsChangeHint_ReconstructFrame) {
> +      // If we generate a reconstruct here, remove any non-reconstruct hints we
> +      // may have already generated for this content.
> +      aChangeList.RemoveChangesForContent(aChildFrame->GetContent());

Instead of this, we should be able to just scan the list right to left, popping all the previous change hints for `content`. This is what I was trying to do for Gecko, but Gecko's style system has another bunch of quirks that prevented me from doing so. For anon boxes we push changes for the same content at once, so no need to traverse the whole list.
Attachment #8844286 - Flags: review?(emilio+bugs) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> As the try push at [1] demonstrates, UpdateStyleOfChildAnonBox can generate
> a reconstruct when there wasn't one on the parent.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1568e6df953d69c9ac3cf02d4d830ff42a28a2a7&selectedJob=8
> 1979209

:(. Well, I guess that answers bz's question about whether an anon box change hint can be bigger than the one for the owner.
Well, it totally can, if the parent never had the struct queried, at the very least.  I mean, I ran into testcases that didn't behave properly for that reason if I didn't include the anon box hint.  ;)
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1112d053e282
Don't look for overlapping change hints in stylo documents. r=emilio
Attachment #8843538 - Attachment is obsolete: true
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06fb2588dcca
Followup bustage fix for bad implicit constructor. r=me
https://hg.mozilla.org/mozilla-central/rev/1112d053e282
https://hg.mozilla.org/mozilla-central/rev/06fb2588dcca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.