Consider inlining parseInt for numeric (double or int) arguments

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsperf])

Attachments

(2 attachments)

Created attachment 486378 [details]
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.
Whiteboard: [jsperf]
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.
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
(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
> 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....  ;)
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
> 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.

Updated

7 years ago
Keywords: perf
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
Summary: Odd tracer behavior in this testcase → Odd JIT behavior in this testcase
_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.
Summary: Odd JIT behavior in this testcase → Consider inlining parseInt for numeric (double or int) arguments
Should be easy, will do.
Assignee: general → evilpies
Created attachment 584625 [details] [diff] [review]
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 :)
Attachment #584625 - Flags: review?(bhackett1024)
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
Status: NEW → ASSIGNED
Blocks: 467263
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 {} ?
Attachment #584625 - Flags: review?(bhackett1024) → review+
Not sure why, I always forget about the js::

https://hg.mozilla.org/integration/mozilla-inbound/rev/db8ea6327311
https://hg.mozilla.org/mozilla-central/rev/db8ea6327311
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.