412526 - ActionMonkey: New, efficient JS_SUSPEND_REQUEST, JS_RESUME_REQUEST macros

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v1

In ActionMonkey, JS_SuspendRequest() is expensive because it makes a copy of the calling thread's C stack.  (I think this is necessary to protect values in callee-save registers.)

However, if suspend and resume are called from the same function, in the same C block, copying the stack can be avoided-- but only by implementing the suspend and resume functions as macros.

I propose adding new macros JS_SUSPEND_REQUEST(cx) and JS_RESUME_REQUEST(cx) to the JSAPI.  They must be called from the same C block and they must match.

The ugly part is, the macros would only work in C++ code, because the implementation uses MMgc APIs that can't be abstracted away (the MMGC_GET_STACK_EXTENTS macro).

The attached patch should illustrate what I mean.  It's not final.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v2

Eh, I take it back, it was pretty much final.  A few tweaks and unbitrotting here.

The macros still only work in C++.

Comment 2

[Benjamin Smedberg]

Comment on attachment 314443 [details] [diff] [review]
v2

per IRC, this won't work as-is because JSContext is an incomplete type for clients.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v3

Added a few JS_Internal_ functions, a kinda funny implementation detail.  Let me know if you think that cast from void* is comment-worthy.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 316453 [details] [diff] [review]
v3

By the book, you need a SpiderMonkey peer to review. I'm doing a drive-by and noting from bitter experience that "Internal_" means nothing. It does not ward off abusage. It's certainly ugly in terms of the prevailing style.

I would rather see helpers as static methods on C++ classes, in __cplusplus #ifdefs.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Oops--I will get SpiderMonkey peer review before committing.  I need Benjamin's "this works" review first.

I'll change those to static member functions in the next rev.

Comment 6

[Benjamin Smedberg]

Comment on attachment 316453 [details] [diff] [review]
v3

code inspection says this looks good. I haven't checked XPCOMGC with it yet.