Leon Sorokin Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:14.0) Gecko/20120418 Firefox/14.0a1
Build ID: 20120418052015

Steps to reproduce:

there seems to be a significant regression in CanvasPixelArray perfomance

FF nightly currently shows > 50% performance degradation for the direct pixel manip vs when aliased/scoped locally. FF 11 stable as well as Opera and Chrome only show about a 5% difference.

http://jsperf.com/pixel-pre-rendering/5

maybe an ion JIT miss?

	Alice0775 White
	Comment 1 • 12 years ago

Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/e5f6caa40409
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120315 Firefox/14.0a1 ID:20120315214227
Bad:
http://hg.mozilla.org/mozilla-central/rev/ee56787a20fb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120316 Firefox/14.0a1 ID:20120316025529
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5f6caa40409&tochange=ee56787a20fb

Suspected:Bug 550309

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

Yep, almost certainly due to the .data getter getting slower now that it's following all the WebIDL rules instead of just being a native JS get.

I'm going to test what happens if I use new bindings here.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

But no matter what, it'll be slower than it used to be and quite noticeably so, up until the JS engine manages to CSE this or something.  No idea whether there are any plans for that.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

And it's possible that we just need to back out bug 550309 until we can make this better.  :(

If so, we need to make that call soon, not after we've shipped some betas, since this is an interface change (albeit not one likely to mess up binary addons).

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

On the other hand, anyone who really cares about performance is probably caching the .data anyway....

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

The Codegen.py change is bogus because the value may not be object; if doing this right we would write codegen for returning a typed array.

With this patch and the patch to IC jsnative getters, the version that keeps getting .data is about 1.5x faster (not least because it avoids the JS_WrapValue).  It's still about 2x slower than the version that does NOT get .data over and over; lots of time spent in the gray-marking, compartment checks, binding overhead, etc, etc.

I _really_ wish there were a way to teach the JIT here that the getter is idempotent...

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

Oh, and with that last patch we're spending at least 35% of our time under that getter.  That's not counting whatever jitcode is needed to call into it.  :(

	David Mandelin [:dmandelin]
	Comment 8 • 12 years ago

Setting tracking because it seems important. bz, do you think we have time to attempt optimizing this better for 14, or is it more just about deciding whether to back out bug 550309?

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 9 • 12 years ago

Throwing this over to Ms2ger to determine if a backout of bug 550309 will happen, and to do the backout if required.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 10 • 12 years ago

Backing out 550309? No way! That was a long overdue conformance fix and backing out would regress WebGL conformance at a time when we're getting ready to announce conformance. The only way forward is to fix the perf regression.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 11 • 12 years ago

Also, comment 5 another reason why I don't worry much about this.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 12 • 12 years ago

[Triage Comment]
Removing tracking if comment 5 and comment 10 are saying there's not too much concern about this particular performance metric from the user perspective and a backout is not an option.  If there's a case for continuing to track this for potential uplift once 14 goes to the Beta channel tomorrow, please renominate.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 13 • 12 years ago

I'm not working on this; bug 762654 will speed us up again.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 12 years ago

So I'm going to convert this stuff to WebIDL, which will make it about 2.5x faster.  That will still leave us 3x slower than 

But if we fix bug 747289 then it will _really_ get fast.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 12 years ago

Er, that should have been "3x slower than Fx13".

I filed bug 801712 on WebIDL here.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

In a current nightly the original testcase for this bug is within the noise level (1% or so) for the two ways of doing it.

This is fixed, for practical purposes.