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

RESOLVED FIXED in mozilla28

Status

()

P3
normal
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: Ehsan, Assigned: roc)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {perf})

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Duplicate of this bug: 729597
tracking-firefox16: --- → ?
Keywords: regression
(Reporter)

Comment 2

7 years ago
This is not a regression, it's an idea for further optimization.  Definitely not needed to be tracked.
tracking-firefox16: ? → ---
Keywords: regression
But bug 729597 is a regression, which has now regressed even further.
(Reporter)

Comment 4

7 years ago
(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.

Updated

6 years ago
Blocks: 302909

Updated

6 years ago
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
Created attachment 810249 [details] [diff] [review]
diff -w

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
Last Resolved: 5 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.
(Reporter)

Comment 14

5 years ago
(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.

Updated

5 years ago
Whiteboard: [qa-]

Updated

3 years ago
Depends on: 1200585
No longer depends on: 1485251
You need to log in before you can comment on or make changes to this bug.