Closed Bug 832576 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics: CanvasWebGL, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- unaffected
firefox19 + verified
firefox20 + fixed
firefox21 + fixed

People

(Reporter: bzbarsky, Assigned: bjacob)

References

()

Details

(Keywords: perf, regression)

Attachments

(1 file)

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.
Keywords: perf
So, Vlad does run perf tests regularly and this is part of what he runs, but I suppose that he started after this landed.
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.
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.
Attachment #704171 - Flags: review?(jgilbert)
Also, this patch moves the declaration of |info| further down, where it's used.
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.
Attachment #704171 - Flags: review?(jgilbert) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/ccf23b56150e

Filing a follow-up about the remaining regression.
Assignee: nobody → bjacob
Target Milestone: --- → mozilla21
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
Attachment #704171 - Flags: approval-mozilla-beta?
Attachment #704171 - Flags: approval-mozilla-aurora?
Blocks: 834782
Filed bug 834782 for the remaining regression.
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.
Attachment #704171 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f7a25c052c5a shows it's still failing android reftest-4 in the same way.
(Btw, this failure is in reftest-svg, so there was little chance that a WebGL-only patch could be relevant to it)
https://hg.mozilla.org/mozilla-central/rev/91b952220896
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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 !
Attachment #704171 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
QA Contact: ioana.budnar
> 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...
(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.
Ioana, which platform(s) are you seeing that on?  Trying to figure out why we see different things....
(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.
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.....
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.
OK.  Thank you for checking that!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: