Closed Bug 420399 Opened 17 years ago Closed 17 years ago

pc stack has substantial overhead

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 7 obsolete files)

Attached file sunspider results (obsolete) (deleted) —
To measure the overhead of the parallel pc that the interpreter maintains to help the decompiler, I modified PUSH_OPND and STORE_OPND from js_interp.c so they no longer maintain the pc stack: #define PUSH_OPND(v) (PUSH(v)) #define STORE_OPND(n,v) (sp[n] = (v)) As the attached SunSpider compare file demonstartes for a thread-safe optimized build of js shell compiled with GCC 4.1.3 on Fedora8, this made things faster by 5%. It would be nice to reduce this overhead.
Here is an idea about replacing the dynamic stack PC with extra work in the decompiler. The decompiler can replay the bytecodes from the start of the expression statement until the current PC. AFAICS this replay should be able to reconstruct the pc stack that the decompiler needs removing the need to maintain it in the interpreter. Or have I missed something?
(In reply to comment #1) > Or have I missed something? I have missed branches that && || and ?" generates. But then the interpreter can record just the branch taken by such operation. With this is information the full reconstruction of the pc stack should be possible.
(In reply to comment #2) > I have missed branches that && || and ?" generates. But then the interpreter > can record just the branch taken by such operation. With this is information > the full reconstruction of the pc stack should be possible. On the other hand this is no necessary: the decompiler can take all the branches to find the one that leads to the current PC. If the branches merge, this is fine since they would lead to the equivalent pc stacks. Of cause this leads to potentially exponential complexity in the decompiler, but that can be restricted with explicit refusal to deal with way too complex cases.
Nominating for FF3 as this may allow up to 5% benchmark improvement.
Flags: blocking1.9?
Keywords: perf
Heck yea!
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
I'm all in favor of this, especially after I added an opcode interpreter loop to js_DecompileValueGenerator to compute the correct stack depth. Please feel free to use and modify to suit. Igor, are you going to take this bug? /be
(In reply to comment #6) > I'm all in favor of this, especially after I added an opcode interpreter loop > to js_DecompileValueGenerator to compute the correct stack depth. Then the problem is simpler. After the code has determined the frame with the value that triggered error, js_DecompileValueGenerator can run that loop until the current pc constructing the stack of (pc, def_index) pairs, where def_index is the index of the slot that the opcode at *pc defines. After the loop the code accesses the simulated stack at the position corresponding to the trigger value recovering both pc that stored the value on the interpreter stack and the stack depth of that pc based on def_index.
Assignee: general → igor
Priority: P1 → --
Attached patch v 0.9 (obsolete) (deleted) — Splinter Review
Work in progress. This is more or less done. What is essentially left is updating comments and testing.
Attached patch v 0.95 (obsolete) (deleted) — Splinter Review
The new version passes js shell test suite without regressions.
Attachment #306931 - Attachment is obsolete: true
Here is the results with the latest patch. It again shows 5% improvement.
Attachment #306628 - Attachment is obsolete: true
The patch from comment 9 passes both the test suite run with tread-safe jsshell and mochi test on Linux.
Attached patch v1 (obsolete) (deleted) — Splinter Review
The new version updates the comments.
Attachment #306937 - Attachment is obsolete: true
Attachment #306972 - Flags: review?(brendan)
Comment on attachment 306972 [details] [diff] [review] v1 >+#define PUSH(v) (*sp++ = (v)) >+#define PUSH_OPND(v) PUSH(v) >+#define STORE_OPND(n,v) (sp[n] = (v)) >+#define POP() (*--sp) > #define POP_OPND() POP() > #define FETCH_OPND(n) (sp[n]) Followup bug to get rid of the _OPND rename/extra-name-parts would be nice. >+JSClass js_UnknownPropertyClass = { >+ "UnknownProperty", >+ JSCLASS_HAS_RESERVED_SLOTS(1) | Why not JSCLASS_HAS_PRIVATE? >+ JSCLASS_IS_ANONYMOUS | JSCLASS_HAS_CACHED_PROTO(JSProto_UnknownProperty), JSCLASS_IS_ANONYMOUS on the previous line? This line does not hit column 79 so it's ok by that style rule, but it reads a bit better to me. >+ JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, JS_PropertyStub, >+ JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, JS_FinalizeStub, >+ NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL No need to trace the atom at reserved slot index 0? Bigger question: could you avoid an object and anonymous class by using a pseudo-boolean? It would have to encode a JSString *, but it seems do-able. Make NoSuchMethod faster for sure. >+ proto = JS_InitClass(cx, obj, NULL, &js_UnknownPropertyClass, NULL, 0, NULL, Nit: 80 columns in use here, if I'm not mistaken -- wrap after 0, and that looks better with the four NULLs in a row on the overflow line. >+ * When JSOP_CALLPROP does not find the method property of the object, we >+ * wrap property's id into an object of UnknownProperty class and store the >+ * object into callee's stack slot. Later js_Invoke will recognise such object >+ * and transfer control to NoSuchMethod. IIRC the old code handled JSOP_NAME. Looking ahead: >- pc = (jsbytecode *) vp[-(intN)fp->script->depth]; >- switch ((JSOp) *pc) { >- case JSOP_NAME: >- case JSOP_GETPROP: >- case JSOP_CALLPROP: >- GET_ATOM_FROM_BYTECODE(fp->script, pc, 0, atom); Was this deadwood, a bogus case? Or did, e.g. with(o)noSuchMethod() work given o.__noSuchMethod__? > if (clasp != &js_FunctionClass) { >+#if JS_HAS_NO_SUCH_METHOD >+ if (clasp == &js_UnknownPropertyClass) { >+ ok = NoSuchMethod(cx, argc, vp, flags); >+ goto out2; >+ } >+#endif Nice to move nSM off the critical (function-calling) path. > BEGIN_CASE(JSOP_LEAVEBLOCK) > { > #ifdef DEBUG > jsval *blocksp = fp->spbase + OBJ_BLOCK_DEPTH(cx, > fp->blockChain > ? fp->blockChain > : fp->scopeChain); > >- JS_ASSERT(fp->spbase <= blocksp && blocksp <= fp->spbase + depth); >+ JS_ASSERT(fp->spbase <= blocksp && >+ blocksp <= fp->spbase + script->depth); This could be simplified to something like: JS_ASSERT((uintN)(blocksp - fp->spbase) <= script->depth); but you prolly need jsuword on the left instead of uintN for 64-bit systems (LLP64 and LP64 compilation models). >+#define LOCAL_ASSERT(cond, onfail) \ >+ JS_BEGIN_MACRO \ >+ if (!(cond)) { \ >+ JS_ASSERT(cond); \ >+ onfail; \ >+ } \ >+ JS_END_MACRO This isn't used consistently in new code -- look for JS_ASSERTs in the patch below this point (I won't cite them). >+ restart: >+ LOCAL_ASSERT(script->main <= pc && pc < script->code + script->length, >+ goto do_fallback); We should restart at most once, right? >+ * Prepare computing pcstack containing PC pointers to opcodes that >+ * populated interpreter's stack with its current content. Suggest "Prepare computing a stack of pointers to opcodes that populated the interpreter's current operand stack." >+ * We know the address of opcode that triggered the exception. Find s/of opcode/of the opcode/ s/exception/diagnostic/ >+ * JSOP_BINDNAME is special: it generates a value, the base object of >+ * a reference. But if it is the generating op for a diagnostic For less ragged right, suggest "generating opcode" instead of "generating op". >+ * From this point the control must flow through the release_pcstack >+ * label. The code nulls pc before "goto release_pcstack" to jump to >+ * do_fallback after releasing pcstack. Suggest "From this point, control must flow through the release_pcstack label." And "We null pc before ... and jump to do_fallback after releasing pcstack." >+ /* >+ * We search from limit to base to find the most recently calculated >+ * value matching v under assumption that it is it that caused >+ * exception, see bug 328664. s/under assumption/under the assumption/ s/that it is it that caused exception/that it is what caused this diagnostic/ >+ */ >+ JS_ASSERT((size_t) (fp->sp - fp->spbase) <= fp->script->depth); Here's an assert like the one I wanted in jsinterp.c's LEAVEBLOCK case -- size_t is good too, arguably better than uintN and jsuword. C99's uptrdiff_t would be best I suppose, but we can't count on it yet. >+ JS_free(cx, pcstack); >+ >+ /* >+ * We know the pc that caused the exception but we do not know the >+ * stack depth and the decompilation limits for *pc. >+ */ >+ spindex = JSDVG_IGNORE_STACK; >+ goto restart; Paranoia and readability favor nulling pcstack after freeing it. >+ } >+#undef LOCAL_ASSERT >+ >+ release_pcstack: >+ if (pcstack) >+ JS_free(cx, pcstack); >+ if (!pc) >+ goto do_fallback; Have to consider a goto reduction for js_DVG -- it might help to have a static helper subroutine. Not important unless you find something a lot cleaner. Great to get this patch in, finally win back the perf I intentionally lost all those years ago in order to have better diagnostics. I should have done this then. /be
(In reply to comment #13) > Bigger question: could you avoid an object and anonymous class by using a > pseudo-boolean? Yes, and one do not even need to encode JSString for that as encoding the atom index should be sufficient. On the other hand a separated object allows to call noSuchMethod for x[y]() case... Ok, that is for a separated bug. > IIRC the old code handled JSOP_NAME. Looking ahead: > > >- pc = (jsbytecode *) vp[-(intN)fp->script->depth]; > >- switch ((JSOp) *pc) { > >- case JSOP_NAME: > >- case JSOP_GETPROP: > >- case JSOP_CALLPROP: > >- GET_ATOM_FROM_BYTECODE(fp->script, pc, 0, atom); > > Was this deadwood, a bogus case? Or did, e.g. with(o)noSuchMethod() work given > o.__noSuchMethod__? It was a dead wood. The non-existing name is reported before the noSuchMethod would have a chance to be called. > This isn't used consistently in new code -- look for JS_ASSERTs in the patch > below this point (I won't cite them). > > >+ restart: > >+ LOCAL_ASSERT(script->main <= pc && pc < script->code + script->length, > >+ goto do_fallback); > > We should restart at most once, right? Yes. > >+ JS_free(cx, pcstack); > >+ > >+ /* > >+ * We know the pc that caused the exception but we do not know the > >+ * stack depth and the decompilation limits for *pc. > >+ */ > >+ spindex = JSDVG_IGNORE_STACK; > >+ goto restart; > > Paranoia and readability favor nulling pcstack after freeing it. The restart sets pcstack almost immediately. > > >+ } > >+#undef LOCAL_ASSERT > >+ > >+ release_pcstack: > >+ if (pcstack) > >+ JS_free(cx, pcstack); > >+ if (!pc) > >+ goto do_fallback; > > Have to consider a goto reduction for js_DVG -- it might help to have a static > helper subroutine. gotos makes the patch smaller ;) But you are right, a helper simulator routine should make things easier to follow and avoids restart:
(In reply to comment #13) > No need to trace the atom at reserved slot index 0? That is traced by js_TraceObject which traces all the slots.
Attached patch v2 (obsolete) (deleted) — Splinter Review
The new version that addresses the nits in jsopcode.c but still uses a class to wrap the unknown method property.
Attachment #306972 - Attachment is obsolete: true
Attachment #306972 - Flags: review?(brendan)
Comment on attachment 307164 [details] [diff] [review] v2 >+ * When JSOP_CALLPROP does not find the method property of the object, we s/object/base object/ >+ * wrap property's id into an object of UnknownProperty class and store the s/property's/the property's/ s/store the/store this/ >+ * object into callee's stack slot. Later js_Invoke will recognise such object s/such object/such an object/ >- JS_ASSERT(fp->spbase <= blocksp && blocksp <= fp->spbase + depth); >+ JS_ASSERT((size_t) (blocksp - fp->spbase) <= >+ (size_t) script->depth); I don't think the (size_t) on the right is needed -- shorter unsigned types widen naturally to size_t. See your later JS_ASSERT((size_t) (fp->sp - fp->spbase) <= fp->script->depth); in jsopcode.c. >+ * Find the depth of operand stack when the interpreter reaches the given pc. s/of operand stack/of the operand stack/ >+ * When pcstack is not null, it must be an array with space for at least >+ * script->depth elements. On return the array will contain pointers to >+ * opcodes that populated the interpreter's operand stack. s/operand stack/current operand stack/ >+ * The function cannot raise an exception or error but due to a risk of s/The function/This function/ >+ * potential bugs when modeling the stack the function returns -1 if it >+ * detects an inconsistency in the model. In a debug build such s/such/such an/ >+ if (!(script->main <= pc && pc < script->code + script->length)) { Minor nit: I prefer no logical negation and number line order: if (pc < script->main || script->code + script->length <= pc) >+ * Prepare computing pcstack containing PC pointers to opcodes that "PC pointers" could just be "pointers" without loss. >+ * Walk forward from script->main and compute the stack depth and pcstack >+ * content. Instead of "pcstack content", say more what it means: "stack of operand-generating opcode PCs in pcstack." >+#undef LOCAL_ASSERT_RV Just double-checking that JS_ASSERT usage remaining is intentional. r/a=me with nits picked. Pleas land ASAP modulo tree rules. Thanks! /be
Attachment #307164 - Flags: review+
Attachment #307164 - Flags: approval1.9+
Without the patch: js> (let (a=undefined) a).b = 3 typein:1: TypeError: (let (a = undefined) a) is undefined With the patch: js> (let (a=undefined) a).b = 3 typein:5: TypeError: (void 0) is undefined
Igor: feel free to do another patch to fix that case if it's easy, else a followup bug. Jesse: thanks, keep 'em coming, sooner the better. /be
(In reply to comment #18) > Without the patch: > > js> (let (a=undefined) a).b = 3 > typein:1: TypeError: (let (a = undefined) a) is undefined > > With the patch: > > js> (let (a=undefined) a).b = 3 > typein:5: TypeError: (void 0) is undefined > This is caused by inaccurate modeling of JSOP_LEAVEBLOCKEXPR. jsopcode.tbl should really set ndefs to 1 and the reconstructor should set nuses = number_of_locals + 1. The updated patch will fix this.
(In reply to comment #20) > (In reply to comment #18) > > Without the patch: > > > > js> (let (a=undefined) a).b = 3 > > typein:1: TypeError: (let (a = undefined) a) is undefined > > > > With the patch: > > > > js> (let (a=undefined) a).b = 3 > > typein:5: TypeError: (void 0) is undefined > > > > This is caused by inaccurate modeling of JSOP_LEAVEBLOCKEXPR. jsopcode.tbl > should really set ndefs to 1 and the reconstructor should set nuses = > number_of_locals + 1. > > The updated patch will fix this. The precise reason for the bug is that the decompiler wants to to know both the start and the end of the let block. For that it expects that the stack's pc points to leaveblockexpr from which it finds enterblock using a source note. With the current patch it is enterblock that pc will point to.
Attached patch v3 (obsolete) (deleted) — Splinter Review
Changes from v2: 1. The patch is updated to reflect changes from bug 419969. 2. I addressed the nits in jsopcode.c and fixed ReconstructPCStack so the constructed pcstack matches the decompiler expectations regarding the let expressions. 3. I renamed UnknownPropertyClass into NoSuchMethodClass and changed the code to search for noSuchMethod when the property was not found. This way the code reuses not-a-function error reporting. I also added support for noSuchMethod for JSOP_CALLELEM.
Attachment #307164 - Attachment is obsolete: true
Attachment #307270 - Flags: review?(brendan)
Comment on attachment 307270 [details] [diff] [review] v3 > +++ js/src/jsinterp.c 4 Mar 2008 17:51:52 -0000 > @@ -52,16 +52,17 @@ > #include "jsapi.h" > #include "jsarray.h" > #include "jsatom.h" > #include "jsbool.h" > #include "jscntxt.h" > #include "jsconfig.h" > #include "jsdbgapi.h" > #include "jsfun.h" > +#include "jsemit.h" Is this really needed? > + * When JSOP_CALLPROP or JSOP_CALLELEM does not find the method property of > + * the base object, we search for the __noSuchMethod__ method in the base. > + * If it exists, we store the method and the property's id into an object of > + * NoSuchMethod class and store this object into callee's stack slot. Later s/callee's/the callee's/ > + * js_Invoke will recognise such an object and transfer control to (so this line should wrap better ;-) > + * NoSuchMethod that invokes the method like: > + * > + * this.__noSuchMethod__(id, args) > + * > + * where id is the name of the method that this invocation attempted to > + * call by name, and args is an Array containing this invocation's actual > + * parameters. > + */ > +JSBool > +js_OnUnknownMethod(JSContext *cx, jsval *vp) > +{ > + JSObject *obj; > JSTempValueRooter tvr; > jsid id; > JSBool ok; Blank line after declarations end here, before the next line in the post-patch file: > + JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1])); > + obj = JSVAL_TO_OBJECT(vp[1]); > + JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr); Don't need a tvr here for a single root -- use vp[0]! > #endif /* !defined js_invoke_c__ */ > > +/* LocalWords: NoSuchMethod > + */ Any reason for this? > +/* > + * Find the depth of the operand stack when the interpreter reaches the given > + * pc. When pcstack is not null, it must be an array with space for at least s/pc/pc in script/ and rewrap should be nice: /* * Find the depth of the operand stack when the interpreter reaches the given * pc in script. When pcstack is not null, it must be an array with space for * at least script->depth elements. On return the array will contain pointers * to opcodes that populated the interpreter's current operand stack. */ > + * This function cannot raise an exception or error but due to a risk of > + * potential bugs when modeling the stack the function returns -1 if it Commas before "but due" and "the stack". > @@ -5063,47 +5079,85 @@ js_DecompileValueGenerator(JSContext *cx > /* Pop [exception or hole, retsub pc-index]. */ > JS_ASSERT(nuses == 0); > nuses = 2; > } else if (op == JSOP_LEAVEBLOCK || op == JSOP_LEAVEBLOCKEXPR) { > JS_ASSERT(nuses == 0); > nuses = GET_UINT16(pc); > } > pcdepth -= nuses; > - JS_ASSERT(pcdepth >= 0); > + LOCAL_ASSERT(pcdepth >= 0); So here, if op is JSOP_LEAVEBLOCKEXPR, the model stack is empty of all locals and ready for the ndefs=1 result to be pushed. Minus-prefixed lines deleted below: > > ndefs = cs->ndefs; > if (op == JSOP_FINALLY) { > /* Push [exception or hole, retsub pc-index]. */ > JS_ASSERT(ndefs == 0); > ndefs = 2; > } else if (op == JSOP_ENTERBLOCK) { > JSObject *obj; > > JS_ASSERT(ndefs == 0); > GET_OBJECT_FROM_BYTECODE(script, pc, 0, obj); > JS_ASSERT(OBJ_BLOCK_DEPTH(cx, obj) == pcdepth); > ndefs = OBJ_BLOCK_COUNT(cx, obj); > } > > + LOCAL_ASSERT(pcdepth + ndefs <= script->depth); > + if (pcstack) { > + intN i; > + jsbytecode *pc2; > + > + /* > + * Fill the slots that the opcode defines withs its pc unless it > + * just reshuffle the stack. In the latter case we want to > + * preserve the opcode that generated the original value. > + */ > + switch (op) { > + default: > + for (i = 0; i != ndefs; ++i) > + pcstack[pcdepth + i] = pc; So why can't this default case handle JSOP_LEAVEBLOCKEXPR, instead of writing the following special case code? > + case JSOP_LEAVEBLOCKEXPR: > + /* > + * The decompiler wants [leaveblockexpr], not [enterblock], to > + * be left on pcstack after a simulated let expression. > + */ > + JS_ASSERT(ndefs == 0); > + LOCAL_ASSERT(pcdepth >= 1); > + LOCAL_ASSERT(*pcstack[pcdepth - 1] == JSOP_ENTERBLOCK); > + pcstack[pcdepth - 1] = pc; > + break; > + } > } > - js_DestroyPrinter(jp); > + pcdepth += ndefs; Here's the pcdepth "push" we want, so it seems to me no case JSOP_LEAVEBLOCKEXPR is required. /be
Attachment #307164 - Flags: review+
Attachment #307164 - Flags: approval1.9+
(In reply to comment #23) > > + JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1])); > > + obj = JSVAL_TO_OBJECT(vp[1]); > > + JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr); > > Don't need a tvr here for a single root -- use vp[0]! Here vp[0] stores the id that must survive noSuchMethod lookup. For JSOP_CALLELEM it can be the only root. > > > @@ -5063,47 +5079,85 @@ js_DecompileValueGenerator(JSContext *cx > > /* Pop [exception or hole, retsub pc-index]. */ > > JS_ASSERT(nuses == 0); > > nuses = 2; > > } else if (op == JSOP_LEAVEBLOCK || op == JSOP_LEAVEBLOCKEXPR) { > > JS_ASSERT(nuses == 0); > > nuses = GET_UINT16(pc); > > } > > pcdepth -= nuses; > > - JS_ASSERT(pcdepth >= 0); > > + LOCAL_ASSERT(pcdepth >= 0); > > So here, if op is JSOP_LEAVEBLOCKEXPR, the model stack is empty of all locals > and ready for the ndefs=1 result to be pushed. The problem is that ndefs == 0 for JSOP_LEAVEBLOCKEXPR. I tried to change that to 1, but it broke stack depth asserts in the interpreter and would require to patch AFAICS both jsemit.c and js_Interpret.
Attached patch v4 (obsolete) (deleted) — Splinter Review
This version fixes nits from the comment 23. Here is an interdiff from the previous patch: ~/m/ff/mozilla/js/src/> quilt diff -z --- /home/igor/m/ff/mozilla/quilt.W24609/js/src/jsopcode.c 2008-03-04 20:13:48.000000000 +0100 +++ js/src/jsopcode.c 2008-03-04 20:13:11.000000000 +0100 @@ -4766,12 +4766,12 @@ js_DecompileFunction(JSPrinter *jp) /* * Find the depth of the operand stack when the interpreter reaches the given - * pc. When pcstack is not null, it must be an array with space for at least - * script->depth elements. On return the array will contain pointers to - * opcodes that populated the interpreter's current operand stack. + * pc in script. When pcstack is not null, it must be an array with space for + * at least script->depth elements. On return the array will contain pointers + * to opcodes that populated the interpreter's current operand stack. * - * This function cannot raise an exception or error but due to a risk of - * potential bugs when modeling the stack the function returns -1 if it - * detects an inconsistency in the model. In a debug build such an - * inconsistency triggers an assert. + * This function cannot raise an exception or error. However, due to a risk of + * potential bugs when modeling the stack, the function returns -1 if it + * detects an inconsistency in the model. Such an inconsistency triggers an + * assert in a debug build. */ static intN --- /home/igor/m/ff/mozilla/quilt.W24609/js/src/jsinterp.c 2008-03-04 20:13:48.000000000 +0100 +++ js/src/jsinterp.c 2008-03-04 20:02:54.000000000 +0100 @@ -58,5 +58,4 @@ #include "jsdbgapi.h" #include "jsfun.h" -#include "jsemit.h" #include "jsgc.h" #include "jsinterp.h" @@ -872,6 +871,6 @@ js_InitNoSuchMethodClass(JSContext *cx, * the base object, we search for the __noSuchMethod__ method in the base. * If it exists, we store the method and the property's id into an object of - * NoSuchMethod class and store this object into callee's stack slot. Later - * js_Invoke will recognise such an object and transfer control to + * NoSuchMethod class and store this object into the callee's stack slot. + * Later js_Invoke will recognise such an object and transfer control to * NoSuchMethod that invokes the method like: * @@ -7027,5 +7026,2 @@ interrupt: #endif /* !defined js_invoke_c__ */ - -/* LocalWords: NoSuchMethod - */
Attachment #307270 - Attachment is obsolete: true
Attachment #307283 - Flags: review?(brendan)
Attachment #307270 - Flags: review?(brendan)
I've run jsfunfuzz on v2 and v3. Haven't found any new crashes. Searching the jsfunfuzz logs for things like "(void 0) is undefined" resulted in me finding comment 18 and several bugs that also existed before the patch (bug 420837, bug 420839, bug 420919, bug 420925).
Comment on attachment 307283 [details] [diff] [review] v4 >+ * Later js_Invoke will recognise such an object and transfer control to Final nit: comma after "Later" and r/a=me. Thanks again, with the bug marked blocking1.9 you are clear to commit. /be
Attachment #307283 - Flags: review?(brendan) → review+
Attached patch v4b (deleted) — Splinter Review
The new version addresses the nit from the comment 27.
Attachment #307283 - Attachment is obsolete: true
Attachment #307330 - Flags: review+
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
This changed the behavior of e4x/extensions/regress-312196.js. See bug 355258.
Igor, Where is JSProto_NoSuchMethod defined? With this patch JS won't compile for me: 'JSProto_NoSuchMethod' : undeclared identifier This occurs on line 849 of jsinterp.c.
(In reply to comment #31) > Igor, > > Where is JSProto_NoSuchMethod defined? > > With this patch JS won't compile for me: > 'JSProto_NoSuchMethod' : undeclared identifier Make sure that you have updated/rebuild all the sources. The constant is defined by jsproto.tbl when it is included by jspubtd.h, http://lxr.mozilla.org/seamonkey/source/js/src/jspubtd.h#96
$ grep NoSuchMeth jsproto.tbl # define NO_SUCH_METHOD_INIT js_InitNoSuchMethodClass JS_PROTO(NoSuchMethod, 32, NO_SUCH_METHOD_INIT) /be
Yes that was it. I was grepping only .h and .c files. Sorry for the false alarm! Does this patch really boost speed by 5%? If so awesome!
(In reply to comment #34) > Does this patch really boost speed by 5%? 5% is for my Linux laptop ;). For some Windows installation it was 4%.
Depends on: 421806
/cvsroot/mozilla/js/tests/js1_7/regress/regress-420399.js,v <-- regress-420399.js initial revision: 1.1 covers comment 18 but not the overall patch.
Flags: in-testsuite+
Flags: in-litmus-
Depends on: 422864
v 1.9.0 js1_7/regress/regress-420399.js only.
Status: RESOLVED → VERIFIED
Depends on: 429739
Depends on: 457521
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: