Mike Conley (:mconley) (:⚙️) Reporter
	Description • 11 years ago

According to a recent reflow profile, during the customize transition, we spend quite a bit of time (approximately 150ms) reflowing the customization-panelHolder.

This is because the panel holder is visible during the entering transition, and the panel gets stretched to the bottom of the blue pane. As the padding increases, the blue pane gets smaller, squeezing the customization-panelHolder, causing its contents to reflow.

	Blake Winton (:bwinton) (:☕️)
	Comment 1 • 11 years ago

This seems to work for me, on Mac.  Let's see if it changes the profile numbers?  :)

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 2 • 11 years ago

Comment on attachment 8363834 [details] [diff] [review]
bug962657.patch

I can't seem to get a comparison profile working with the new profiler file format. :/

Manually looking at them side by side though, I don't see an appreciable difference in the amount of time we're spending painting or reflowing.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 3 • 11 years ago

So I think the panel holder is a major contributor to bad customization transition performance on Windows.

Here's my proof:

http://tests.themasta.com/cleopatra/

Filter on "Reflow". According to this, 1865ms was spent reflowing, and if you drill down, the culprit is clearly the panel. Specifically, it's the individual item icons in the menu panel, taking a long time to reflow on MaxSize and MinSize.

I think this is where we should focus next.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 4 • 11 years ago

Comment on attachment 8363834 [details] [diff] [review]
bug962657.patch

We don't animate the bottom margin of the content-deck, so this patch is no longer necessary.

	Blake Winton (:bwinton) (:☕️)
	Comment 5 • 11 years ago

The main point of that patch was to have the panel not change size, so that the things inside it wouldn't reflow.  If we're not animating the bottom margin, then why are we still seeing reflows?

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 6 • 11 years ago

(In reply to Blake Winton (:bwinton) from comment #5)
> The main point of that patch was to have the panel not change size, so that
> the things inside it wouldn't reflow.  If we're not animating the bottom
> margin, then why are we still seeing reflows?

I don't know - I think I've exposed some of my ignorance on what causes reflows. :/ I'm going to ask around in #layout to see what these MaxSize and MinSize things are all about.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 7 • 11 years ago

dholbert - mattwoodrow tells me you might be the one to ask about this, seeing as this involves CSS (and maybe XUL) flexboxes.

Do you know what MaxSize and MinSize reflows are caused by, why we're hitting them while transitioning the margin of a container element, and how best to mitigate them?

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 8 • 11 years ago

Bogus profile link - this one is better:

http://tests.themasta.com/cleopatra/?report=2f8bce629c9fa5756ebd4278e48f8e6ec2218227

	Daniel Holbert [:dholbert]
	Comment 9 • 11 years ago

[caveat: I'm only tangentially familiar with the "old" XUL (flex)box implementation, from picking at it a few times. And from looking at the profile, it looks like this *is* mostly XUL, not CSS "new" flexbox. (at least, there's a deeply nested <hbox>/<vbox>/<box> tree that all of this is happening inside, based on the profile)  So I may not be super-useful, unless I dive in headfirst and spend a while on this.]

Hight-level: as you noticed in IRC, it looks like MinSize/MaxSize in that profile correspond to DISPLAY_MIN_SIZE/DISPLAY_MAX_SIZE, which happen in calls to GetMinSize() & GetMaxSize(), which are specific to XUL Box reflow.

Documentation on these methods here:
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#2749

They're probably being called as part of nsBox::GetPrefSize(), which gets called in various places.

I don't know offhand what to suggest to reduce these calls.  You might have better luck asking someone who's written a lot of XUL, who may have run into issues like this before. :)  But if I were to look into this myself, I'd probably need a testcase and to dive in with GDB to see where these calls are originating and what looks redundant.

	Daniel Holbert [:dholbert]
	Comment 10 • 11 years ago

(In reply to Daniel Holbert [:dholbert] from comment #9)
> And from looking at the
> profile, it looks like this *is* mostly XUL, not CSS "new" flexbox.

(actually s/mostly/entirely/, I think)

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 11 • 11 years ago

Thanks dholbert.

One thing that's immediately suspicious is that the huge cost of the panel during the transition isn't nearly as apparent on OS X. Those MaxSize and MinSize calls take fractions of a millisecond, as opposed to 50ms on Windows 7.

I'm pretty certain layout is supposed to be platform independent, so this makes me wonder if there's some style being applies to Windows (or to OS X) that is making the difference here.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 12 • 11 years ago

So, did some more detective work here.

I moved the reflow profiler probe in nsBox::GetMaxSize and nsBox::GetMinSize to after AddBorderAndPadding, and GetMaxSize improved substantially, which suggests to me that for GetMaxSize, we're doing something slow related to borders or paddings on the menu panel toolbarbutton icons.

I can't say the same for GetMinSize - that didn't move at all, really. Still, I think this sounds like an avenue worth exploration.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 13 • 11 years ago

An idea I had last night that I wanted to write down:

UX originally had the idea of animating the panel _moving in_ to the blue grid area via a smooth transition (as opposed to the re-parenting and slide over that we're currently doing).

Originally this plan was abandoned as being too complex, and a "nice to have", but thinking about it now, if we use CSS transforms to pull this off, we can effectively remove the panel from the flow of the document during the transition, and skip _all of the reflows_ that occur during it.

Once the transition is done, if necessary, we can re-parent in order to make the panel snap (or grow) to the height of the window.

This not only has the potential of skipping these slow MinSize / MaxSize reflow things on Windows (and the other OS's as well), but is also closer to UX's original vision of this transition.

I think this idea is worth pursuing.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 14 • 11 years ago

So talking with bz, BenWa and mstange, I'm starting to suspect that this reflow profile is a red herring. The SPS profile I got from a Windows 7 machine tells a completely different story:

http://people.mozilla.org/~bgirard/cleopatra/#report=ef6cb917dcbfeeb84fac5c4d835a38968e12e506

This profile lands the blame squarely in Painting / Graphics. box-shadow's, for example, are in the list of expensive things that occur during the transition.

Bug 962640 tracks reducing box-shadows - I might lean on that a bit and see if / where it moves things on Windows.

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 15 • 11 years ago

For context, I'm starting to suspect that for this 150ms transition, the reflow profiler is adding enough overhead to skew the results. mstange has told me that if the reflow profiler and the SPS profiler disagree, go with the SPS profiler's results.

	Blake Winton (:bwinton) (:☕️)
	Comment 16 • 10 years ago

(I'm not actually working on this, and suspect it could be resolved WFM…)

	Mike Conley (:mconley) (:⚙️) Reporter
	Comment 17 • 10 years ago

Resolving INVALID as per comments 14 through 16.