Last Comment Bug 674998 - someday we should rip out InvokeSessionGuard and self-host its 3 uses
: someday we should rip out InvokeSessionGuard and self-host its 3 uses
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-07-28 12:00 PDT by Luke Wagner [:luke]
Modified: 2011-09-07 08:37 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

rm (21.77 KB, patch)
2011-09-06 09:33 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Luke Wagner [:luke] 2011-07-28 12:00:20 PDT
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 User image Luke Wagner [:luke] 2011-09-02 17:39:42 PDT
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 User image Luke Wagner [:luke] 2011-09-06 09:33:03 PDT
Created attachment 558497 [details] [diff] [review]

Brian, when this merges to jm, could you remove that bool parameter to functionPrologue?
Comment 3 User image Brian Hackett (:bhackett) 2011-09-06 10:21:36 PDT
Comment on attachment 558497 [details] [diff] [review]

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

Comment 5 User image Luke Wagner [:luke] 2011-09-06 12:01:17 PDT
Fix bustage from missing LeaveTrace:
Comment 6 User image :Ehsan Akhgari 2011-09-07 07:47:22 PDT

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 and and see if we need to remove these two hunks again?

Sorry for doing the stupid thing during the merge.  :(
Comment 7 User image :Ehsan Akhgari 2011-09-07 08:37:11 PDT
Pushed as a followup.

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