Closed Bug 995267 Opened 10 years ago Closed 9 years ago

FrameLayerBuilder::WillEndTransaction spends 7 second in region code

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Attached file testcase
Attachment #8405455 - Attachment is obsolete: true
STR:
1) Open link
2) Scroll to the bottom
3) Switch tab

It should take about 7 second to switch tab.
After some investigation:
* WillEndTransaction will iterate over the display item. If they haven't been used this transaction (like when we switch tab) they are considered no longer needed and removed.
* When removing the display item data we invalidate their layer for their bounds.
* In the testcase we can invalidate up to 50,000 items so we end up with a region with 50,000 rects.
* At some point before that, and this depends on the Hash Table iteration order, we will invalidate the whole page likely from the background display item which will simplify the valid region to the empty region. This means that the runtime is variable depending on the iteration order.
Attached patch patchSplinter Review
After a certain point accumulating invalid rect is just not worth the effort. Maybe 32 is too low, but 50,000 is clearly too high. Perhaps it would be better to roll this up in InvalidateRegion.

Not a huge fan of this but it preventing us from doing more work trying to save painting work then we will potentially save.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8405518 - Flags: review?(matt.woodrow)
I'm seeing the heaviest leaf of BuildDisplayList on my crazy testcase resizing this list.

Sadly nsFrameList::GetLength traverses the list which still shows up in profile but it's much cheaper then re-allocing the list several times.
Attachment #8405541 - Flags: review?(matt.woodrow)
Blocks: 981899
Comment on attachment 8405518 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1140,5 @@
>                                      data->mClip,
>                                      layerBuilder->GetLastPaintOffset(t));
> +      if (t->GetValidRegion().GetNumRects() > 32) {
> +        t->InvalidateRegion(t->GetValidRegion());
> +      }

Why not just use SimplifyOutward(32) inside InvalidatePostTransformRegion?

Jeff is working on patches to make SimplifyOutward smarter, and we'll almost get that for free (might have to switch to the new api call) when it lands if we do it that way. We'll probably forget to update this code otherwise.
Attachment #8405541 - Flags: review?(matt.woodrow) → review+
Alright blocking on bug 945079.
Depends on: 945079
Depends on: 999121
Comment on attachment 8405541 [details] [diff] [review]
Part 2: Speed up MarkFramesForDisplayList

moved to bug 999121
Attachment #8405541 - Attachment is obsolete: true
Comment on attachment 8405518 [details] [diff] [review]
patch

Is this still an issue? We simplify the invalid region in InvalidateRegion() now, but we could switch to SimplifyOutwardByArea to improve things more.
Attachment #8405518 - Flags: review?(matt.woodrow)
Nope, it's fine now.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: