Last Comment Bug 607692 - Consider inlining parseInt for numeric (double or int) arguments
: Consider inlining parseInt for numeric (double or int) arguments
Status: RESOLVED FIXED
[jsperf]
: perf
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks: 467263
  Show dependency treegraph
 
Reported: 2010-10-27 11:20 PDT by Boris Zbarsky [:bz]
Modified: 2012-01-04 17:28 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (445 bytes, text/plain)
2010-10-27 11:20 PDT, Boris Zbarsky [:bz]
no flags Details
v1 (9.23 KB, patch)
2011-12-28 13:11 PST, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2010-10-27 11:20:09 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2010-10-27 11:26:19 PDT
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] 2010-10-27 13:26:47 PDT
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] 2010-10-27 13:27:47 PDT
(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 [:bz] 2010-10-27 13:35:16 PDT
> 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] 2010-10-27 14:17:59 PDT
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 [:bz] 2010-10-27 14:22:16 PDT
> 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] 2011-11-25 22:59:55 PST
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 [:bz] 2011-11-26 12:12:59 PST
_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 Schuster [:evilpie] 2011-11-26 12:15:40 PST
Should be easy, will do.
Comment 10 Tom Schuster [:evilpie] 2011-12-28 13:11:23 PST
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 :)
Comment 11 Tom Schuster [:evilpie] 2011-12-28 13:42:15 PST
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 (:bhackett) 2012-01-02 07:08:09 PST
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 Schuster [:evilpie] 2012-01-04 13:00:48 PST
Not sure why, I always forget about the js::

https://hg.mozilla.org/integration/mozilla-inbound/rev/db8ea6327311
Comment 14 Ed Morley [:emorley] 2012-01-04 17:28:06 PST
https://hg.mozilla.org/mozilla-central/rev/db8ea6327311

Note You need to log in before you can comment on or make changes to this bug.