Last Comment Bug 581263 - remove slow natives
: remove slow natives
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
Mentors:
: 523459 (view as bug list)
Depends on: 579471 589028 592962 593277 593473 593557 593559 593611 595923 599446
Blocks: 539144 587725 601505 608324
  Show dependency treegraph
 
Reported: 2010-07-22 19:27 PDT by Luke Wagner [:luke]
Modified: 2010-12-22 10:03 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fast ctor patch ported to TM (28.73 KB, patch)
2010-08-10 13:32 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Review
WIP (220.95 KB, patch)
2010-08-13 18:15 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
WIP 2 (243.90 KB, patch)
2010-08-14 19:13 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
WIP 3 (422.46 KB, patch)
2010-08-17 17:12 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
xpc changes (92.78 KB, patch)
2010-08-17 18:46 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
remove slow natives in !xpconnect (323.52 KB, patch)
2010-08-17 19:20 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Review
xpc changes (85.57 KB, patch)
2010-08-20 09:31 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2010-07-22 19:27:17 PDT
Brendan says its been long enough and we can remove slow natives.  Bug 579471 looks to remove the biggest remaining source of slow natives: constructors.  The patch would mostly be removing code and rewriting remaining slow natives in JS and the browser.

Its unclear whether this would any real speedup so this may not be a high priority atm, but we should definitely do this sometime.
Comment 1 Brendan Eich [:brendan] 2010-07-22 19:43:04 PDT
We would need the compartments and wrappers stuff, IINM, to do without certain native methods' frames.

/be
Comment 2 Brendan Eich [:brendan] 2010-07-22 19:43:56 PDT
So mark dependencies accordingly.

/be
Comment 3 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-07-23 00:18:08 PDT
How will constructors know if they've been called as a constructor: |new name()| or as a function: |name()|?
Comment 4 Andreas Gal :gal 2010-07-23 00:20:54 PDT
JS_IsConstructing(cx) and a flag in cx?
Comment 5 Luke Wagner [:luke] 2010-07-23 07:45:58 PDT
JSFRAME_CONSTRUCTING in fp->flags
Comment 6 Brendan Eich [:brendan] 2010-07-23 08:12:39 PDT
No frame for fast natives, is comment 5 meant to address a pigeon-hole problem in comment 4's idea?

If a fast native ctor reenters the interpreter on its context, it'll need a new call stack (segment -- Luke knows all), which will have to save the cx flag and clear it.

/be
Comment 7 Luke Wagner [:luke] 2010-07-23 09:18:23 PDT
(In reply to comment #6)
> No frame for fast natives, is comment 5 meant to address a pigeon-hole problem
> in comment 4's idea?

No, comment 5, when written, was retarded :)  But now that I think about the cx flag, I get the that creepy "more state in the context -- what can I assume about the context again?" feeling.  So perhaps the "constructing" flag itself can hang off cx->currentSegment->flags, taking advantage of the property comment 6 mentioned.  (In the future, btw, I'd like to move more state out of cx and into CallStackSegment.)
Comment 8 Brendan Eich [:brendan] 2010-07-23 09:21:42 PDT
(In reply to comment #7)
> So perhaps the "constructing" flag itself
> can hang off cx->currentSegment->flags, taking advantage of the property
> comment 6 mentioned.  (In the future, btw, I'd like to move more state out of
> cx and into CallStackSegment.)

+1

No speed-demon reason to put the flag in the cx, and yeah: more like this would undump the cx dumping ground.

/be
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-23 09:33:05 PDT
Stay on target...
Comment 10 Brendan Eich [:brendan] 2010-07-23 09:41:29 PDT
bhackett's patch for bug 579471 avoids any cx or stack segment flag by passing a magic js::Value as |this| to the fast native ctor, but it doesn't maintain JS API compatibility. To keep JS_IsConstructing working we'd need the flag, unless I'm missing something.

/be
Comment 11 Igor Bukanov 2010-07-23 10:07:01 PDT
(In reply to comment #6)
> If a fast native ctor reenters the interpreter on its context, it'll need a new
> call stack (segment -- Luke knows all), which will have to save the cx flag and
> clear it.

The documentation can state that the flag must be read as the first thing that the function does. Since the code has to be updated for a new signature in any case I see no point in maintaining the precise semantics of JS_IsConstructing.
Comment 12 Brendan Eich [:brendan] 2010-07-23 10:16:00 PDT
API has to be foolproof. Changing signature doesn't help there.

If we could see the magic |this| on the stack, we could avoid the flag, though.

/be
Comment 13 Igor Bukanov 2010-07-23 10:59:17 PDT
(In reply to comment #12)
> API has to be foolproof. Changing signature doesn't help there.

What about separating the call and constructor and require that an embedding supplies both the constructor and the call version of the native? This would avoid a flag check in the constructor.
Comment 14 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-07-23 11:05:10 PDT
Separate call and construct are sauce for the proxy, indeed...
Comment 15 Brendan Eich [:brendan] 2010-07-23 12:01:40 PDT
(In reply to comment #14)
> Separate call and construct are sauce for the proxy, indeed...

FYI, from http://wiki.ecmascript.org/doku.php?id=harmony:proxies#feedback under "TC39 Meeting 3/24 (Apple)":

"There was consensus that the constructTrap argument to Proxy.createFunction can be optional. If absent, new aFunctionProxy() calls the [[Construct]] built-in method of the callTrap."

/be
Comment 16 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-07-23 12:04:06 PDT
Sure, I think that's fine for embedders too.
Comment 17 Brian Hackett (:bhackett) 2010-07-26 14:43:04 PDT
Is the goal to maintain API compatibility?  If so, wouldn't that preclude removing slow natives completely?  If the plan is to remove slow natives, could JS_IsConstructing be removed too?

Alternatively, API compatibility could be maintained (keeping slow natives around) by adding a new bit to JSFunction.flags for fast natives which can be called with a magic |this|.  The problem there is that JSFunction.flags is out of bits, but after talking to dmandelin it looks like it might be possible to expand this field to 32 bits --- rearrange the u.i.nupvars or u.i.wrapper fields.
Comment 18 Jeff Walden [:Waldo] (remove +bmo to email) 2010-07-26 14:55:15 PDT
JSFunction::flags will not be out of bits after bug 581744.  :-)  (I haven't thought about whether using a bit as posited makes sense, or the most sense of all possibilities to address the noted problems.)
Comment 19 Brendan Eich [:brendan] 2010-07-26 15:49:59 PDT
Re: comment 17 -- this bug proposes removing slow natives from the JS API, but any API worth its salt needs JS_IsConstructing -- it is a fundamental misfeature of JS that a function (including a native) can be called or new'ed, and natives including standardized builtins need to know how they were called.

JSFunction::flags have bits free even now (low bits).

/be
Comment 20 Brian Hackett (:bhackett) 2010-07-26 17:30:13 PDT
(In reply to comment #19)
> JSFunction::flags have bits free even now (low bits).

Ah, I hadn't realized the JSPROP_* flags are disjoint from the JSFUN_* flags.  Patch in the other bug which shouldn't break anything in the API.
Comment 21 Igor Bukanov 2010-07-27 00:49:30 PDT
(In reply to comment #19)
> Re: comment 17 -- this bug proposes removing slow natives from the JS API, but
> any API worth its salt needs JS_IsConstructing

The comment 13 suggests how that can be avoided.
Comment 22 Brendan Eich [:brendan] 2010-07-27 07:07:21 PDT
(In reply to comment #21)
> (In reply to comment #19)
> > Re: comment 17 -- this bug proposes removing slow natives from the JS API, but
> > any API worth its salt needs JS_IsConstructing
> 
> The comment 13 suggests how that can be avoided.

Maybe, but a lot of native constructors always construct, even when called. Is it worth the two pointers to native functions per object?

OTOH, JSFunction objects could possibly grow a bit without hurting (they are 14 words, IIRC). Still it seems strictly more complicated to have separate entry points if in most cases construct is null or the same as call. But I could be overlooking some benefits of separating.

/be
Comment 23 Luke Wagner [:luke] 2010-08-10 10:53:47 PDT
Slow natives (esp. fun->u.n.extra) makes things slower and more complicated for argv/argc/fun/script/thisv removal.  Let's do this now.
Comment 24 Brian Hackett (:bhackett) 2010-08-10 13:32:49 PDT
Created attachment 464547 [details] [diff] [review]
Fast ctor patch ported to TM

This rebases the fast constructor patch from bug 579471 to TM.  With tracing on, I get the same failures as clean TM.
Comment 25 Luke Wagner [:luke] 2010-08-11 00:38:48 PDT
(In reply to comment #24)
Thanks Brian!
http://hg.mozilla.org/tracemonkey/rev/f84b470314a8
Comment 26 Luke Wagner [:luke] 2010-08-12 10:59:57 PDT
*** Bug 523459 has been marked as a duplicate of this bug. ***
Comment 27 Luke Wagner [:luke] 2010-08-13 09:59:04 PDT
So one tricky issue with no-slow-native-constructors is InternalConstruct.  This let's the caller (viz., js_InitClass and js_ConstructObject) choose the 'this' argument of the constructor call.  If all constructors are passed magic 'this' values when constructing, then then the chosen 'this' is lost.  Using the caller-chosen 'this', instead of letting the constructor do the normal constructing path is important, e.g., in js_InitClass to avoid weird recursive lazy initialization issues.

My proposal is to:

 1. change JS_IsNativeCalledAsConstructor (JSAPI function that replaces JS_IsConstructing) to be

  JSBool
  JS_IsNativeCalledAsConstructor(JSContext *cx, const jsval *vp,
                                 jsval *ctorThis)

with contract: this function returns true and *ctorThis is non-null, use *ctorThis, otherwise, build your own return value; if it returns false, you are called as a native, use JS_THIS; and

 2. allow magic values to store a 47-bit payload and pass the caller-provided thisv as the payload.  This allows JS_IsNativeCalledAsConstructor to test "is constructing" by testing the magic tag and fill in *ctorThis with the magic payload.

Does this make sense?  Is there any way that this generalization of the JS_IsNativeCalledAsConstructor interface isn't necessary... perhaps some restriction on InternalConstruct I am missing?
Comment 28 Brendan Eich [:brendan] 2010-08-13 10:51:46 PDT
Changing the signature seems enough, why not keep JS_IsConstructing instead of the MySummerVacationWasLong name? :-P

The pre-created case is important, you're right. The plan seems good so long as you can fit that payload in and disambiguate it from other magics. Unless maybe bhackett has a better idea?

/be
Comment 29 Luke Wagner [:luke] 2010-08-13 11:27:37 PDT
(In reply to comment #28)
> Changing the signature seems enough, why not keep JS_IsConstructing instead of
> the MySummerVacationWasLong name? :-P

Well, I was thinking "different semantics, different name", but, yes, the different signature seems enough.

> The pre-created case is important, you're right. The plan seems good so long as
> you can fit that payload in and disambiguate it from other magics. Unless maybe
> bhackett has a better idea?

That's where the key property (asserted by this whole JSWhyMagic monkey business) of "magics don't cross streams" comes in: conceptually, a magic value is a singleton, indistinguishable from other magics.  The payload just allows code to indicate the source of the magic value and assert that only the intended recipients actually receive the magic value.
Comment 30 Luke Wagner [:luke] 2010-08-13 18:15:05 PDT
Created attachment 465911 [details] [diff] [review]
WIP

Compiles interp-only js shell and passes trace/ref tests.  Mostly pretty straightforward, some delightful simplifications made in jsinterp.cpp, esp the Invoke family.  More simplifications when I get to the tracer.
Comment 31 Luke Wagner [:luke] 2010-08-14 19:13:13 PDT
Created attachment 466066 [details] [diff] [review]
WIP 2

jstracer.cpp -= 100 lines
Finished with js/src, browser time.
Comment 32 Luke Wagner [:luke] 2010-08-16 10:53:24 PDT
SS/V8 show no change on Linux, but on 10.6, SS is 1.8% faster (9ms) and V8 is 1% faster (50ms).
Comment 33 Luke Wagner [:luke] 2010-08-17 17:12:20 PDT
Created attachment 466858 [details] [diff] [review]
WIP 3

Builds browser, passes basic tests.  Still need to try-server it, fill in comments, and make sure --enable-shark et al work.
Comment 34 Luke Wagner [:luke] 2010-08-17 18:46:13 PDT
Created attachment 466876 [details] [diff] [review]
xpc changes

XPConnect changes, for review.  The rest coming after I fill in some comments.
Comment 35 Luke Wagner [:luke] 2010-08-17 19:20:36 PDT
Created attachment 466889 [details] [diff] [review]
remove slow natives in !xpconnect
Comment 36 Luke Wagner [:luke] 2010-08-18 18:15:51 PDT
Need to fix bug exposed by mochitests.  There is a call to JS_GetFrameCalleeObject buried in nsContentUtils::GetDocumentFromCaller which expected there to be a constructor frame.
Comment 37 Luke Wagner [:luke] 2010-08-20 09:31:20 PDT
Created attachment 467795 [details] [diff] [review]
xpc changes

With the patch in bug 589028 applied, everything is green on try server.
Comment 38 Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-25 01:21:46 PDT
Comment on attachment 466889 [details] [diff] [review]
remove slow natives in !xpconnect

>diff --git a/dom/src/threads/nsDOMWorker.cpp b/dom/src/threads/nsDOMWorker.cpp

> JSBool
> nsDOMWorkerFunctions::Dump(JSContext* aCx,
>-                           JSObject* /* aObj */,
>                            uintN aArgc,
>-                           jsval* aArgv,
>-                           jsval* /* aRval */)
>+                           jsval* aVp)
> {
>   if (!nsGlobalWindow::DOMWindowDumpEnabled()) {
>     return JS_TRUE;
>   }
> 
>   JSString* str;
>-  if (aArgc && (str = JS_ValueToString(aCx, aArgv[0])) && str) {
>+  if (aArgc && (str = JS_ValueToString(aCx, JS_ARGV(aCx, aVp)[0])) && str) {
>     nsDependentJSString string(str);
>     fputs(NS_ConvertUTF16toUTF8(nsDependentJSString(str)).get(), stderr);
>     fflush(stderr);
>   }
>+  JS_SET_RVAL(cx, aVp, JSVAL_VOID);
>   return JS_TRUE;
> }

The JS_SET_RVAL line should be at the top of the function to cover the !enabled case as well.


> JSBool
> nsDOMWorkerFunctions::MakeTimeout(JSContext* aCx,
>-                                  JSObject* /* aObj */,
>                                   uintN aArgc,
>-                                  jsval* aArgv,
>-                                  jsval* aRval,
>+                                  jsval* aVp,
>                                   PRBool aIsInterval)
> {
>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     return JS_FALSE;
>   }

Is this failing without reporting an error?  Looks non-reporty to me, definite followup fodder.


> JSBool
> nsDOMWorkerFunctions::KillTimeout(JSContext* aCx,
>-                                  JSObject* /* aObj */,
>                                   uintN aArgc,
>-                                  jsval* aArgv,
>-                                  jsval* /* aRval */)
>+                                  jsval* aVp)
> {
>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     return JS_FALSE;
>   }

Same song, second verse.


> JSBool
> nsDOMWorkerFunctions::LoadScripts(JSContext* aCx,
>-                                  JSObject* /* aObj */,
>                                   uintN aArgc,
>-                                  jsval* aArgv,
>-                                  jsval* /* aRval */)
>+                                  jsval* aVp)
> {
>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     return JS_FALSE;
>   }

Third verse...


>@@ -251,18 +237,19 @@ nsDOMWorkerFunctions::LoadScripts(JSCont
> 
>   nsAutoTArray<nsString, 10> urls;
> 
>   if (!urls.SetCapacity((PRUint32)aArgc)) {
>     JS_ReportOutOfMemory(aCx);
>     return JS_FALSE;
>   }
> 
>+  jsval *argv = JS_ARGV(aCx, aVp);

Star by name in this code -- a few other places in this file as well.


> JSBool
> nsDOMWorkerFunctions::NewXMLHttpRequest(JSContext* aCx,
>-                                        JSObject* aObj,
>                                         uintN aArgc,
>-                                        jsval* /* aArgv */,
>-                                        jsval* aRval)
>+                                        jsval* aVp)
> {
>+  JSObject *obj = JS_THIS_OBJECT(aCx, aVp);
>+  if (!obj) {
>+    return JS_FALSE;
>+  }
>+
>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     return JS_FALSE;
>   }

Fourth verse...


> JSBool
> nsDOMWorkerFunctions::MakeNewWorker(JSContext* aCx,
>-                                    JSObject* aObj,
>                                     uintN aArgc,
>-                                    jsval* aArgv,
>-                                    jsval* aRval,
>+                                    jsval* aVp,
>                                     WorkerPrivilegeModel aPrivilegeModel)
> {
>+  JSObject *obj = JS_THIS_OBJECT(aCx, aVp);
>+  if (!obj) {
>+    return JS_FALSE;
>+  }
>+
>   nsDOMWorker* worker = static_cast<nsDOMWorker*>(JS_GetContextPrivate(aCx));
>   NS_ASSERTION(worker, "This should be set by the DOM thread service!");
> 
>   if (worker->IsCanceled()) {
>     return JS_FALSE;
>   }

♫ La la la... ♫


>diff --git a/js/ipc/ObjectWrapperParent.cpp b/js/ipc/ObjectWrapperParent.cpp

> /*static*/ JSBool
>-ObjectWrapperParent::CPOW_Call(JSContext* cx, JSObject* obj, uintN argc,
>-                               jsval* argv, jsval* rval)
>+ObjectWrapperParent::CPOW_Call(JSContext* cx, uintN argc, jsval* vp)
> {
>     CPOW_LOG(("Calling CPOW_Call..."));
> 
>+    JSObject *thisobj = JS_THIS_OBJECT(cx, vp);

Misplaced here too.


>     if (!receiver) {
>         // Substitute child global for parent global object.
>         // TODO First make sure we're really replacing the global object?
>         ContextWrapperParent* manager =
>             static_cast<ContextWrapperParent*>(function->Manager());
>         receiver = manager->GetGlobalObjectWrapper();
>     }
> 
>     nsTArray<JSVariant> in_argv(argc);
>+    jsval *argv = JS_ARGV(cx, vp);

Misplaced.

>     for (uintN i = 0; i < argc; i++)
>         if (!jsval_to_JSVariant(cx, argv[i], in_argv.AppendElement()))
>             return JS_FALSE;
> 
>     JSVariant out_rval;
> 
>     return (function->Manager()->RequestRunToCompletion() &&
>             function->CallCall(receiver, in_argv,
>                                aco.StatusPtr(), &out_rval) &&
>             aco.Ok() &&
>-            jsval_from_JSVariant(cx, out_rval, rval));
>+            jsval_from_JSVariant(cx, out_rval, vp));

Hum.  We don't have JS_RVAL_ADDRESS, but we need it here if JS_SET_RVAL is really supposed to be an abstraction.  Followup to add?  (I suppose I should point out these places further, to make such a change easier, but it's enough effort -- and might be redone if we pass call vectors as struct refs rather than pointer/length pairs -- that I'm not going to bother.)


>     nsTArray<JSVariant> in_argv(argc);
>+    jsval *argv = JS_ARGV(cx, vp);

Again misplaced.


>diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp

>-static JSBool ConstructAbstract(JSContext* cx, JSObject* obj, uintN argc,
>-  jsval* argv, jsval* rval);
>+static JSBool ConstructAbstract(JSContext* cx, uintN argc, jsval *vp);

Misplaced.


> namespace FunctionType {
>   static JSBool Create(JSContext* cx, uintN argc, jsval* vp);
>-  static JSBool ConstructData(JSContext* cx, JSObject* typeObj,
>+  static JSBool ConstructData(JSContext* cx, JSObject *typeObj,
>     JSObject* dataObj, JSObject* fnObj, JSObject* thisObj);

Misplaced.


> JSBool
> CType::ConstructData(JSContext* cx,
>-                     JSObject* obj,
>                      uintN argc,
>-                     jsval* argv,
>-                     jsval* rval)
>+                     jsval* vp)
> {
>   // get the callee object...
>-  obj = JSVAL_TO_OBJECT(JS_ARGV_CALLEE(argv));
>+  JSObject *obj = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));

Misplaced.


>   if (argc == 0) {
>     // Construct a null pointer.
>     return JS_TRUE;
>   }
> 
>+  jsval *argv = JS_ARGV(cx, vp);

Misplaced.


>@@ -3526,16 +3514,17 @@ ArrayType::ConstructData(JSContext* cx,
>   } else {
>     if (argc != 1) {
>       JS_ReportError(cx, "constructor takes one argument");
>       return JS_FALSE;
>     }
> 
>     JSObject* baseType = GetBaseType(cx, obj);
> 
>+    jsval *argv = JS_ARGV(cx, vp);

.


>   char* buffer = static_cast<char*>(CData::GetData(cx, result));
>   const FieldInfoHash* fields = GetFieldInfo(cx, obj);
> 
>+  jsval *argv = JS_ARGV(cx, vp);

.


> JSBool
> FunctionType::Call(JSContext* cx,
>-                   JSObject* obj,
>                    uintN argc,
>-                   jsval* argv,
>-                   jsval* rval)
>+                   jsval* vp)
> {
>   // get the callee object...
>-  obj = JSVAL_TO_OBJECT(JS_ARGV_CALLEE(argv));
>+  JSObject *obj = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));

.


>@@ -4972,16 +4959,17 @@ FunctionType::Call(JSContext* cx,
>   // prepare the values for each argument
>   AutoValueAutoArray values;
>   AutoValueAutoArray strings;
>   if (!values.resize(argc)) {
>     JS_ReportOutOfMemory(cx);
>     return false;
>   }
> 
>+  jsval *argv = JS_ARGV(cx, vp);

.


> JSBool
> Int64::Construct(JSContext* cx,
>-                 JSObject* obj,
>                  uintN argc,
>-                 jsval* argv,
>-                 jsval* rval)
>+                 jsval* vp)
> {
>   // Construct and return a new Int64 object.
>   if (argc != 1) {
>     JS_ReportError(cx, "Int64 takes one argument");
>     return JS_FALSE;
>   }
> 
>+  jsval *argv = JS_ARGV(cx, vp);

.


> JSBool
> UInt64::Construct(JSContext* cx,
>-                  JSObject* obj,
>                   uintN argc,
>-                  jsval* argv,
>-                  jsval* rval)
>+                  jsval* vp)
> {
>   // Construct and return a new UInt64 object.
>   if (argc != 1) {
>     JS_ReportError(cx, "UInt64 takes one argument");
>     return JS_FALSE;
>   }
> 
>+  jsval *argv = JS_ARGV(cx, vp);

.


>diff --git a/js/src/jsapi.h b/js/src/jsapi.h

>@@ -1057,19 +1057,22 @@ JS_InitCTypesClass(JSContext *cx, JSObje
>  * NB: there is an anti-dependency between JS_CALLEE and JS_SET_RVAL: native
>  * methods that may inspect their callee must defer setting their return value
>  * until after any such possible inspection. Otherwise the return value will be
>  * inspected instead of the callee function object.
>  *
>  * WARNING: These are not (yet) mandatory macros, but new code outside of the
>  * engine should use them. In the Mozilla 2.0 milestone their definitions may
>  * change incompatibly.
>+ *
>+ * N.B. constructors may not use JS_THIS, as no 'this' object has been
>+ * created.
>  */

"must", RFC 2119.  :-)


>+ * Note that embeddings do not need to use this query unless they use the
>+ * aformentioned API/flags.
>+ */

"aforementioned"


>+static JS_ALWAYS_INLINE JSBool
>+JS_IsConstructing_PossiblyWithGivenThisObject(JSContext *cx, const jsval *vp,
>+                                              JSObject **ctorThis)

This name is awesome.  WouldYouLikeFriesAndADrinkWithThat?

JS_IsConstructingMaybeWithProvidedThis seems better (and shorter) than the double-underscored name.  As for |ctorThis|, a name like |partialThis| or |incompleteThis| seems better to me: your name leaves me confused about what exactly is being returned.


>+/*
>+ * If a constructor does not have any static knowledge about the type of
>+ * object to create, it can request that the JS engine creates a default new
>+ * 'this' object, as is done for non-constructor natives when called with new.
>+ */

"create"


>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp

>-JS_REQUIRES_STACK void
>-StackSpace::getSynthesizedSlowNativeFrame(JSContext *cx, StackSegment *&seg, JSStackFrame *&fp)
>-{
>-    Value *start = firstUnused();
>-    JS_ASSERT(size_t(end - start) >= VALUES_PER_STACK_SEGMENT + VALUES_PER_STACK_FRAME);
>-    seg = new(start) StackSegment;
>-    fp = reinterpret_cast<JSStackFrame *>(seg + 1);
>-}
>-
>-JS_REQUIRES_STACK void
>-StackSpace::pushSynthesizedSlowNativeFrame(JSContext *cx, StackSegment *seg, JSStackFrame *fp,
>-                                           JSFrameRegs &regs)
>-{
>-    JS_ASSERT(!fp->hasScript() && FUN_SLOW_NATIVE(fp->getFunction()));
>-    fp->down = cx->fp;
>-    seg->setPreviousInMemory(currentSegment);
>-    currentSegment = seg;
>-    cx->pushSegmentAndFrame(seg, fp, regs);
>-    seg->setInitialVarObj(NULL);
>-}
>-
>-JS_REQUIRES_STACK void
>-StackSpace::popSynthesizedSlowNativeFrame(JSContext *cx)
>-{
>-    JS_ASSERT(isCurrentAndActive(cx));
>-    JS_ASSERT(cx->hasActiveSegment());
>-    JS_ASSERT(currentSegment->getInitialFrame() == cx->fp);
>-    JS_ASSERT(!cx->fp->hasScript() && FUN_SLOW_NATIVE(cx->fp->getFunction()));
>-    cx->popSegmentAndFrame();
>-    currentSegment = currentSegment->getPreviousInMemory();
>-}

\o/


>diff --git a/js/src/jscntxtinlines.h b/js/src/jscntxtinlines.h

>+JS_ALWAYS_INLINE bool
>+callJSNative(JSContext *cx, js::Native native, uintN argc, js::Value *vp)
> {
>+#ifdef DEBUG
>+        JSBool alreadyThrowing = cx->throwing;
>+#endif

Mis-indented.


>+    assertSameCompartment(cx, ValueArray(vp, argc + 2));

This looks...transposed.


>diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp

>+    Value *argv = vp + 2;

We usually don't tend to add extra little helpers like this, or use JS_ARGV, for engine-internal code, but I don't much care either way.


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>+    /* initialize args.thisv on all paths below */

Not a sentence fragment, should be capitalized and have a period.


>@@ -5687,24 +5527,24 @@ BEGIN_CASE(JSOP_LAMBDA)
>                          * so regs.sp[1 - (iargc + 2)], and not regs.sp[-(iargc + 2)],
>                          * is the callee for this JSOP_CALL.
>                          */
>                         const Value &cref = regs.sp[1 - (iargc + 2)];
>                         JSObject *callee;
> 
>                         if (IsFunctionObject(cref, &callee)) {
>                             JSFunction *calleeFun = GET_FUNCTION_PRIVATE(cx, callee);
>-                            FastNative fastNative = FUN_FAST_NATIVE(calleeFun);
>-
>-                            if (fastNative) {
>-                                if (iargc == 1 && fastNative == array_sort) {
>+                            Native native = calleeFun->maybeNative();
>+
>+                            if (native) {

Unify to if (Native native = ...).


>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp

> static JSBool
>-Number(JSContext *cx, JSObject *obj, uintN argc, Value *argv, Value *rval)
>+Number(JSContext *cx, uintN argc, Value *vp)
> {
>-    if (argc != 0) {
>+    Value num;

You could do |Value &num = vp[0]| here and avoid the temporary (and an assignment further down).


>+    if (IsConstructing(vp)) {
>+        JSObject *obj = NewBuiltinClassInstance(cx, &js_NumberClass);
>+        if (!obj)
>+            return false;
>+        obj->setPrimitiveThis(num);
>+        vp->setObject(*obj);
>+    } else {
>+        *vp = num;
>+    }
>     return true;

Early-return for the not-constructing case seems nicer to me.


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>+JSObject*
>+js_NewInstance(JSContext *cx, JSObject *callee)

This is elegant.


>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp

>+        Value rval;
>+        if (!ExternalInvoke(cx, newobj, callable->fslots[JSSLOT_CALLABLE_CALL],
>+                            argc, vp + 2, &rval)) {
>             return false;
>         }
>-        if (rval->isPrimitive())
>-            rval->setObject(*newobj);
>+        if (rval.isPrimitive())
>+            rval.setObject(*newobj);
> 
>+        *vp = rval;
>         return true;

This seems slightly better as:

  if (rval->isPrimitive())
    vp->setObject(*newobj);
  else
    *vp = rval;
  return true;


>diff --git a/js/src/jsxml.cpp b/js/src/jsxml.cpp

>@@ -630,18 +629,18 @@ NamespaceHelper(JSContext *cx, JSObject 
>             /* Namespace called with one Namespace argument is identity. */
>             *rval = urival;
>             return JS_TRUE;
>         }
> 
>         obj = NewBuiltinClassInstance(cx, &js_NamespaceClass);
>         if (!obj)
>             return JS_FALSE;
>-        *rval = OBJECT_TO_JSVAL(obj);
>-    }
>+    }
>+    *rval = OBJECT_TO_JSVAL(obj);

I don't see a reason to move this assigment; please leave it where it was.


>@@ -737,18 +735,18 @@ QNameHelper(JSContext *cx, JSObject *obj
> 
>         /*
>          * Create and return a new QName or AttributeName object exactly as if
>          * constructed.
>          */
>         obj = NewBuiltinClassInstance(cx, clasp);
>         if (!obj)
>             return JS_FALSE;
>-        *rval = OBJECT_TO_JSVAL(obj);
>-    }
>+    }
>+    *rval = OBJECT_TO_JSVAL(obj);

Again, don't see a reason for the move.


>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp

> static JSBool
>-Trap(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Trap(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSString *str;
>     JSScript *script;
>     int32 i;
> 
>+    jsval *argv = JS_ARGV(cx, vp);
>     if (argc == 0) {
>         JS_ReportErrorNumber(cx, my_GetErrorMessage, NULL, JSSMSG_TRAP_USAGE);
>         return JS_FALSE;
>     }
>     argc--;
>     str = JS_ValueToString(cx, argv[argc]);
>     if (!str)
>         return JS_FALSE;
>     argv[argc] = STRING_TO_JSVAL(str);
>     if (!GetTrapArgs(cx, argc, argv, &script, &i))
>         return JS_FALSE;
>     return JS_SetTrap(cx, script, script->code + i, TrapHandler, STRING_TO_JSVAL(str));
> }

Looks like it needs a JS_SET_RVAL.


> static JSBool
>-Untrap(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Untrap(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSScript *script;
>     int32 i;
> 
>-    if (!GetTrapArgs(cx, argc, argv, &script, &i))
>+    if (!GetTrapArgs(cx, argc, JS_ARGV(cx, vp), &script, &i))
>         return JS_FALSE;
>     JS_ClearTrap(cx, script, script->code + i, NULL, NULL);
>     return JS_TRUE;
> }

Ditto.


> static JSBool
>-Notes(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Notes(JSContext *cx, uintN argc, jsval *vp)
> {
>     uintN i;
>     JSScript *script;
> 
>+    jsval *argv = JS_ARGV(cx, vp);
>     for (i = 0; i < argc; i++) {
>         script = ValueToScript(cx, argv[i]);
>         if (!script)
>             continue;
> 
>         SrcNotes(cx, script);
>     }
>     return JS_TRUE;

Ditto.


> static JSBool
>-Disassemble(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Disassemble(JSContext *cx, uintN argc, jsval *vp)
> {
>     bool lines = false, recursive = false;
> 
>+    jsval *argv = JS_ARGV(cx, vp);
>     while (argc > 0 && JSVAL_IS_STRING(argv[0])) {
>         const char *bytes = JS_GetStringBytes(JSVAL_TO_STRING(argv[0]));
>         lines = !strcmp(bytes, "-l");
>         recursive = !strcmp(bytes, "-r");
>         if (!lines && !recursive)
>             break;
>         argv++, argc--;
>     }
>@@ -1861,68 +1871,71 @@ Disassemble(JSContext *cx, JSObject *obj
>     for (uintN i = 0; i < argc; i++) {
>         if (!DisassembleValue(cx, argv[i], lines, recursive))
>             return false;
>     }
>     return true;
> }

Ditto.


>-    *rval = OBJECT_TO_JSVAL(obj); /* I like to root it, root it. */
>-    ok = Disassemble(cx, obj, 1, rval, rval); /* gross, but works! */
>-    *rval = JSVAL_VOID;
>+    *vp = OBJECT_TO_JSVAL(obj); /* I like to root it, root it. */
>+    ok = Disassemble(cx, 1, vp); /* gross, but works! */
>+    *vp = JSVAL_VOID;

JS_SET_RVAL?  Although I suppose purity on this point is kinda hopeless.


> static JSBool
>-DisassWithSrc(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>-              jsval *rval)
>+DisassWithSrc(JSContext *cx, uintN argc, jsval *vp)
> {
> #define LINE_BUF_LEN 512
>     uintN i, len, line1, line2, bupline;
>     JSScript *script;
>     FILE *file;
>     char linebuf[LINE_BUF_LEN];
>     jsbytecode *pc, *end;
>     JSBool ok;
>     static char sep[] = ";-------------------------";
> 
>     ok = JS_TRUE;
>+    jsval *argv = JS_ARGV(cx, vp);
>     for (i = 0; ok && i < argc; i++) {
>         script = ValueToScript(cx, argv[i]);
>         if (!script)
>            return JS_FALSE;
> 
>         if (!script->filename) {
>             JS_ReportErrorNumber(cx, my_GetErrorMessage, NULL,
>                                  JSSMSG_FILE_SCRIPTS_ONLY);
>@@ -1991,25 +2004,26 @@ DisassWithSrc(JSContext *cx, JSObject *o
>       bail:
>         fclose(file);
>     }
>     return ok;
> #undef LINE_BUF_LEN
> }

Another missing return-set.


> static JSBool
>-Tracing(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Tracing(JSContext *cx, uintN argc, jsval *vp)
> {
>     FILE *file;
> 
>     if (argc == 0) {
>-        *rval = BOOLEAN_TO_JSVAL(cx->tracefp != 0);
>+        *vp = BOOLEAN_TO_JSVAL(cx->tracefp != 0);
>         return JS_TRUE;
>     }
> 
>+    jsval *argv = JS_ARGV(cx, vp);
>     switch (JS_TypeOfValue(cx, argv[0])) {
>       case JSTYPE_NUMBER:
>       case JSTYPE_BOOLEAN: {
>         JSBool bval;
>         JS_ValueToBoolean(cx, argv[0], &bval);
>         file = bval ? stderr : NULL;
>         break;
>       }

Ditto.


> static JSBool
>-DumpStats(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+DumpStats(JSContext *cx, uintN argc, jsval *vp)
> {
>     uintN i;
>     JSString *str;
>     const char *bytes;
>     jsid id;
>     JSObject *obj2;
>     JSProperty *prop;
>     Value value;
> 
>+    jsval *argv = JS_ARGV(cx, vp);
>     for (i = 0; i < argc; i++) {
>         str = JS_ValueToString(cx, argv[i]);
>         if (!str)
>             return JS_FALSE;
>         argv[i] = STRING_TO_JSVAL(str);
>         bytes = JS_GetStringBytes(str);
>         if (strcmp(bytes, "arena") == 0) {
> #ifdef JS_ARENAMETER
>@@ -2074,16 +2089,17 @@ DumpStats(JSContext *cx, JSObject *obj, 
> #endif
>         } else if (strcmp(bytes, "atom") == 0) {
>             js_DumpAtoms(cx, gOutFile);
>         } else if (strcmp(bytes, "global") == 0) {
>             DumpScope(cx, cx->globalObject, stdout);
>         } else {
>             if (!JS_ValueToId(cx, STRING_TO_JSVAL(str), &id))
>                 return JS_FALSE;
>+            JSObject *obj;
>             if (!js_FindProperty(cx, id, &obj, &obj2, &prop))
>                 return JS_FALSE;
>             if (prop) {
>                 obj2->dropProperty(cx, prop);
>                 if (!obj->getProperty(cx, id, &value))
>                     return JS_FALSE;
>             }
>             if (!prop || !value.isObjectOrNull()) {

Ditto.


> JSBool
>-DumpObject(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+DumpObject(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *arg0 = NULL;
>-    if (!JS_ConvertArguments(cx, argc, argv, "o", &arg0))
>+    if (!JS_ConvertArguments(cx, argc, JS_ARGV(cx, vp), "o", &arg0))
>         return JS_FALSE;
> 
>     js_DumpObject(arg0);
> 
>     return JS_TRUE;
> }

Ditto.


> static JSBool
>-ConvertArgs(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+ConvertArgs(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSBool b = JS_FALSE;
>     jschar c = 0;
>     int32 i = 0, j = 0;
>     uint32 u = 0;
>     jsdouble d = 0, I = 0, re = 0, im = 0;
>     char *s = NULL;
>     JSString *str = NULL;
>     jschar *w = NULL;
>     JSObject *obj2 = NULL;
>     JSFunction *fun = NULL;
>     jsval v = JSVAL_VOID;
>     JSBool ok;
> 
>     if (!JS_AddArgumentFormatter(cx, "ZZ", ZZ_formatter))
>         return JS_FALSE;
>-    ok = JS_ConvertArguments(cx, argc, argv, "b/ciujdIsSWofvZZ*",
>+    ok = JS_ConvertArguments(cx, argc, JS_ARGV(cx, vp), "b/ciujdIsSWofvZZ*",
>                              &b, &c, &i, &u, &j, &d, &I, &s, &str, &w, &obj2,
>                              &fun, &v, &re, &im);
>     JS_RemoveArgumentFormatter(cx, "ZZ");
>     if (!ok)
>         return JS_FALSE;
>     fprintf(gOutFile,
>             "b %u, c %x (%c), i %ld, u %lu, j %ld\n",
>             b, c, (char)c, i, u, j);

.


> static JSBool
>-Clear(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Clear(JSContext *cx, uintN argc, jsval *vp)
> {
>-    if (argc != 0 && !JS_ValueToObject(cx, argv[0], &obj))
>+    JSObject *obj;
>+    if (argc != 0 && !JS_ValueToObject(cx, JS_ARGV(cx, vp)[0], &obj))
>         return JS_FALSE;
>     JS_ClearScope(cx, obj);
>     return JS_TRUE;
> }

.


> static JSBool
>-Seal(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Seal(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *target;
>     JSBool deep = JS_FALSE;
> 
>-    if (!JS_ConvertArguments(cx, argc, argv, "o/b", &target, &deep))
>+    if (!JS_ConvertArguments(cx, argc, JS_ARGV(cx, vp), "o/b", &target, &deep))
>         return JS_FALSE;
>     if (!target)
>         return JS_TRUE;
>     return JS_SealObject(cx, target, deep);
> }

.


>@@ -2510,24 +2530,24 @@ StackQuota(JSContext *cx, uintN argc, js
>     return JS_TRUE;
> }
> 
> static const char* badUTF8 = "...\xC0...";
> static const char* bigUTF8 = "...\xFB\xBF\xBF\xBF\xBF...";
> static const jschar badSurrogate[] = { 'A', 'B', 'C', 0xDEEE, 'D', 'E', 0 };
> 
> static JSBool
>-TestUTF8(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+TestUTF8(JSContext *cx, uintN argc, jsval *vp)
> {
>     int32 mode = 1;
>     jschar chars[20];
>     size_t charsLength = 5;
>     char bytes[20];
>     size_t bytesLength = 20;
>-    if (argc && !JS_ValueToInt32(cx, *argv, &mode))
>+    if (argc && !JS_ValueToInt32(cx, *JS_ARGV(cx, vp), &mode))
>         return JS_FALSE;
> 
>     /* The following throw errors if compiled with UTF-8. */
>     switch (mode) {
>       /* mode 1: malformed UTF-8 string. */
>       case 1:
>         JS_NewStringCopyZ(cx, badUTF8);
>         break;
>@@ -2546,17 +2566,17 @@ TestUTF8(JSContext *cx, JSObject *obj, u
>       default:
>         JS_ReportError(cx, "invalid mode parameter");
>         return JS_FALSE;
>     }
>     return !JS_IsExceptionPending (cx);
> }

.



> static JSBool
>-Snarl(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Snarl(JSContext *cx, uintN argc, jsval *vp)
> {
>     if (argc < 1) {
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
>                              "compile", "0", "s");
>     }
> 
>-    jsval arg0 = argv[0];
>+    JSObject *thisobj = JS_THIS_OBJECT(cx, vp);

Null-check needed.


>+    jsval arg0 = JS_ARGV(cx, vp)[0];
>     if (!JSVAL_IS_STRING(arg0)) {
>         const char *typeName = JS_GetTypeName(cx, JS_TypeOfValue(cx, arg0));
>         JS_ReportError(cx, "expected string to compile, got %s", typeName);
>         return JS_FALSE;
>     }
> 
>     JSString *scriptContents = JSVAL_TO_STRING(arg0);
>     JSScript *script = JS_CompileUCScript(cx, NULL, JS_GetStringCharsZ(cx, scriptContents),
>                                           JS_GetStringLength(scriptContents), "<string>", 0);
>     if (!script)
>         return JS_FALSE;
> 
>-    JS_ExecuteScript(cx, obj, script, NULL);
>+    JS_ExecuteScript(cx, thisobj, script, NULL);
>     JS_DestroyScript(cx, script);
> 
>     return JS_TRUE;
> }

Return-set needed.


> static JSBool
>-Help(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Help(JSContext *cx, uintN argc, jsval *vp)
> {
>     uintN i, j;
>     int did_header, did_something;
>     JSType type;
>     JSFunction *fun;
>     JSString *str;
>     const char *bytes;
> 
>     fprintf(gOutFile, "%s\n", JS_GetImplementationVersion());
>     if (argc == 0) {
>         fputs(shell_help_header, gOutFile);
>         for (i = 0; shell_functions[i].name; i++)
>             fprintf(gOutFile, "%s\n", shell_help_messages[i]);
>     } else {
>         did_header = 0;
>+        jsval *argv = JS_ARGV(cx, vp);
>         for (i = 0; i < argc; i++) {
>             did_something = 0;
>             type = JS_TypeOfValue(cx, argv[i]);
>             if (type == JSTYPE_FUNCTION) {
>                 fun = JS_ValueToFunction(cx, argv[i]);
>                 str = fun->atom ? ATOM_TO_STRING(fun->atom) : NULL;
>             } else if (type == JSTYPE_STRING) {
>                 str = JSVAL_TO_STRING(argv[i]);

.


> static JSBool
>-Exec(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+Exec(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSFunction *fun;
>     const char *name, **nargv;
>     uintN i, nargc;
>     JSString *str;
>     pid_t pid;
>     int status;
> 
>-    fun = JS_ValueToFunction(cx, argv[-2]);
>+    fun = JS_ValueToFunction(cx, vp[0]);
>     if (!fun)
>         return JS_FALSE;
>     if (!fun->atom)
>         return JS_TRUE;
>     name = JS_GetStringBytes(ATOM_TO_STRING(fun->atom));
>     nargc = 1 + argc;
>     nargv = JS_malloc(cx, (nargc + 1) * sizeof(char *));
>     if (!nargv)
>         return JS_FALSE;
>     nargv[0] = name;
>+    jsval *argv = JS_ARGV(cx, vp);
>     for (i = 1; i < nargc; i++) {
>         str = JS_ValueToString(cx, argv[i-1]);
>         if (!str) {
>             JS_free(cx, nargv);
>             return JS_FALSE;
>         }
>         nargv[i] = JS_GetStringBytes(str);
>     }

.


>diff --git a/js/src/shell/jsworkers.cpp b/js/src/shell/jsworkers.cpp
>@@ -1121,17 +1120,17 @@ Worker::processOneEvent()
>             if (!threadPool->getWorkerQueue()->post(this))
>                 JS_ReportOutOfMemory(context);
>         }
>     }
>     delete event;
> }
> 
> JSBool
>-Worker::jsPostMessageToParent(JSContext *cx, int argc, jsval *vp)
>+Worker::jsPostMessageToParent(JSContext *cx, uintN argc, jsval *vp)
> {
>     jsval workerval;
>     if (!JS_GetReservedSlot(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)), 0, &workerval))
>         return false;
>     Worker *w = (Worker *) JSVAL_TO_PRIVATE(workerval);
> 
>     {
>         JSAutoSuspendRequest suspend(cx);  // avoid nesting w->lock in a request
>@@ -1149,17 +1148,17 @@ Worker::jsPostMessageToParent(JSContext 
>     if (!w->parent->post(event)) {
>         delete event;
>         JS_ReportOutOfMemory(cx);
>     }
>     return true;
> }

. (if pre-existing, it seems)


> JSBool
>-Worker::jsPostMessageToChild(JSContext *cx, int argc, jsval *vp)
>+Worker::jsPostMessageToChild(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *workerobj = JS_THIS_OBJECT(cx, vp);
>     if (!workerobj)
>         return false;
>     Worker *w = (Worker *) JS_GetInstancePrivate(cx, workerobj, &jsWorkerClass, JS_ARGV(cx, vp));
>     if (!w) {
>         if (!JS_IsExceptionPending(cx))
>             JS_ReportError(cx, "Worker was shut down");
>@@ -1176,17 +1175,17 @@ Worker::jsPostMessageToChild(JSContext *
>     if (!w->post(event)) {
>         JS_ReportOutOfMemory(cx);
>         return false;
>     }
>     return true;
> }

.


> JSBool
>-Worker::jsTerminate(JSContext *cx, int argc, jsval *vp)
>+Worker::jsTerminate(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *workerobj = JS_THIS_OBJECT(cx, vp);
>     if (!workerobj)
>         return false;
>     Worker *w = (Worker *) JS_GetInstancePrivate(cx, workerobj, &jsWorkerClass, JS_ARGV(cx, vp));
>     if (!w)
>         return !JS_IsExceptionPending(cx);  // ok to terminate twice
> 

.


Nothing too substantial, so r=me.  You might or might not slightly conflict with bug 429507, if I get a test review in time -- just be wary.  And now, sleep...
Comment 39 Luke Wagner [:luke] 2010-08-28 17:57:41 PDT
(In reply to comment #38)

Thanks for the big review!

> Is this failing without reporting an error?  Looks non-reporty to me, definite
> followup fodder.

Filed bug 591698.

> >+static JS_ALWAYS_INLINE JSBool
> >+JS_IsConstructing_PossiblyWithGivenThisObject(JSContext *cx, const jsval *vp,
> >+                                              JSObject **ctorThis)
> 
> This name is awesome.  WouldYouLikeFriesAndADrinkWithThat?
> 
> JS_IsConstructingMaybeWithProvidedThis seems better (and shorter) than the
> double-underscored name.  As for |ctorThis|, a name like |partialThis| or
> |incompleteThis| seems better to me: your name leaves me confused about what
> exactly is being returned.

The _ denotes a pause in reading, stemming from Brendan's good observation in the newsgroup post that we'd really like to say "JS_IsConstructing comma PossiblyWithGivenThis".  The length should be no matter.  It is very rare that a constructor needs this (like, there are 2 or 3 uses in mozilla), and so I think the length and specificity of the name is justified.

As for 'ctorThis': 'partialThis' or 'incompleteThis' would lead me to believe that I am always given an object, but that it is partially constructed (whatever that means).  I do like 'maybeThis' though.  (In general I think maybe would be a nice naming convention for arguments that are nullable).

> >+    assertSameCompartment(cx, ValueArray(vp, argc + 2));
> 
> This looks...transposed.

It does.  But its not.

> >         obj = NewBuiltinClassInstance(cx, &js_NamespaceClass);
> >         if (!obj)
> >             return JS_FALSE;
> >-        *rval = OBJECT_TO_JSVAL(obj);
> >-    }
> >+    }
> >+    *rval = OBJECT_TO_JSVAL(obj);
> 
> I don't see a reason to move this assigment; please leave it where it was.

The reason is that constructors must set rval on success and NamespaceHelper (called from Namespace) was not setting rval on the !!obj path.

> And now, sleep...

Much appreciated
Comment 40 Luke Wagner [:luke] 2010-09-01 14:41:59 PDT
http://hg.mozilla.org/tracemonkey/rev/66c8ad02543b

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