Closed Bug 997266 Opened 6 years ago Closed 6 years ago

26-52% tresize regression on osx 10.6/10.8 on inbound due to revision 263f232855ad

Categories

(Core :: Layout, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jmaher, Unassigned)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

tresize didn't like the change in bug 974197.  tresize depends on mozafterpaint, so this might be explainable.

I had commented in bug 974197, 18, 19:
https://bugzilla.mozilla.org/show_bug.cgi?id=974197#c18
https://bugzilla.mozilla.org/show_bug.cgi?id=974197#c19

now that this is landed, we see the tests showing the same changes:
http://graphs.mozilla.org/graph.html#tests=[[254,63,24],[254,63,21]]&sel=1397429436000,1397602236000&displayrange=7&datatype=running

I had done some retriggers and made sure we had tests before/after this patch, it is definitely the patch from bug 974197:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=40260af420a7&tochange=d71e43379733&jobname=Rev5%20MacOSX%20Mountain%20Lion%2010.8%20mozilla-inbound%20talos%20chromez

I know this is a large regression- does it make sense with the change?  

If this is the only thing affected, is there a reason why other platforms didn't have problems?
Bas, is this the stuff you mentioned on IRC? Does this mean that mozAfterPaint now fires later - and more correctly - than before, which is what causes the numbers to go up?
Flags: needinfo?(bas)
Blocks: 974197
No longer depends on: 974197
Oops, I meant to send an email to m.d.tree-management about this, looks like I just emailed the bot instead.

This is an expected regression from my change.

It changes the timing of the MozAfterPaint to be fired when we actually composite rather than when we finishing painting layers.

Test like this that rely on the timing of MozAfterPaint will show as regressions, since we're actually measuring the full pipeline now instead of part of it.

I assume this is what Bas referred to on IRC as well.
Flags: needinfo?(bas)
That makes sense.  What I am confused on is why this is only on osx?  Many of our tests use mozafterpaint (tp5, ts_paint, tpaint, tresize, a11y), tresize is the only one that adjusts the application size once it is launched.
(In reply to Joel Maher (:jmaher) from comment #3)
> That makes sense.  What I am confused on is why this is only on osx?

Only OS X has OMTC enabled by default, an Matt's patch only makes a difference with OMTC. Without OMTC, we were already including compositing in our measurement because it happens synchronously on the main thread before the MozAfterPaint event can fire.

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Test like this that rely on the timing of MozAfterPaint will show as
> regressions, since we're actually measuring the full pipeline now instead of
> part of it.

At least in tresize, haven't we been testing the full pipeline already? The test measures the time between the start of the call to window.resizeTo(dims,dims) and the next MozAfterPaint event. But window resizing triggers a sync composite, so during the next trip through the main thread event loop, the composite has already happened, so shouldn't the MozAfterPaint event always be there, with or without your patch? Or does the IPC message have enough of a delay to show up in the measurements? Or does the different timing at which the event gets inserted into the event loop allow another event to get in between?
(In reply to Markus Stange [:mstange] from comment #4)
 
> At least in tresize, haven't we been testing the full pipeline already? The
> test measures the time between the start of the call to
> window.resizeTo(dims,dims) and the next MozAfterPaint event. But window
> resizing triggers a sync composite, so during the next trip through the main
> thread event loop, the composite has already happened, so shouldn't the
> MozAfterPaint event always be there, with or without your patch? Or does the
> IPC message have enough of a delay to show up in the measurements? Or does
> the different timing at which the event gets inserted into the event loop
> allow another event to get in between?

I think the flow of events is:

JS resizes the window
We receive a resize event, very little processing is done (painting is deferred)
We receive a paint event, we synchronously flush layer painting
Without my patch we fire the MozAfterPaint event to JS
We synchronously block on the compositor until it completes
With my patch Compositor adds a 'did composite' event to the main thread queue
We return did the paint event
With my patch we process the 'did composite' event and fire MozAfterPaint

Especially since composite will likely block until the next vsync, then it makes sense that there would be significant time different between when we used to fire the event and when we do now.
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> (In reply to Markus Stange [:mstange] from comment #4)
>  
> > At least in tresize, haven't we been testing the full pipeline already? The
> > test measures the time between the start of the call to
> > window.resizeTo(dims,dims) and the next MozAfterPaint event. But window
> > resizing triggers a sync composite, so during the next trip through the main
> > thread event loop, the composite has already happened, so shouldn't the
> > MozAfterPaint event always be there, with or without your patch? Or does the
> > IPC message have enough of a delay to show up in the measurements? Or does
> > the different timing at which the event gets inserted into the event loop
> > allow another event to get in between?
> 
> I think the flow of events is:
> 
> JS resizes the window
> We receive a resize event, very little processing is done (painting is
> deferred)
> We receive a paint event, we synchronously flush layer painting
> Without my patch we fire the MozAfterPaint event to JS

You're right. I didn't think we'd call the event listeners while window.resizeTo was still on the stack, but that's exactly what we did. (I confirmed in a local talos tresize run.)
Thanks, that clears it up!
Thanks Matt and Markus.  I vote to mark this as wontfix unless you guys know of something that has been overlooked?
chatting with :avih on IRC, wontfix seems like the best fit here!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.