Closed Bug 1054010 Opened 7 years ago Closed 6 years ago

Skip final reflow of flex items if they had a "measuring" reflow at the correct final size

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Spinning this off to flesh out the strategy in my patch for bug 946167. (posted over there as attachment 8458929 [details] [diff] [review])

Basically, the idea here is: if a flex container has done a "measuring reflow" of one of its flex items, and then it turns out that the flex item's final size (produced by the flex layout algorithm) is the same size that we happened to use in that earlier measuring reflow, then we should skip the item's "final" reflow and (at most) just adjust the flex item's position.

(I'm spinning this off into its own bug because bug 946167 is already kind of long, and it's also kind of fixed, as discussed in bug 946167 comment 21 - bug 946167 comment 22.)
Summary: Skip final reflow of flex items if we've already reflowed them at their final size → Skip final reflow of flex items if they had a "measuring" reflow at the correct final size
OS: Linux → All
Hardware: x86_64 → All
Depends on: 1054046
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 1054054
Blocks: 1082780
Duplicate of this bug: 1082780
Duplicate of this bug: 1097482
Keywords: perf
Blocks: 1108772
Blocks: 1124917
Attached patch fix v1 (obsolete) — Splinter Review
For flex items that have received a "measuring reflow" earlier in this call to nsFlexContainerFrame::Reflow, which ended up at the correct final size (e.g. because they're inflexible, or there's no extra space), this patch makes us skip the final reflow & instead just adjust their position as-necessary.

Crashtest included, which times out the crashtest harness on current trunk but which passes in ~1/2 second with this patch applied.

(This patch also reduces the assertion-count in another crashtest, because that other crashtest's assertions are triggered during reflow of a flex item, and we'll reflow that flex item fewer times now.)
Attachment #8553904 - Flags: review?(mats)
Attached patch fix v1aSplinter Review
(er, previous version had some stale junk in 2nd/3rd line of commit message, from folding patches together. Pruned that now, so it's just got a single commit message.)
Attachment #8553904 - Attachment is obsolete: true
Attachment #8553904 - Flags: review?(mats)
Attachment #8553909 - Flags: review?(mats)
Comment on attachment 8553904 [details] [diff] [review]
fix v1

>+  aItem.Frame()->SetPosition(outerWM, aFramePos, aContainerWidth);

I think we need a "PositionFrameView(aItem.Frame());" after the above
line, in case the frame has a view (eg nsPluginFrame).

r=mats with that
Attachment #8553904 - Flags: review+
Attachment #8553909 - Flags: review?(mats) → review+
Hmm. Do you know how I can test for that working correctly? (P

I'm playing with a testcase that has:
   <embed src="http://homestarrunner.com/welcome.swf"
          width="300" height="300"></embed>
inside a flex container -- which I've set up to rely on this MoveFlexItemToFinalPosition() codepath to get to the right spot -- and I can't get the rendering to break.

(I've confirmed that I'm not getting any "lucky" calls to PositionFrameView for the nsPluginFrame, and I also confirmed that I *do* get calls if I disable this bug's optimization.)
I don't know if/what rendering depends on a nsView, but I think event dispatch still
does.  Perhaps a test that uses a click event / mouseenter / :hover might fail?
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (I've confirmed that I'm not getting any "lucky" calls to PositionFrameView
> for the nsPluginFrame

(I should clarify this slightly -- I was talking about incremental reflows there, e.g. from resizing my browser-window, with my flex container sized to fill the window.

In the *initial* reflow, I *do* end up getting a PositionFrameView call for the plugin.  It happens just after nsFlexContainerFrame::Reflow finishes, when its parent calls FinishReflowChild on it -- that method invokes PositionChildViews() for the child that's just finished reflow (the flex container in this case), and that recursively walks the flex container's descendants, including the plugin.

But then in subsequent incremental reflows, e.g. from adjusting the size of the nsFlexContainerFrame such that the plugin moves around, I don't get that call anymore (presumably because the FinishReflowChild() on the flex container in *that* reflow sees that its origin hasn't moved, which makes it skip PositionChildViews.)
(In reply to Mats Palmgren (:mats) from comment #8)
> I don't know if/what rendering depends on a nsView, but I think event
> dispatch still
> does.  Perhaps a test that uses a click event / mouseenter / :hover might
> fail?

The homestarrunner welcome screen has text in it, which is hoverable, and the text hover effect still works. (Text changes color, & cursor changes to a finger.)

Regardless -- I think you're right (particularly after looking at the call in FinishReflowChild, which similarly happens if the frame has moved), so I'll add the call, and I'll add a test later if I figure out how.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Regardless -- I think you're right

(about the call being needed, I mean)
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/d87c50c30329
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1128354
I just wanted to say I'm following these performance issues (from bug #946167 and #1082780) as I do very similar UI generation as in the test case, and the patch does fix them. This has a huge impact on our product, so thank you for prioritizing it.

I see the patch isn't in Aurora yet. Can you point me to the right place to track its progress down the pipeline?
(In reply to Amit Zur from comment #14)
> I just wanted to say I'm following these performance issues (from bug
> #946167 and #1082780) as I do very similar UI generation as in the test
> case, and the patch does fix them. This has a huge impact on our product, so
> thank you for prioritizing it.

Great!

> I see the patch isn't in Aurora yet. Can you point me to the right place to
> track its progress down the pipeline?

This isn't getting explicitly backported -- it's "riding the release trains" like most changes do. It landed on Nightly for version 38, which moves to Aurora this week (once the version number increments), and will go to release in May, per https://wiki.mozilla.org/RapidRelease/Calendar .
Blocks: 1141108
Depends on: 1142686
Depends on: 1503173
You need to log in before you can comment on or make changes to this bug.