Closed
Bug 832576
Opened 12 years ago
Closed 12 years ago
WebGL uniform setters got 3-4x slower due to refactoring
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: bzbarsky, Assigned: bjacob)
References
()
Details
(Keywords: perf, regression)
Attachments
(1 file)
(deleted),
patch
|
jgilbert
:
review+
akeybl
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
So, Vlad does run perf tests regularly and this is part of what he runs, but I suppose that he started after this landed.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
Also, this patch moves the declaration of |info| further down, where it's used.
Reporter | ||
Comment 5•12 years ago
|
||
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.
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #704171 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 6•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ccf23b56150e
Filing a follow-up about the remaining regression.
Assignee: nobody → bjacob
Target Milestone: --- → mozilla21
Assignee | ||
Comment 7•12 years ago
|
||
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?
Assignee | ||
Comment 8•12 years ago
|
||
Filed bug 834782 for the remaining regression.
Comment 9•12 years ago
|
||
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+
Comment 10•12 years ago
|
||
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
Assignee | ||
Comment 11•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f7a25c052c5a shows it's still failing android reftest-4 in the same way.
Assignee | ||
Comment 12•12 years ago
|
||
(Btw, this failure is in reftest-svg, so there was little chance that a WebGL-only patch could be relevant to it)
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
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+
Assignee | ||
Comment 17•12 years ago
|
||
Comment 18•12 years ago
|
||
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
Reporter | ||
Comment 19•12 years ago
|
||
> 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•12 years ago
|
||
(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.
Reporter | ||
Comment 21•12 years ago
|
||
Ioana, which platform(s) are you seeing that on? Trying to figure out why we see different things....
Comment 22•12 years ago
|
||
(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.
Reporter | ||
Comment 23•12 years ago
|
||
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•12 years ago
|
||
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.
Reporter | ||
Comment 25•12 years ago
|
||
OK. Thank you for checking that!
You need to log in
before you can comment on or make changes to this bug.
Description
•