Closed Bug 745485 Opened 8 years ago Closed 6 years ago

Avoid reflows when a position change does not result in an actual change to the dimensions of a frame (top/left/right/bottom changes)

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ehsan, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

Bug 157681's optimization is a little bit more conservative than it could be for the abs-pos case.  To avoid all unnecessary reflows, nsCSSFrameConstructor::RecomputePosition should be modified to compute the new dimensions where we suspect there to be a dimension change, and only fall back to reflow when the dimensions really change.
Duplicate of this bug: 729597
This is not a regression, it's an idea for further optimization.  Definitely not needed to be tracked.
Keywords: regression
But bug 729597 is a regression, which has now regressed even further.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #3)
> But bug 729597 is a regression, which has now regressed even further.

It is not a regression, it's just a test case which used to do nothing because of bug 10209, and now does something, and the thing it does is expensive, and has always been so.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Blocks: 302909
Summary: Avoid reflows when a position change does not result in an actual change to the dimensions of a frame → Avoid reflows when a position change does not result in an actual change to the dimensions of a frame (top/left/right/bottom changes)
Assignee: nobody → roc
Comment on attachment 810216 [details] [diff] [review]
fix

Not quite right yet.
Attachment #810216 - Attachment is obsolete: true
Attached patch diff -wSplinter Review
This turned out to be pretty simple, I think!
Attachment #810249 - Flags: review?(dbaron)
Attachment #810249 - Flags: review?(dbaron) → review?(dholbert)
Comment on attachment 810249 [details] [diff] [review]
diff -w

Looks good! Just some comment-clarifying nits:

>+++ b/layout/base/RestyleManager.cpp
>@@ -379,27 +379,16 @@ RestyleManager::RecomputePosition(nsIFra
>     // For the absolute positioning case, set up a fake HTML reflow state for
>     // the frame, and then get the offsets from it.

This contextual comment needs adjusting - s/offsets/offsets and size/
(due to the reflowState.ComputedWidth() & ComputedHeight() accesses added in this patch)

BUT, while you're at it: it might be good to extend that comment slightly to explain the high-level idea of this chunk a little better now, along the lines of the comment higher up that says "For relative positioning, we can simply update the frame rect".

Maybe something like:
  // For the absolute positioning case: if the frame's size hasn't changed, we
  // can simply update the frame rect (and otherwise, fall back to a reflow).
  // To determine whether the size has changed, and the frame's new position,
  // set up a fake reflow state and get the size/offsets from it.

>+++ b/layout/style/nsStyleStruct.cpp
>+  // Don't try to handle changes between "auto" and non-auto efficiently.

Maybe add:
... "because such changes are likely to change the size and require reflow"
(right?)

r=me
Attachment #810249 - Flags: review?(dholbert) → review+
(er, s/and otherwise/or otherwise/, probably)
https://hg.mozilla.org/mozilla-central/rev/046d7a70ef24
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Does this mean that we do off-main-thread animations when top/left/bottom/right is animated? Except when reflow is actually needed of course.
(In reply to comment #13)
> Does this mean that we do off-main-thread animations when top/left/bottom/right
> is animated? Except when reflow is actually needed of course.

No, we just avoid reflowing, and move the frame directly, but this all happens on the main thread.
Whiteboard: [qa-]
Depends on: 1200585
Depends on: 1485251
No longer depends on: 1485251
You need to log in before you can comment on or make changes to this bug.