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

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 558497 [details] [diff] [review]
rm

Brian, when this merges to jm, could you remove that bool parameter to functionPrologue?
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #558497 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #558497 - Flags: review? → review?(bhackett1024)
Comment on attachment 558497 [details] [diff] [review]
rm

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

Thanks!
Attachment #558497 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d6242989d95d
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
(Assignee)

Comment 5

6 years ago
Fix bustage from missing LeaveTrace:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8218f2ac3ce6
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.  :(
Whiteboard: [inbound]
Pushed http://hg.mozilla.org/mozilla-central/rev/787ebf02a8c1 as a followup.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.