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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.)

Comment 4

[Daniel Holbert [:dholbert]]

Try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=63cba9376f18
Try run with just the crashtest, for fun (to watch it time out the harness):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=448d4bac8884

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(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.)

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Daniel Holbert [:dholbert]]

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.)

Comment 8

[Mats Palmgren (inactive)]

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?

Comment 9

[Daniel Holbert [:dholbert]]

(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.)

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Regardless -- I think you're right

(about the call being needed, I mean)

Comment 12

[Daniel Holbert [:dholbert]]

Landed, with comment 6 addressed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d87c50c30329

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d87c50c30329

Comment 14

[Amit Zur]

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?

Comment 15

[Daniel Holbert [:dholbert]]

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