Closed Bug 1413833 Opened 2 years ago Closed 2 years ago

Poor performance on searchfox with retained-dl

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(2 files)

Searchfox can be really slow with retained-dl, because deleting frames does an O(n) search over the set of WeakFrames, and we can add a lot of these.
If we end up with too many modified frames, then the overhead of storing and processing them gets to be too high for us to win anything on building.

Let's use the same cap as we do for building, and fallback to just marking the root as invalid.
Attachment #8924478 - Flags: review?(mikokm)
Rather than using WeakFrame, we can do our own tracking instead.

This doesn't need a hashtable, only does the linear search if we know that it's going to find something, and breaks; as soon as it does (we know it'll only be in there once).
Attachment #8924479 - Flags: review?(mikokm)
Comment on attachment 8924478 [details] [diff] [review]
Cap the number of modified frames

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

LGTM.

::: layout/generic/nsFrame.cpp
@@ +986,5 @@
>    if (!retainedBuilder) {
>      return;
>    }
>  
> +  nsIFrame* rootFrame = PresContext()->PresShell()->GetRootFrame();

Well, this is a lot nicer than nsLayoutUtils::GetViewportFrame. :) Could you also remove that function, since it was only used here?
Attachment #8924478 - Flags: review?(mikokm) → review+
Comment on attachment 8924479 [details] [diff] [review]
Don't use WeakFrame for modified frames

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

::: layout/generic/nsFrame.cpp
@@ +974,5 @@
> +    nsTArray<nsIFrame*>* modifiedFrames =
> +      rootFrame->GetProperty(nsIFrame::ModifiedFrameList());
> +    MOZ_ASSERT(modifiedFrames);
> +
> +    for (auto& frame : *modifiedFrames) {

I thought about this part a bit, and I am worried about the performance implications of the linear search here. According to Emilio, there might be cases where we first invalidate a lot of frames, and then delete them. This would have the same problem of O(n^2) complexity during frame destruction that WeakFrames had.

@@ +1020,5 @@
>  
>    if (this == rootFrame) {
>      // If this is the root frame, then marking us as needing a display
>      // item rebuild implies the same for all our descendents. Clear them
>      // all out to reduce the number of WeakFrames we keep around.

Could you update the comment (still referring to weakframes)?
Attachment #8924479 - Flags: review?(mikokm) → review+
(In reply to Miko Mynttinen [:miko] from comment #4)
> I thought about this part a bit, and I am worried about the performance
> implications of the linear search here. According to Emilio, there might be
> cases where we first invalidate a lot of frames, and then delete them. This
> would have the same problem of O(n^2) complexity during frame destruction
> that WeakFrames had.

Based on some profiling (a reduced testcase with invalidations followed by deletion, and Facebook browsing) this does not seem like a big deal.
Blocks: 1414778
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05d3e232865
Cap the number of modified frames that we track to avoid the overhead getting too large. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd69298b447
Don't use WeakFrame for the modified frame list since get slow with large numbers of frames. r=miko
https://hg.mozilla.org/mozilla-central/rev/e05d3e232865
https://hg.mozilla.org/mozilla-central/rev/8bd69298b447
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.