899867 - Avoid uninterruptible reflow in tabbrowser's resize handler

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Matthew N. [:MattN]]

Attachment: v.1 Move access to width before updateAppearance

updateAppearance causes a reflow but then makes layout dirty and then we request another uninterruptible reflow to get the width of the tabstrip. We can reduce one uninterruptible reflow by moving one line.

I want to re-verify that this is the case as I wrote this patch a few weeks ago. It didn't show a significant difference for tpaint but I think it should be re-evaluated since data shows were are doing more reflows and spending more time in them.

Comment 1

[:Gijs (he/him)]

But in updateAppearance, we'll change the placeholder sizes, which will change the tabstrip width, won't it?

Comment 2

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #1)
> But in updateAppearance, we'll change the placeholder sizes, which will
> change the tabstrip width, won't it?

Yes, you're right that this gives incorrect results.

Talos results from try anyways: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=36ed8db7aadc&newRev=924aa22d07ac&submit=true

I think there is still ways to optimize this by limiting when/what _fillTrailingGap runs and using XUL persistence for the placeholders sizes. This may only work for window creation, not in general for the resize event. I'll look at this tomorrow.

Comment 3

[:Gijs (he/him)]

Well, we could make TabsInTitlebar expose the placeholder size, and subtract it from the result of this getter, although I guess that means we're implicitly assuming they'd be 0-sized when this gets called. We could check that assumption, but it might be slightly messy. OTOH, if avoiding this reflow really gets us this much on Linux, let's do it...

Comment 4

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #3)
> OTOH, if avoiding this
> reflow really gets us this much on Linux, let's do it...

TabsInTitlebar.updateAppearance is a no-op on Linux.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > OTOH, if avoiding this
> > reflow really gets us this much on Linux, let's do it...
> 
> TabsInTitlebar.updateAppearance is a no-op on Linux.

And this is where I burst out crying about how useless talos is. How we are supposed to fix anything with this much noise is beyond me.

</rant>

Comment 6

[Matthew N. [:MattN]]

http://compare-talos.mattn.ca/?oldRevs=36ed8db7aadc&newRev=924aa22d07ac&submit=true

WONTFIX for now since if attachment 783498 [details] [diff] [review] wasn't a win then it seems like a patch that doesn't break stuff is also not going to help. Bug 901786 reduces the amount of work to be done in the reflow.

Comment 7

[Matthew N. [:MattN]]

Attachment: Part 2 - v.1 - Early return in _fillTrailingGap if we're not overflowing

If we don't flush layout from getting the tabstrip's width (e.g. part 1 / attachment 783498 [details] [diff] [review]), then _fillTrailingGap ends up causing us to flush.

It seems like _fillTrailingGap doesn't need to do anything if we aren't overflowing since I believe that means we aren't scrolling either. I haven't tested this yet so it's possible that the overflow attribute is not set in time when narrowing the window. i.e. I need to check the ordering of the resize and overflow events.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 790091 [details] [diff] [review]
Part 2 - v.1 - Early return in _fillTrailingGap if we're not overflowing

Can you file this as a separate bug so we can land it on mozilla-central and properly keep track of it?

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 790091 [details] [diff] [review]
> Part 2 - v.1 - Early return in _fillTrailingGap if we're not overflowing
> 
> Can you file this as a separate bug so we can land it on mozilla-central and
> properly keep track of it?

Matt, did you do this? If not, sounds like we should still.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Per comment #9.

Comment 11

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Comment on attachment 790091 [details] [diff] [review]
> > Part 2 - v.1 - Early return in _fillTrailingGap if we're not overflowing
> > 
> > Can you file this as a separate bug so we can land it on mozilla-central and
> > properly keep track of it?
> 
> Matt, did you do this? If not, sounds like we should still.

I think after bug 905695 this may not have much impact (at least on our benchmarks) but I filed it as bug 935241.