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
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Luke Wagner [:luke]
:
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 Luke Wagner [:luke] 2011-09-06 09:33:03 PDT
Created attachment 558497 [details] [diff] [review]
rm

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

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

Thanks!
Comment 5 Luke Wagner [:luke] 2011-09-06 12:01:17 PDT
Fix bustage from missing LeaveTrace:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8218f2ac3ce6
Comment 6 :Ehsan Akhgari 2011-09-07 07:47:22 PDT
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 :Ehsan Akhgari 2011-09-07 08:37:11 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/787ebf02a8c1 as a followup.

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