928423 - IonMonkey: GetElementIC GetProp stub doesn't work well with non-atomized strings

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

GetElementIC::attachGetProp guards the input index Value is equal to the atomized index value. This works great if the index is always an atomized string, but doesn't work well for non-atomized strings: the index is the same string but not atomized so a different JSString pointer and the stub fails.

I noticed this on Peacekeeper's domJQueryBasicFilters test. Here's a small testcase:

function f() {
    var obj = {foo: 1, bar: 2};
    for (var i=0; i<100000; i++) {
	var res = obj["foo1".substr(0, 3)];
    }
}
var t = new Date;
f();
print(new Date - t);

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch compares the strings in the IC stub and seems to get rid of most of the GetElementIC::update calls I was seeing on the jQuery benchmark. For the micro-benchmark in comment 0:

js new:  6 ms
d8:     10 ms
js old: 16 ms

Comment 2

[Eric Faust [:efaust]]

Comment on attachment 821155 [details] [diff] [review]
Patch

Review of attachment 821155 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. r=me

::: js/src/jit-test/tests/ion/bug928423.js
@@ +1,2 @@
> +function f(o, p) {
> +    try {} catch(e) {};

I don't understand what this is doing here? If we compile it, then nothing, and if we don't then it forces us out of ion?

::: js/src/jit/IonCaches.cpp
@@ +3059,5 @@
> +    masm.movePtr(ImmGCPtr(name), objReg);
> +    masm.passABIArg(objReg);
> +    masm.unboxString(val, scratch);
> +    masm.passABIArg(scratch);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, EqualStringsHelper));

nit: The style elsewhere in the file is to fills the arg registers first, and then setupUnalignedABICall(); passABIArg() ... and callWithABI() all in quick succession.

It's a little annoying here because it requires another register. In the past, we have taken a random one from volatileRegs. It's up to you if you think it's worth it.

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c37138aadb0c

Thanks for the quick review.

(In reply to Eric Faust [:efaust] from comment #2)
> I don't understand what this is doing here? If we compile it, then nothing,
> and if we don't then it forces us out of ion?

try-catch is compiled but not inlined. It's a bit subtle and may change at some point; we should probably run jit-tests with --ion-inlining=off too..

> It's a little annoying here because it requires another register. In the
> past, we have taken a random one from volatileRegs. It's up to you if you
> think it's worth it.

Yeah and we also have to make sure the extra register != scratch and objReg. I think not requiring an extra register is simpler here.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I think it is this which helped quite a bit with
Dromaeo DOM
http://graphs.mozilla.org/graph.html#tests=[[73,131,25]]&sel=1382542160000,1382714960000

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, indeed.  See http://perf.snarkfest.net/compare-talos/index.html?oldRevs=dc598f50a09a&newRev=c37138aadb0c&submit=true which is a comparison of the last push before this one that did sane talos tests and this push.

In particular, we saw 7-10% wins on dromaeo-dom and 2-3% wins on dromaeo-css.

http://perf.snarkfest.net/compare-talos/breakdown.html?oldTestIds=30785241&newTestIds=30785427&testName=dromaeo_dom says that in particular dom-attr got a lot faster (16% or so), which is almost certainly due to this test:

	test( "element.expando", function(){
		for ( var i = 0; i < num; i++ )
			ret = a["test" + num];
	});

getting way faster.

And in fact, here's a dom-attr test in the Oct 24 nightly (left column) and in the first build with this change (right column): <http://dromaeo.com/?id=208043,208044>.  Note that the score for the element.expando test went from ~750 to ~2000.  ;)

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c37138aadb0c