Open
Bug 738143
Opened 13 years ago
Updated 2 years ago
Don't do potentially wasteful reflows of reflow roots whose parent is dirty ("ASSERTION: Reflowing while a resize is pending is wasteful")
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
NEW
People
(Reporter: jruderman, Assigned: jwatt)
References
Details
(4 keywords)
Attachments
(3 files, 1 obsolete file)
###!!! ASSERTION: Reflowing while a resize is pending is wasteful: '!(GetStateBits() & NS_FRAME_IS_DIRTY)', file layout/svg/base/src/nsSVGForeignObjectFrame.cpp, line 163
This assertion was added in bug 734079. Based on the comment above the assertion, it seems like a case of http://mozillamemes.tumblr.com/post/19412101741/well-let-the-machines-think-for-us ;)
Reporter | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
Yeah, that assert is just bogus as discussed over e-mail...
Assignee | ||
Comment 3•13 years ago
|
||
Oh, yeah, I should just remove that.
Boris, how about having the presShell skip reflow roots whose parent has a dirty bit set, on the basis that its going to get a reflow coming via its parent, and if it waits until then it will reflow at the correct size?
Comment 4•13 years ago
|
||
Hmm. That seems like it would work. David?
Comment 5•13 years ago
|
||
Yeah, that sounds fine. (The reflow root will always have an is-dirty or has-dirty-children bit set.)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → jwatt
Component: SVG → Layout
Keywords: perf
OS: Mac OS X → All
QA Contact: general → layout
Hardware: x86_64 → All
Summary: "ASSERTION: Reflowing while a resize is pending is wasteful" → Don't do potentially wasteful reflows of reflow roots whose parent is dirty ("ASSERTION: Reflowing while a resize is pending is wasteful")
Assignee | ||
Comment 6•13 years ago
|
||
The "It's not dirty anymore" comment that I'm modifying was originally written by you, dbaron:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.1129#6394
Attachment #610254 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #610254 -
Attachment is obsolete: true
Attachment #610254 -
Flags: review?(dbaron)
Attachment #610670 -
Flags: review?(dbaron)
Comment 8•13 years ago
|
||
Comment on attachment 610254 [details] [diff] [review]
patch
> if (!NS_SUBTREE_DIRTY(target)) {
>- // It's not dirty anymore, which probably means the notification
>- // was posted in the middle of a reflow (perhaps with a reflow
>- // root in the middle). Don't do anything.
>+ // It's not dirty anymore. This may be because reflow came via an
>+ // ancestor reflow root that was reflowed before it, or it may be
>+ // because the FrameNeedsReflow call happened in the middle of a
>+ // reflow (perhaps with a reflow root in the middle). Don't do
>+ // anything.
>+ continue;
>+ }
I don't think the "(perhaps with a reflow root in the middle)" adds anything useful, unless I'm misunderstanding what it's saying. Otherwise good.
>+ if (target->GetCrossDocParentFrame() &&
>+ NS_SUBTREE_DIRTY(target->GetCrossDocParentFrame()) {
>+ // Reflowing this reflow root using its parent's current size could
>+ // be wasteful since it's parent is awaiting a reflow which may
>+ // resize it and require this reflow root to reflow again. Instead of
>+ // reflowing this reflow root now, we skip it and let its reflow come
>+ // via its parent.
> continue;
> }
Could you put the result of target->GetCrossDocParentFrame() in an nsIFrame* variable rather than calling the function twice?
You should also add a comment explaining why this is particularly useful for the SVG case, since in general this is a very weak optimization.
So what happens if the cross-doc parent frame is dirty, but the reflow for which its dirty leads to it not actually changing size? What ensures we'd reflow in that case?
e.g., in the stack:
#0 nsPresShell::ResizeReflow
#1 nsViewManager::DoSetWindowDimensions
#2 nsViewManager::SetWindowDimensions
#3 DocumentViewerImpl::SetBounds
#4 nsDocShell::SetPositionAndSize
#5 nsFrameLoader::UpdateBaseWindowPositionAndSize
#6 nsFrameLoader::UpdatePositionAndSize
#7 ReflowFinished
#8 nsSubDocumentFrame::ReflowFinished
, nsViewManager::DoSetWindowDimensions (and probably some of the other points) has an !oldDim.IsEqualEdges(newDim) check that would prevent anything from propagating through
Comment 9•13 years ago
|
||
Comment on attachment 610670 [details] [diff] [review]
patch
Marking review- per the previous comment (particularly the last issue)... at least to prompt a response.
Attachment #610670 -
Flags: review?(dbaron) → review-
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•