609296 - Number.toString() is slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Peacekeeper's SHA1 benchmark calls Number.prototype.toString 2.000.000 times, this is 3x slower in JM than in V8. Pics output tells us why: "callprop disabled: non-string primitive"

I think CALLPROP needs a generateNumberCallStub based on generateStringCallStub? Or a generic generatePrimitiveCallStub that handles both strings and numbers.

For this benchmark:
----
function test(x) {
    var t0 = new Date;
    var s;
    for(var i=0; i<2000000; i++) {
	s = (x|0).toString();
    }
    print(new Date - t0);
}
test(10);
----
I get these numbers:

-j: 28 ms
v8: 52 ms
-m: 147 ms

So we can win at least 70 ms (5-8%) on this Peacekeeper benchmark (JM-only, don't know about JM+TM).

Comment 1

[Boris Zbarsky [:bzbarsky]]

What's TM doing right here, and can JM just do that?  ;)

Comment 2

[Jan de Mooij [:jandem]]

(In reply to comment #1)
> What's TM doing right here, and can JM just do that?  ;)

IIUC TM calls js_NumberToString{WithBase} directly (jsnum.cpp). JM calls num_toString, which does GetPrimitiveThis and then also calls NumberToStringWithBase. Maybe JM can do what TM does by special-casing num_toString.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Now that primitive wrapping is gone, we should be able to generate normal ICs here, like we do for strings.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: WIP v1

This makes the callprop string path more generic so we can use it for numbers and booleans. Some numbers:

Peacekeeper SHA1:
-m:
before: 1714 ms
after: 1623 ms

-m -j -p:
before: 1539 ms
after: 1451 ms

Micro-bench, comment 0, -m:
before: 145 ms
after: 81 ms

If I add this line:
Number.prototype.toString = function() { return 0; }

-m (before) 94 ms
-m (after): 18 ms
-j: 318 ms
V8: 4452 ms

This also wins 5 ms on v8 earley-boyer and 1-2 ms on SS unpack-code. This passes tests but I still need to do some cleanup and write tests.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Current js shell numbers for the comment #0 testcase:
Interp: 270
JM: 143
JM+TI: 130
d8: 37

jandem, is this still on your radar?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen from comment #5)
> jandem, is this still on your radar?

I'm not going to work on this, but a contributor may be interested. With TI, it should be much easier to add a fast path.

Comment 7

[Till Schneidereit [:till]]

Apparently, we got worse at this with IM:

Current js shell numbers for the comment #0 testcase:
Interp: 446
Baseline: 266
IM: 161
d8: 25

I wonder why Interp regressed this much. (Assuming we did, but I can't believe that my machine is that much slower than jandem's from 1.5 years ago *and* v8 got that huge a speed boost.)

Comment 8

[Tom S [:evilpie]]

v8 has an inline stub in crankshaft that looks up the cached string representation for numbers.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

Baseline has a stub for GETPROP on strings. This patch makes that stub work on numbers and booleans as well.

It also fixes IonBuilder so that it optimizes the number.toString singleton access.

Improves the testcase in comment 0 from 217 to 35 ms and seems to win a few ms on SS unpack-code.

Comment 10

[Brian Hackett [Laid off!]]

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

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

::: js/src/jit/BaselineIC.cpp
@@ +6183,5 @@
>      if (IsIonEnabled(cx))
> +        types::EnsureTrackPropertyTypes(cx, proto, id);
> +
> +    // For now, only look for properties directly set on the prototype.
> +    RootedShape shape(cx, proto->nativeLookupPure(id));

This can use nativeLookup instead.

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0583bb4a0b46

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0583bb4a0b46