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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ion:t])
Attachments
(3 files)
3.43 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
15.26 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
13.38 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Comment 1•13 years ago
|
||
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?
![]() |
||
Updated•13 years ago
|
Whiteboard: [ion:t]
Assignee | ||
Comment 2•13 years ago
|
||
Some simple changes I need for other patches.
Attachment #667467 -
Flags: review?(dvander)
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
(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.
![]() |
||
Updated•13 years ago
|
Attachment #667467 -
Flags: review?(dvander) → review+
![]() |
||
Updated•13 years ago
|
Attachment #667540 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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.
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43849b4e3cc0
https://hg.mozilla.org/mozilla-central/rev/edfcc6ffb63e
https://hg.mozilla.org/mozilla-central/rev/20fe6e539f7f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Assignee | ||
Comment 11•13 years ago
|
||
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 → ---
Comment 12•13 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
![]() |
||
Comment 13•13 years ago
|
||
Should this be [leave open]? I think jandem disabled it..
![]() |
||
Updated•13 years ago
|
Whiteboard: [ion:t] → [ion:t][leave open]
![]() |
||
Comment 15•13 years ago
|
||
This got fixed by the patch for bug 798823.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [ion:t][leave open] → [ion:t]
![]() |
||
Comment 16•13 years ago
|
||
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)?
Assignee | ||
Comment 17•13 years ago
|
||
(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);
![]() |
||
Comment 18•13 years ago
|
||
Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•