674998 - someday we should rip out InvokeSessionGuard and self-host its 3 uses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

The InvokeSessionGuard optimizes repeated Invoke calls for the same function.  It is only applied in 3 places: String replace (given a function), Array sort, and Array extras (like map).  Measuring again just now for a simple loop that sorts a 10000 element array 10000 times with the comparator (function(x,y){return x-y}), InvokeSessionGuard is a 57% speedup.

The problem is that InvokeSessionGuard is kindof a terrible hack in that it pushes a stack frame and keeps it there even when the function returns.  In the interest of going fast, we also try to do the minimal work to reset this frame.  Getting this wrong has caused several bugs.

Ideally, we'd just self-host these library functions so that it was JS calling JS which is fast and getting faster.  v8 self hosts replace and is 30% faster for a simple benchmark.  (How does it get around the garbage problem in bug 460904 (which tried to self host replace)?  By reusing a single array that holds the replace results and is passed as an outparm to the regexp engine.)

OTOH, v8 is 6x slower on sort (which it completely implements in JS).  But in this case, we could just take the 50% hit taking out InvokeSessionGuard and still be way faster.

Comment 1

[Luke Wagner [:luke]]

That's it, this keeps coming up and making code more complex than it needs to be.  I'm going to remove this.  SS will take a <1ms hit, V8 doesn't use it, we'll still be 3x faster on sort, and we need to self-host replace(lambda) anyway.

Comment 2

[Luke Wagner [:luke]]

Attachment: rm

Brian, when this merges to jm, could you remove that bool parameter to functionPrologue?

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 558497 [details] [diff] [review]
rm

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

Thanks!

Comment 4

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d6242989d95d

Comment 5

[Luke Wagner [:luke]]

Fix bustage from missing LeaveTrace:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8218f2ac3ce6

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d6242989d95d
http://hg.mozilla.org/mozilla-central/rev/8218f2ac3ce6

During the merge to mozilla-central, I got one conflict in jsinterp.cpp and one in jsinterpinlines.h, both regarding the removed InvokeSessionGuard class.  I added them back before I saw this bug, but now I think that was the wrong thing to do.

Can someone who knows what's going on here look at http://hg.mozilla.org/mozilla-central/file/0c7303e897c5/js/src/jsinterp.cpp#l686 and http://hg.mozilla.org/mozilla-central/file/0c7303e897c5/js/src/jsinterpinlines.h#l74 and see if we need to remove these two hunks again?

Sorry for doing the stupid thing during the merge.  :(

Comment 7

[(no longer active)]

Pushed http://hg.mozilla.org/mozilla-central/rev/787ebf02a8c1 as a followup.