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.

We would need the compartments and wrappers stuff, IINM, to do without certain native methods' frames. /be

So mark dependencies accordingly. /be

How will constructors know if they've been called as a constructor: |new name()| or as a function: |name()|?

JS_IsConstructing(cx) and a flag in cx?

JSFRAME_CONSTRUCTING in fp->flags

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

(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.)

(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

Stay on target...

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

(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.

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

(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.

Separate call and construct are sauce for the proxy, indeed...

(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

Sure, I think that's fine for embedders too.

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.

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.)

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

(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.

(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.

(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

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.

This rebases the fast constructor patch from bug 579471 to TM. With tracing on, I get the same failures as clean TM.

(In reply to comment #24) Thanks Brian! http://hg.mozilla.org/tracemonkey/rev/f84b470314a8

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?

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

(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.

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.

jstracer.cpp -= 100 lines Finished with js/src, browser time.

SS/V8 show no change on Linux, but on 10.6, SS is 1.8% faster (9ms) and V8 is 1% faster (50ms).

Builds browser, passes basic tests. Still need to try-server it, fill in comments, and make sure --enable-shark et al work.

PConnect changes, for review. The rest coming after I fill in some comments.

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.

With the patch in bug 589028 applied, everything is green on try server.

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...

(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