832576 - WebGL uniform setters got 3-4x slower due to refactoring

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk (but it's a problem in 20 and 19 as well).

STEPS TO REPRODUCE:
1)  Load the URL in the URL field in Firefox 18.
2)  Load the URL in the URL field in current nightly.

EXPECTED RESULTS: Nightly is faster, or at least no slower.

ACTUAL RESULTS: Nightly is 3-4x slower, pull out profiler.

Profiler says there's a ton of time in string manipulation, malloc, free.  In particular, well over 2/3 of the time is under ValidateUniformArraySetter doing all those things.

If we actually care about these functions being fast (and I had been given to understand we do), we should at the very least nix the string manipulation.  These strings are always the same; we should be computing them _once_ and then using them.  In my opinion.

It might also be a good idea to inline ValidateUniformArraySetter and/or ValidateUniformLocation; once we fix the string mess we should remeasure/reprofile and see if that's needed.

It would further be a good idea to add some Talos tests for WebGL so this sort of thing doesn't regress again...

Benoit, Saurabh, do you have time to look into this sometime this coming week?  If not, I will.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

So, Vlad does run perf tests regularly and this is part of what he runs, but I suppose that he started after this landed.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ouch, so I *was* the reviewer for this. oops, my bad.

At least, it's an  easy fix. There's no reason for us to do this string manipulation here, it's only used for generating error messages so it should only be done in error cases, and plain literal strings + printf-like format characters should be enough anyway.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix uniform setters

In defense of the faulty patch -- the intention, to make error messages more specific, was really good. Still, doing this right (without slowness) was nontrivial enough (due to other functions around here work) that I think it's not worth it. This reverts the error messages to showing the function name without specifically ": location" appended to it.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Also, this patch moves the declaration of |info| further down, where it's used.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That helps a good bit.  The result is still about 25% slower than Fx18 over here, though....

A profile only shows the ValidateUniformSetter calls taking about 10% of the time, though maybe some of the time they take is in the register spills and restores in the caller.

So it's possible this 25% regression is at least partially from something else.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ccf23b56150e

Filing a follow-up about the remaining regression.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 704171 [details] [diff] [review]
fix uniform setters

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 745840
User impact if declined: performance regression in WebGL
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): not risky, very simple patch basically just removing code
String or UUID changes made by this patch: none

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Filed bug 834782 for the remaining regression.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 704171 [details] [diff] [review]
fix uniform setters

Assuming this sticks on m-c, approving for m-a as well in preparation for landing to m-b.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out speculatively as the possible cause of android reftest-4 failures (for lack of any better guesses that I could find):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a25c052c5a

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f7a25c052c5a shows it's still failing android reftest-4 in the same way.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

(Btw, this failure is in reftest-svg, so there was little chance that a WebGL-only patch could be relevant to it)

Comment 13

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/91b952220896

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91b952220896

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a83f24162110

Comment 16

[bhavana bajaj [:bajaj]]

Comment on attachment 704171 [details] [diff] [review]
fix uniform setters

Please make sure to land on beta no later than EOD today/early tomorrow morning to make sure it gets in FF19 beta 4, targeting go-to-build tomorrow . Thanks !

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e5dbf1c67c2d

Comment 18

[Ioana (away)]

Verified fixed on Firefox 19 beta 4 (20130130080006):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0

According to the performance tests linked in the URL section, Fx19b4 works quite the same as Fx 18.0.1.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> According to the performance tests linked in the URL section, Fx19b4 works quite the same
> as Fx 18.0.1.

That's very interesting.  Does trunk?  If not, that means the additional 25% regression I see on trunk postdates the 19 branch...

Comment 20

[Ioana (away)]

(In reply to Boris Zbarsky (:bz) from comment #19)
> > According to the performance tests linked in the URL section, Fx19b4 works quite the same
> > as Fx 18.0.1.
> 
> That's very interesting.  Does trunk?  If not, that means the additional 25%
> regression I see on trunk postdates the 19 branch...

On Nightly, I get pretty much the same medians as on 18.0.1, but I never get standard deviations larger than 3ms. On 18.0.1 and beta, I get standard deviations of 7ms quite often.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ioana, which platform(s) are you seeing that on?  Trying to figure out why we see different things....

Comment 22

[Ioana (away)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Ioana, which platform(s) are you seeing that on?  Trying to figure out why
> we see different things....

The same as in comment 18: Mac OSX 10.8. To be more specific, I use Mac OSX 10.8.2 and start Firefox in 32bit mode.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Huh.  I'm on 10.8 as well.  What happens for you in 64-bit mode?  I just retested and I'm still seeing a 10% or so regression on trunk vs 18.....

Comment 24

[Ioana (away)]

Here is the behavior I get now on both 32bit and 64bit (Mac 10.7 and Mac 10.8): for most test runs, I get standard deviations of 3-5ms. On the first or second load of the test right after starting Firefox, I get high values (8ms to 182ms, lots of 72-78ms). I never got high values on going further.

This same behavior is displayed on Firefox 18.0.2 and the 02/06 Nightly.

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK.  Thank you for checking that!