991434 - WebGL fur demo is much slower in Beta 29 than Firefox 28

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Chris Peterson [:cpeterson]]

The WebGL fur demo at [1] feels much slower (like 10x slower) in Beta 29 than Firefox 28 (which feels about 2x slower than Chrome 33). I'm using OS X 10.9.2 on a Retina MBP.

[1] http://oos.moxiecode.com/js_webgl/fur/

Comment 1

[Chris Peterson [:cpeterson]]

jwatt: could this performance regression be fallout from Moz2D bug 950371?

I bisected this problem using mozregression. This problem seems to be a regression in Nightly 30 build 2014-02-22, though I'm not sure why Beta 29 seems to be affected too. Perhaps the regressing patch was uplifted from 30 to 29?

Pushlog from build 02-21 to 02-22:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7010ab83a06e&tochange=84c9885475e7

Comment 2

[u480271]

It could be related to Bug 947227. QA can you check?

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Chris Peterson (:cpeterson) from comment #1)
> jwatt: could this performance regression be fallout from Moz2D bug 950371?

No, the code that was touched in the patch for that bug is not hit during this demo.

Comment 4

[u279076]

Florin, can you please assign this to someone to investigate the regression window?

Comment 5

[Paul Silaghi, QA [:pauly]]

This is not repro on Win 7. Investigating...

Comment 6

[Paul Silaghi, QA [:pauly]]

Last good revision: 660b62608951
First bad revision: 128cbf1edc40
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=660b62608951&tochange=128cbf1edc40

Comment 7

[Paul Silaghi, QA [:pauly]]

Forget about comment 6. Mozregression doesn't seem to be very accurate this time.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange= bb4e11e818fe&tochange= 42996f3b6b75

Comment 8

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #7)
> Forget about comment 6. Mozregression doesn't seem to be very accurate this
> time.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=
> bb4e11e818fe&tochange= 42996f3b6b75

Link was broken, here's the corrected URL:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bb4e11e818fe&tochange=42996f3b6b75

This points squarely at bug 947227. Matt Woodrow, can you please investigate?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That bug basically reverted the async WebGL update work that was done in bug 829747.  Note that snorp asked about the exact issue that was fixed in bug 947227 in bug 829747 comment 21.

I suspect that what's happening is that now we're hitting the |if (mStaging) Scrap(mStaging);| bit -- basically, overproduction.  We have no way to abort the GPU commands from actually being executed, so the work we did to render to mStaging will still happen, even if it never makes it to the screen.  This seems consistent with the visible framerate drop.

Pretty urgently need a real fix here.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> That bug basically reverted the async WebGL update work that was done in bug
> 829747.  Note that snorp asked about the exact issue that was fixed in bug
> 947227 in bug 829747 comment 21.
> 
> I suspect that what's happening is that now we're hitting the |if (mStaging)
> Scrap(mStaging);| bit -- basically, overproduction.  We have no way to abort
> the GPU commands from actually being executed, so the work we did to render
> to mStaging will still happen, even if it never makes it to the screen. 
> This seems consistent with the visible framerate drop.
> 
> Pretty urgently need a real fix here.

Matt has a plan in bug 854421. Not sure of the status, though.

Comment 11

[u279076]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Pretty urgently need a real fix here.

Nominating for tracking.

Comment 12

[Matt Woodrow (:mattwoodrow)]

My solution is blocked on bug 974197 which got backed out due to bizarre b2g desktop UI test failures, none of which I've been able to reproduce.

I'll have another go at that, but we should probably look at simpler fixes to uplift.

My original suggestion in the deadlock bug was to add a maximum time we wait on the compositor, and just dump the frame if it is exceeded.

If we choose a high-ish value (say 500ms), then we shouldn't hit this in any reasonable use case. If the compositor really is taking half a second, then we've lost entirely already. It should give us a reasonable safety net for these deadlock situations though.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> My solution is blocked on bug 974197 which got backed out due to bizarre b2g
> desktop UI test failures, none of which I've been able to reproduce.
> 
> I'll have another go at that, but we should probably look at simpler fixes
> to uplift.
> 
> My original suggestion in the deadlock bug was to add a maximum time we wait
> on the compositor, and just dump the frame if it is exceeded.
> 
> If we choose a high-ish value (say 500ms), then we shouldn't hit this in any
> reasonable use case. If the compositor really is taking half a second, then
> we've lost entirely already. It should give us a reasonable safety net for
> these deadlock situations though.

I don't think it will help -- the issue here (I'm pretty sure) is that we've already issued the draw commands for the frame.  The fact that we time out waiting for the compositor doesn't matter; there would still a bogus will-never-be-composited webgl frame going through the pipe (because WaitForCompositor() will return false if it hits the timeout, right?).

Comment 14

[Matt Woodrow (:mattwoodrow)]

Jeff, how would you feel about the above?

Backout the current change, and instead land the uglier code to add a time limit to our blocking.

That way we can avoid shipping a regression, and we can fix this properly on m-c.

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> 
> I don't think it will help -- the issue here (I'm pretty sure) is that we've
> already issued the draw commands for the frame.  The fact that we time out
> waiting for the compositor doesn't matter; there would still a bogus
> will-never-be-composited webgl frame going through the pipe (because
> WaitForCompositor() will return false if it hits the timeout, right?).

I'm not entirely sure what you mean.

Everything you said sounds correct, and it would result in us both hanging for 500ms and throwing away a frame without compositing it, but only in very rare cases.

That seems like a big improvement over hanging indefinitely in rare cases (the old situation) or overproducing and getting poor performance in common cases (the current situation).

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh wait, I misunderstood.  With your patch we would hang instead of overproducing, and would only overproduce if the compositor took >500ms to come back to us, right?

Comment 17

[Matt Woodrow (:mattwoodrow)]

Yes, exactly. The first part of that is what we had previously.

It's obviously not ideal (we'd much rather have the main thread free to do GC, respond to input), but it's easy and safe to uplift to branches.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Tracking because of the performance regression. FYI, beta7 will go to build tomorrow (Thursday).

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Yes, exactly. The first part of that is what we had previously.
> 
> It's obviously not ideal (we'd much rather have the main thread free to do
> GC, respond to input), but it's easy and safe to uplift to branches.

Yeah this seems alright short-term, but I think the entire cause of the hang is something we probably still want to fix (bug 947227). Don't want to hang for 500ms very often...

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> > Yes, exactly. The first part of that is what we had previously.
> > 
> > It's obviously not ideal (we'd much rather have the main thread free to do
> > GC, respond to input), but it's easy and safe to uplift to branches.
> 
> Yeah this seems alright short-term, but I think the entire cause of the hang
> is something we probably still want to fix (bug 947227). Don't want to hang
> for 500ms very often...

I wasn't actually able to reproduce the original hang apart from the one time it happened to me in my normal browser.

So I'd suggest that the hangs will be rare. But if you want to try track down the issue that would be good too :)

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> > > Yes, exactly. The first part of that is what we had previously.
> > > 
> > > It's obviously not ideal (we'd much rather have the main thread free to do
> > > GC, respond to input), but it's easy and safe to uplift to branches.
> > 
> > Yeah this seems alright short-term, but I think the entire cause of the hang
> > is something we probably still want to fix (bug 947227). Don't want to hang
> > for 500ms very often...
> 
> I wasn't actually able to reproduce the original hang apart from the one
> time it happened to me in my normal browser.
> 
> So I'd suggest that the hangs will be rare. But if you want to try track
> down the issue that would be good too :)

I seem to recall having a good way to do it, but don't remember exactly. It involved switching tabs, I think. If you switch tabs while there is a frame in-flight, the compositor stops and we hang. Or something like that.

Comment 22

[u480271]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #21)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> > (In reply to James Willcox (:snorp) ([e-mail]) from comment #19)
> > > (In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> > > > Yes, exactly. The first part of that is what we had previously.
> > > > 
> > > > It's obviously not ideal (we'd much rather have the main thread free to do
> > > > GC, respond to input), but it's easy and safe to uplift to branches.
> > > 
> > > Yeah this seems alright short-term, but I think the entire cause of the hang
> > > is something we probably still want to fix (bug 947227). Don't want to hang
> > > for 500ms very often...
> > 
> > I wasn't actually able to reproduce the original hang apart from the one
> > time it happened to me in my normal browser.
> > 
> > So I'd suggest that the hangs will be rare. But if you want to try track
> > down the issue that would be good too :)
> 
> I seem to recall having a good way to do it, but don't remember exactly. It
> involved switching tabs, I think. If you switch tabs while there is a frame
> in-flight, the compositor stops and we hang. Or something like that.

I had hangs when having a webgl tab and either switching to it, or switching away from it. I'm pretty sure when the hang happened, webgl was not the active tab.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

This has missed the window for FF29 and it needs an assignee so we can ensure there's work done towards this with the possibility of fixing on 30.

Who can take this?

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Matt actually fixed this with bug 995065.

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed 31.0a1 (2014-04-22), OS X 10.9.2

Comment 26

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29.0.1
Build Id: 20140506152807

FF 30.0b3
Build Id: 20140508121358

FF 31
Build Id: 20140513004002

OS: Mac Os X 10.9.2