Closed Bug 797131 Opened 13 years ago Closed 13 years ago

IonMonkey: Optimize calls from C++ to Ion-compiled functions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(3 files)

Bug 581893 added the (in?)famous gatling gun for repeated calls from C++ to JS. It was removed again last year in bug 674998. Now with IonMonkey it's much simpler to do something similar for Ion-compiled functions. This should win at least 2 ms on SS string-tagcloud on AWFY, so it seems worth doing. Self-hosting also aims to make these functions (array.sort, array.map etc) faster, but we want to land this in the meantime to avoid regressing SS in FF18.
Based on the grief of the gatling gun, my only recommendation (which I suspect will be forced upon you by Ion's use of the C stack) is to not leave some lingering frame pushed between invocations. Is the idea to build a prototypical Ion activation record so that you can just push+memcpy and call?
Some simple changes I need for other patches.
Attachment #667467 - Flags: review?(dvander)
Adds FastInvokeGuard (better name welcome) and uses it for Array.sort, String.replace and Array extras. Does not yet have a fast path for Ion functions.
Attachment #667540 - Flags: review?(dvander)
(In reply to Luke Wagner [:luke] from comment #1) > Based on the grief of the gatling gun, my only recommendation (which I > suspect will be forced upon you by Ion's use of the C stack) is to not leave > some lingering frame pushed between invocations. Is the idea to build a > prototypical Ion activation record so that you can just push+memcpy and call? Yeah we don't push any frames on the JS stack. There will be a single IonActivation, and for every call we can just call the EnterJit thunk, which will push the arguments and enter the script.
Attachment #667467 - Flags: review?(dvander) → review+
Attachment #667540 - Flags: review?(dvander) → review+
This could use a careful review since I don't know the StackIter and frame code very well. Also let me know if anything is not clear or could use a comment.
Attachment #667843 - Flags: review?(dvander)
Green on OS X/Linux Try: https://tbpl.mozilla.org/?tree=Try&rev=5869b035b84b Will test other platforms later.
Comment on attachment 667843 [details] [diff] [review] Part 3 - Fast path for calling into Ion Review of attachment 667843 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/Bailouts.cpp @@ +410,5 @@ > if (cx->runtime->hasIonReturnOverride()) > cx->regs().sp[-1] = cx->runtime->takeIonReturnOverride(); > > if (retval != BAILOUT_RETURN_FATAL_ERROR) { > + if (activation->entryfp()) { How can we have a bailout with no entryfp? ::: js/src/ion/IonCompartment.h @@ +229,5 @@ > } > + bool empty() const { > + // If we have an entryfp, this activation is active. However, if > + // FastInvoke is used, entryfp may be NULL and a non-NULL prevpc > + // indicates this activation is not empty. good idea ::: js/src/jsinterpinlines.h @@ +1022,5 @@ > > bool invoke(JSContext *cx) { > +#ifdef JS_ION > + if (useIon_ && fun_) { > + HandleScript script = fun_->script(); Check with the GC folks - I *think* this HandleScript pattern (that is elsewhere in the engine too) is actually invalid. I don't know if there is an alternative yet, but it's probably best to just use RootedScript. You could make the RootedScript part of FastInvokeGuard, if it's stacky.
Attachment #667843 - Flags: review?(dvander) → review+
Err scratch that first question, it makes sense now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/43849b4e3cc0 https://hg.mozilla.org/integration/mozilla-inbound/rev/edfcc6ffb63e https://hg.mozilla.org/integration/mozilla-inbound/rev/20fe6e539f7f Broke ARMv6 on Try, had to add an #ifdef JS_ION and did another Linux32 + ARMv6 try run just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=22e9b9149e8e (In reply to David Anderson [:dvander] from comment #7) > > I don't know if there is > an alternative yet, but it's probably best to just use RootedScript. You > could make the RootedScript part of FastInvokeGuard, if it's stacky. Thanks, I added a RootedScript to FastInvokeGuard.
We decided to disable part 3 for now, to fix these fuzz bugs and avoid topcrashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3afcd4f3cc0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 798870
Should this be [leave open]? I think jandem disabled it..
Whiteboard: [ion:t] → [ion:t][leave open]
(leaving open)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This got fixed by the patch for bug 798823.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [ion:t][leave open] → [ion:t]
For the record (b/c I can't find it above) what was the speedup on the case in comment 0? Also: what about on a micro-benchmark (say, forEach)?
(In reply to Luke Wagner [:luke] from comment #16) > For the record (b/c I can't find it above) what was the speedup on the case > in comment 0? It should be at least 2-3 ms on string-tagcloud on AWFY, we will know for sure when it's updating again. > > Also: what about on a micro-benchmark (say, forEach)? Good question. For the forEach micro-benchmark below I get: Before: 133 ms After : 49 ms d8 : 47 ms jsc : 18 ms So we're 2.7x faster than before, about as fast as V8. JSC is still a lot faster though. function test(a) { var t = new Date; a.forEach(function(x) { return x + 1 }); print(new Date - t); } var a = []; for (var i=0; i<1000000; i++) a.push(i); test(a);
Thanks!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: