Closed Bug 674998 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)



(1 file)

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.
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.
Attached patch rmSplinter Review
Brian, when this merges to jm, could you remove that bool parameter to functionPrologue?
Assignee: general → luke
Attachment #558497 - Flags: review?
Attachment #558497 - Flags: review? → review?(bhackett1024)
Comment on attachment 558497 [details] [diff] [review]

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

Attachment #558497 - Flags: review?(bhackett1024) → review+
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Fix bustage from missing LeaveTrace:

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.  :(
Whiteboard: [inbound]
Pushed as a followup.
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.