607692 - Consider inlining parseInt for numeric (double or int) arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

When I run the attached testcase in an opt shell with -j, I get these numbers:

216
216
220
649

There are variants that show similar behavior; for example if I take out the calls to r2 I get these numbers:

215
226

whereas if I reverse the calls on lines 31 and 32 I get these numbers:

218
671

jitstats and TMFLAGS=tracer show that the slow case ends up tracing a branch off a guard on an existing tree, while all the fast cases just trace new trees.  The inconsistency is what confuses me.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This happens because when f() runs the first time the global shape changes due to us resolving parseInt, so we flush all trees...

I really wish we did a better job of LRUing tree branches or something, though.  :(

Is this still valid?  My gut feeling is no.

Comment 2

[Brendan Eich [:brendan]]

Wonder if we should take the hit when starting a trace recording of calling JS_EnumerateStandardClasses(cx, globalObj). Easy patch. Don't know how bad the hit is, though. Boris, can you find out?

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #1)
> I really wish we did a better job of LRUing tree branches or something, though.
>  :(

Is there a bug on this idea?

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Wonder if we should take the hit when starting a trace recording of calling
> JS_EnumerateStandardClasses(cx, globalObj).

What would be the benefit?  Making sure some later code doesn't throw away the tree we're about to compile?

Note that in this case it would consistently make the second function run slower than the first one....  ;)

Comment 5

[Brendan Eich [:brendan]]

Ultimately I think we can use a "root context" with initialized standard objects in their own compartment to do a very fast copy under JS_NewGlobalObject, and make JS_*StandardClass* APIs stubs.

Just calling JS_EnumerateStandardClasses when recording isn't enough, you're right. We would want to call that before attempting to trigger a trace too. Would that make the second function run fast?

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Would that make the second function run fast?

No.  The only thing that makes it run fast in the attached testcase for the second number is that we've changed the global shape and nixed the old trace, so when we record we record a trace specialized for the current function instead of taking the branch exit and gluing on a new branch.

> Is there a bug on this idea?

It's come up in bug 504881, sorta.  And the fact that glued-on branches taken via branch exit are a lot slower is what bug 509069 is about.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Still some pretty interesting behavior even with JM+TI and v8:
Interp:	27192	29449	27217	29474
JM:	1182	4625	1181	4718
JM+TI:	90	4170	89	4171
d8:	137	3095	136	3090

Comment 8

[Boris Zbarsky [:bzbarsky]]

_That_ just shows that calling parseInt is much slower than doing the (i - rest) / 3 dance, esp when "i" is a constant.

I wonder whether we can inline parseInt sanely for arguments that are already double or int.  May not be worth worrying about until IonMonkey, of course.

Comment 9

[Tom S]

Should be easy, will do.

Comment 10

[Tom S]

Attachment: v1

This seems to help, but as already noted the testcase in comment 1 is very wild.

With this patch:
64
8454 <- what?!
63
171
Without:
63
14527
64
14752

So without this very weird case we have 14752 vs 171, not bad :)

Comment 11

[Tom S]

Ignore this, real numbers are:

Without patch:
tom@tom-linux:~/inbound/js/src/build-opt/shell$ ./js -m -n test2.js
66
1881
64
1867

With patch:
tom@tom-linux:~/inbound/js/src/build-opt/shell$ ./js -m -n test2.js
69
171
63
171

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 584625 [details] [diff] [review]
v1

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

::: js/src/jsnum.cpp
@@ +371,1 @@
>  num_parseInt(JSContext *cx, uintN argc, Value *vp)

Can you use js::num_parseInt instead and avoid the namespace js {} ?

Comment 13

[Tom S]

Not sure why, I always forget about the js::

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

Comment 14

[Ed Morley [:emorley]]

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