Currently js_PutArgsObject and js_PutCallObject can fail due to a potential out-of-memory when attempting to allocate slots and properties to save arguments and variables. It complicates an analysis of objects behavior as it ads one more state to consider, that is, a partially put Arguments or Call object. This extra complexity can be avoided if the reserved slots would be pre-allocated when the objects are created. In addition it should make js_PutArgsObject faster as it would not be necessary to define properties for all arguments and callee/length.

This is what I submit to the try server.

Here is n updated patch. I have not yet figured out how to make a call to void function from the trace so the patch still declares js_PutArguments as JSBool.

updated patch to reflect landing of bug 505460

(In reply to comment #5) > Created an attachment (id=393200) [details] > v4 The patch preallocates the reserved slots together with the Arguments object and uses shared (non-slot) getter and setter to access JSStackFrame::argv. To indicate deleted indexed properties the patch stores JSVAL_HOLE in the reserved slots. The patch for now uses JSBool for the traced version of js_PutArguments as I have not figured out yet how to declare void functions for the tracer.

Comment on attachment 393200 [details] [diff] [review] v4 >+/* >+ * Reserved slot structure for Arguments objects: >+ * >+ * JSSLOT_PRIVATE - the corresponding frame until the frame exits. >+ * JSSLOT_ARG_COUNT - the number of actual arguments and a flag indicating >+ * whether arguments.length was overwritten. >+ * JSSLOT_ARG_CALLEE - the arguments.callee value or JSVAL_HOLE if that was >+ * overwritten. >+ * JSSLOT_ARG_COPY_START .. - room to store the corresponding arguments after >+ * the frame exists. The slot's value will be JSVAL_HOLE >+ * if arguments[i] was deleted or overwritten. >+ */ >+const uint32 JSSLOT_ARG_COUNT = JSSLOT_PRIVATE + 1; >+const uint32 JSSLOT_ARG_CALLEE = JSSLOT_PRIVATE + 2; >+const uint32 JSSLOT_ARG_COPY_START = JSSLOT_PRIVATE + 3; Suggest JSSLOT_ARGS_* name scheme, and in particular JSSLOT_ARGS_LENGTH instead of JSSLOT_ARG_COUNT. >+const uint32 ARGS_CLASS_FIXED_RESERVED_SLOTS = JSSLOT_ARG_COPY_START - >+ (JSSLOT_PRIVATE + 1); Why the + 1 ? Seems off by one (too small). /be

(In reply to comment #7) > >+const uint32 ARGS_CLASS_FIXED_RESERVED_SLOTS = JSSLOT_ARG_COPY_START - > >+ (JSSLOT_PRIVATE + 1); > > Why the + 1 ? Seems off by one (too small). That value is JSSLOT_ARG_COPY_START - (JSSLOT_PRIVATE + 1) or JSSLOT_PRIVATE + 3 - (JSSLOT_PRIVATE + 1) or 2. That is exactly the number of the extra fixed slots the class needs besides JSSLOT_PRIVATE which is accounted by JSCLASS_HAS_PRIVATE. I will update the patch with better comments.

The new patch uses the suggested naming schemma like ARGS, not ARGm and removes the useless guard check for js_PutArguments

(In reply to comment #8) > (In reply to comment #7) > > >+const uint32 ARGS_CLASS_FIXED_RESERVED_SLOTS = JSSLOT_ARG_COPY_START - > > >+ (JSSLOT_PRIVATE + 1); > > > > Why the + 1 ? Seems off by one (too small). > > That value is > > JSSLOT_ARG_COPY_START - (JSSLOT_PRIVATE + 1) > > or > > JSSLOT_PRIVATE + 3 - (JSSLOT_PRIVATE + 1) > > or 2. That is exactly the number of the extra fixed slots the class needs > besides JSSLOT_PRIVATE which is accounted by JSCLASS_HAS_PRIVATE. Oh, but then don't you want JS_INITIAL_NSLOTS - (JSSLOT_PRIVATE + 1), or a JS_STATIC_ASSERTion that that value == ARGS_CLASS_FIXED_RESERVED_SLOTS ? /be

Comment on attachment 393306 [details] [diff] [review] v5 >+ * JSSLOT_ARG_COPY_START .. - room to store the corresponding arguments after >+ * the frame exists. The slot's value will be JSVAL_HOLE >+ * if arguments[i] was deleted or overwritten. >+ */ >+const uint32 JSSLOT_ARGS_LENGTH = JSSLOT_PRIVATE + 1; >+const uint32 JSSLOT_ARGS_CALLEE = JSSLOT_PRIVATE + 2; >+const uint32 JSSLOT_ARGS_COPY_START = JSSLOT_PRIVATE + 3; Comment still spells it JSSLOT_ARG_COPY_START. >+/* >+ * JSSLOT_ARGS_LENGTH stores ((argc << 1) | overwritten_flag) as int jsval. Thus >+ * we insist that (ARRAY_INIT_LIMIT << 1) + 1 still fits JSVAL_INT_MAX. >+ */ >+JS_STATIC_ASSERT(ARRAY_INIT_LIMIT <= (JSVAL_INT_MAX - 1) / 2); JSVAL_INT_MAX is max or last value in domain, not limit or fencepost, so no need for the (... - 1). Oh, but ARRAY_INIT_LIMIT *is* a fencepost. So something here is comparing apples to pears :-). Do you need to avoid ARRAY_INIT_LIMIT << 1 in the assertion, just in case it could overflow? It would be better if the assertion could use the expression the comment talks about. Use 64 bit if paranoid, or (better) assert that ARRAY_INIT_LIMIT is < 2^30 or smaller (it's 2^24 currently). >+static inline uint32 >+GetArgsLength(JSObject *obj) >+{ >+ JS_ASSERT(STOBJ_GET_CLASS(obj) == &js_ArgumentsClass); >+ >+ uint32 argc = uint32(JSVAL_TO_INT(obj->fslots[JSSLOT_ARGS_LENGTH])) >> 1; >+ JS_ASSERT(argc <= ARRAY_INIT_LIMIT); s/<=/</ since ARRAY_INIT_LIMIT is a fencepost. Presumably this passes the testsuite, which IIRC has good overriding testcase? /be

The new version fixes comments and ARRAY_INIT_LIMIT issue. Our test suite has a good test coverage. Unfortunately jsDriver has a bug that caused initial versions of the patch to be reported as a success when the patch triggered early termination of the tests. But now everything looks ok in all tests.

Comment on attachment 393498 [details] [diff] [review] v6 >- v = INT_TO_JSVAL(reinterpret_cast<jsint>(fp->callee)); Hmm, I didn't see this go by yet (just saw a request for second review, or comment on API compat issue, in bug 506721). How did this ever work? In cvs.mozilla.org it's more sane: value = OBJECT_TO_JSVAL(fp->callee); When did it change to an INT jsval? >@@ -1098,16 +1098,18 @@ js_Invoke(JSContext *cx, uintN argc, jsv > JSNative native; > JSFunction *fun; > JSScript *script; > uintN nslots, i; > uint32 rootedArgsFlag; > JSInterpreterHook hook; > void *hookData; > >+ JS_ASSERT(argc <= ARRAY_INIT_LIMIT); Hmm, do we use ARRAY_INIT_LIMIT as fencepost for *index* or *length* of the array expressed by the initialiser? This seems to limit the index (which is what jsparse.cpp does. Other places (jsarray.cpp, jsfun.cpp) limit length. Fix or followup bug? Suggest renaming to ARRAY_INDEX_LIMIT if that is what it more usefully bounds as a fencepost, else ARRAY_LENGTH_LIMIT. /be

Jason, see comment 13 first part. Cc'ing mrbkap too for ARRAY_INIT_LIMIT fun. /be

The new version of the patch splits ARRAY_INIT_LIMIT into independent JS_ARGS_LENGTH_MAX and JS_ARRAY_INIT_LIMIT. The patch uses #define, not const, to introduce the consts as jsprvtd.h is included in C-code in the debugger.

Comment on attachment 393742 [details] [diff] [review] v7 >+/* >+ * JSSLOT_ARGS_LENGTH stores ((argc << 1) | overwritten_flag) as int jsval. >+ * Thus (JS_ARGS_LENGTH_MAX << 1) | 1 must fit JSVAL_INT_MAX where >+ */ >+JS_STATIC_ASSERT((JS_ARGS_LENGTH_MAX << 1) <= JS_BIT(31)); Shouldn't this use < not <= (otherwise when argc == JS_ARGS_LENGTH_MAX, argc << 1 won't fit in an int jsval)? I.e., JS_BIT(31) is a fencepost, not a maximum value. >+JS_STATIC_ASSERT(jsval((JS_ARGS_LENGTH_MAX << 1) | 1) <= JSVAL_INT_MAX); This is good, thanks. >@@ -2071,18 +2054,18 @@ fun_applyConstructor(JSContext *cx, uint > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, > JSMSG_BAD_APPLY_ARGS, "__applyConstruct__"); > return JS_FALSE; > } > > if (!js_GetLengthProperty(cx, aobj, &length)) > return JS_FALSE; > >- if (length >= ARRAY_INIT_LIMIT) >- length = ARRAY_INIT_LIMIT - 1; >+ if (length > JS_ARGS_LENGTH_MAX) >+ length = JS_ARGS_LENGTH_MAX; Good so far... >@@ -1098,16 +1098,18 @@ js_Invoke(JSContext *cx, uintN argc, jsv > JSNative native; > JSFunction *fun; > JSScript *script; > uintN nslots, i; > uint32 rootedArgsFlag; > JSInterpreterHook hook; > void *hookData; > >+ JS_ASSERT(argc <= JS_ARGS_LENGTH_MAX); >+ Ditto... >+#define JSFRAME_OVERRIDDEN_ARGS 0x400 /* overridden arguments local variable */ (Mega-nit: could shorten to JSFRAME_OVERRIDE_ARGS.) >@@ -2395,17 +2395,17 @@ array_push1_dense(JSContext* cx, JSObjec > JSBool JS_FASTCALL > js_ArrayCompPush(JSContext *cx, JSObject *obj, jsval v) > { > JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, obj)); > uint32_t length = (uint32_t) obj->fslots[JSSLOT_ARRAY_LENGTH]; > JS_ASSERT(length <= js_DenseArrayCapacity(obj)); > > if (length == js_DenseArrayCapacity(obj)) { >- if (length >= ARRAY_INIT_LIMIT) { >+ if (length >= JS_ARRAY_INIT_LIMIT) { Ok, JS_ARRAY_INIT_LIMIT is a fencepost on length of array expressed via initialiser/comprehension... >@@ -7733,17 +7733,17 @@ PrimaryExpr(JSContext *cx, JSTokenStream > pn->pn_blockid = tc->blockidGen; > #endif > > ts->flags |= TSF_OPERAND; > matched = js_MatchToken(cx, ts, TOK_RB); > ts->flags &= ~TSF_OPERAND; > if (!matched) { > for (index = 0; ; index++) { >- if (index == ARRAY_INIT_LIMIT) { >+ if (index == JS_ARRAY_INIT_LIMIT) { > js_ReportCompileErrorNumber(cx, ts, NULL, JSREPORT_ERROR, > JSMSG_ARRAY_INIT_TOO_BIG); ... but here we fail when index == fencepost, allowing index == fencepost-1 or length == fencepost. Again, if JS_ARRAY_INIT_LIMIT is a length limit, not an index limit, this is wrong. I wonder if it's worth having two ostensibly independent bounds here: >+/* >+ * Maximum supported value of Arguments.length. It bounds the maximum number >+ * of arguments that can be supplied to the function call using >+ * Function.prototype.apply. >+ */ >+#define JS_ARGS_LENGTH_MAX JS_BIT(24) >+ >+/* >+ * Generous sanity-bound on length (in elements) of array initializer. >+ */ >+#define JS_ARRAY_INIT_LIMIT JS_BIT(24) Logically these are independent although one might want JS_ARRAY_INIT_LIMIT <= JS_ARGS_LENGTH_MAX -- but only if JS_ARGS_LENGTH_MAX is a maximum length value (as its name says it is) and JS_ARRAY_INIT_LIMIT is an index limit (fencepost). But the comment says JS_ARRAY_INIT_LIMIT is a bound (half-open interval sense, or to avoid too many terms, again: limit, fencepost) on length (in elements). Please fix the comment, but also fix the index bounding. Preferably we would use maximum values for both of these parameters, if not unify on JS_ARGS_LENGTH_MAX. Mixing max with limit is confusing. r=me with this fixed. /be /be

(In reply to comment #17) > Created an attachment (id=393874) [details] > v8 The new patch uses single JS_ARGS_LENGTH_MAX as the maximum for both Arguments.lenhth and array initializer.

Comment on attachment 393874 [details] [diff] [review] v8 The patch required serious merge due to landing of bug 498488.

Here is an updated patch after a merge with changes from the bug 498488. I will ask for a review after try server runs.

Comment on attachment 393927 [details] [diff] [review] v9 Asking for the final review.

Comment on attachment 393927 [details] [diff] [review] v9 I saved patches and diffed them, all looks good. One very minor nit: >+ uint32 arg = uint32(JSID_TO_INT(id)); >+ if (arg < fp->argc) { >+ if (fp->argsobj) { >+ jsval v = OBJ_GET_SLOT(cx, JSVAL_TO_OBJECT(fp->argsobj), >+ JSSLOT_ARGS_COPY_START + arg); >+ if (v == JSVAL_HOLE) { >+ return JSVAL_TO_OBJECT(fp->argsobj)->getProperty(cx, id, >+ vp); Argh, this missed fitting in 80 columns by one char. I wouldn't wrap if it fit in 80 not 79 (Emacs has been fixed, I'm told). Holding the 80 column line is challenging but doable in this file (and many others; still, tw=99 is infecting the codebase), but could you wrap in a less lopsided way? ..12345678901234567890123456789012345678901234567890123456789012345678901234567890 >+ return JSVAL_TO_OBJECT(fp->argsobj)->getProperty(cx, id, >+ vp); Instead, how about either tolerating overflow to column 81, or using a single-use local JSObject *argsobj? /be

Here is the patch with the latest nits addressed.

followup to fix a typo in js_Arguments declaration - http://hg.mozilla.org/tracemonkey/rev/15f1be966e01