Closed
Bug 995267
Opened 10 years ago
Closed 9 years ago
FrameLayerBuilder::WillEndTransaction spends 7 second in region code
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: BenWa, Assigned: BenWa)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
2.17 KB,
text/html
|
Details | |
942 bytes,
patch
|
Details | Diff | Splinter Review |
Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=9604e9483038440c2f9db9e59192a20ea6cba50e
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8405455 -
Attachment is obsolete: true
Assignee | ||
Comment 2•10 years ago
|
||
STR: 1) Open link 2) Scroll to the bottom 3) Switch tab It should take about 7 second to switch tab.
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8405541 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8405541 [details] [diff] [review] Part 2: Speed up MarkFramesForDisplayList moved to bug 999121
Attachment #8405541 -
Attachment is obsolete: true
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
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.
Description
•