Closed Bug 385729 Opened 13 years ago Closed 13 years ago

Separated table to store script objects

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9alpha8

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 15 obsolete files)

250.84 KB, patch
igor
: review+
Details | Diff | Splinter Review
Currently objects representing nested functions, regexps etc for a script are stored as atoms and referenced through indexes in script's atom map. This is inefficient as it always creates a new entry for each object in the global atom table. This comes from the fact that objects can not be shared between scripts as the hash key for the object is its pointer.

I suggest to remove this unnecessary atomization of objects and instead store them directly in a per-script table.
Attached patch implementation v 0.1 (obsolete) — Splinter Review
Backup of untested implementation prototype that just stores regexps in the object table.
It's a good point that there is no sharing yet for object atoms among scripts. But there could be -- the idea would be to make object-tagged atom key (jsval) hashing content-based, not address-based. Then we would see some gains for common XUL and Ajax/Web functions, maybe. Worth a quick test to measure?

/be
Attached patch implementation v 0.5 (obsolete) — Splinter Review
Another backup of untested implementation. But this time it compiles and even runs.
(In reply to comment #2)
> It's a good point that there is no sharing yet for object atoms among scripts.
> But there could be -- the idea would be to make object-tagged atom key (jsval)
> hashing content-based, not address-based. Then we would see some gains for
> common XUL and Ajax/Web functions, maybe.

Currently those candidates for sharing are cloned during execution. Perhaps it would be better to avoid creating the original objects and just keep their structures in the shared cache. Then instead of cloning one can just wraps the structures.

For example, one do not need cloned-in-any-case RegExp objects, just its compiled bytecode. Then one can share the bytecode and wraps the regexps on demand.

But this is for another bug. For now I would like to determine the benefits of unsharing. I will test the last patch to remove any regressions and then do the measurements.
Status: NEW → ASSIGNED
Attached patch implementation v 0.6 (obsolete) — Splinter Review
The new version deatomize xml objects in addition to regexps and nested functions and passes the test suite without regressions.
Attachment #269660 - Attachment is obsolete: true
Attachment #269847 - Attachment is obsolete: true
Attached patch implementation v 0.9 (obsolete) — Splinter Review
The new version puts to the object table all users of js_AtomizeAtom calls and removes ATOM_IS_OBJECT/ATON_TO_OBJECT calls. Strangely enough it passes the test suite. Now it is time to do browser tests and measurements.
Attachment #269928 - Attachment is obsolete: true
Attached patch implementation v1 (obsolete) — Splinter Review
The patch removes atomization of objects and store them directly in a new JSScript table. To track objects during parsing they are wrapped into JSParsedObjectBox struct with a couple of link fields. One link fields is for GC tracing and another is to put objects to JSCodeGenerator list so the emitter knows which objects to put to the object table. Since this removed all the user of js_AtomizeAtom, the patch removes internal API for object atoms altogether.

To support more then 64K of objects the patch reuses JSOP_ATOMBASE and friends machinery for the object table indexes. In view of that the index segment bytecodes should probably be renamed to JSOP_INDEXBASE etc. but this is not done to keep the patch smaller.

The patch increses XDR magic index and removes the support for reading scripts with older magic. The 1.8 and trunk already effectively require the version match for the scripts since with the same bytecode format new versions introduces changes their runtime property. Thus it does not make sence to pretend that the scripts with ancient magic can be executed in XDRScript.
Attachment #269979 - Attachment is obsolete: true
Attachment #270145 - Flags: review?(brendan)
Attached patch implementation v2 (obsolete) — Splinter Review
In the new version I renamed ATOMBASE* into INDEXBASE* bytecodes and removed asserts from the interpreter cases for the bytecode that the index index register should stay bellow atomMap.length. With the separated object map this is no longer true.

I will ask for a review after regression testing.
Attachment #270145 - Attachment is obsolete: true
Attachment #270145 - Flags: review?(brendan)
Attached patch implementation v3 (obsolete) — Splinter Review
New patch fixes some bugs and passes the test suite and allows to read gmail at least.
Attachment #270159 - Attachment is obsolete: true
Attachment #270186 - Flags: review?(brendan)
When a browser starts to show the default initial page, the number of atomized objects is in my case 3158. Starting GMail brings that number to 3500. So the patch saves in this case 3500 calls to lock/malloc/unlock to allocate hash entries.
Would like this by 1.9b1, so will review next.

/be
Target Milestone: --- → mozilla1.9beta1
(In reply to comment #4)
> Currently those candidates for sharing are cloned during execution. Perhaps it
> would be better to avoid creating the original objects and just keep their
> structures in the shared cache. Then instead of cloning one can just wraps the
> structures.

That would save an object per function or regexp literal, but in the case of functions, it would entail to some extent reinventing the property storage used for formal parameters and local variables. And in both cases it would mean yet another ad-hoc rooting technique.

> For example, one do not need cloned-in-any-case RegExp objects, just its
> compiled bytecode. Then one can share the bytecode and wraps the regexps on
> demand.

Another point in favor of the compiler creating prototypical objects: we can use these objects, no cloning required, when doing "compile-and-go" evaluation. Even when compiling once and executing many times, if the same scope is used for the compilation and all executions, we can avoid cloning on every execution.

/be
Comment on attachment 270186 [details] [diff] [review]
implementation v3


>+            fun = (JSFunction *)JS_GetPrivate(cx,
>+                                              script->objects->elems[index]);

Nit: would prefer to break after the cast, put the call entire on one (second) line.

More in a bit, I will finish the review later today.

/be
Comment on attachment 270186 [details] [diff] [review]
implementation v3

js_atom_uninterner: could eliminate single-use rt local -- your call.

No ATOM_INDEX_LIMIT, checked by js_InitAtomMap previously? Beware reaching it can break decompilation because of inherent srcnote representation limit (SN_3BYTE_OFFSET_FLAG). Ah, so JSMSG_TOO_MANY_LITERALS is raised elsewhere, before we index atoms. Then add an assertion to js_InitAtomMap that count is < ATOM_INDEX_LIMIT.

" * The slots are initially uninitialized (they hold void in the function case" -- the initially uninitialized reads wrong -- how about "The slots initially contain undefined or null"?

EmitObjectOp could be a one-liner (its body, I mean).

Nit: pn_funpob would be more in keeping with all-lowercase naming for pn_*. Same for funpob local in jsparse.c (instead of funPob) and blockpob -- these acronyms become their own abbreviation words (like "grep" in Unix, which is an acronym of sorts), and if short enough are better run together all lowercase.

EmitObjectOp should assert op is not JSOP_STRING or kin, EmitAtomOp the opposite (op is not JSOP_OBJECT or the other object ops).

FinishTakingParsedObjects => FinishParsedObjects

Is it sound to use cx->tempPool to allocate pobs?  That is, does a pob ever outlive any older allocation from cx->tempPool?

Wish numGlobalVars and nregexps naming conventions matched; I favor ngvars for the former. Can do?

Comment under do_lookup_switch:

"             * indexes of its atoms would exceed 64K limit."

s/indexes of its atoms/any atom index in it/

Typo: "             * code bellow we need the absolute value."

Lose #if 0 bogo-code at bottom of JSOP_DEFFUN -- same for #if 0 bogo-code at bottom of JSOP_CLOSURE, unless you see a way to make it work.

Nit: blank line after

            LOAD_FUNCTION(VARNO_LEN);

in JSOP_DEFLOCALFUN case.

Nit: combine these two lines:

            --i;
            if (array->elems[i] == obj)

as the predecrement in the index operation won't have any unwanted effects, only the wanted one.

Nit: since you are changing this line:

     * dereferencing a local that's undefined or null.  Search script->objects

feel free to remove the extra space after period, to match new prevailing punctuation style ;-). Same for entire paragraph comment starting

 * A literal is indexed by a per-script atom or object maps.  Most scripts have

in jsopcode.h.

Would be good to get Jesse's decompiler fuzzer run with this patch.

Typo: "deduce" in " * Get index operand from the bytecode using a bytecode analysis to deducing"

Extra blank lines added at end of jsscan.c?

Nit: in js_XDRScript, "     * array will be written and restored in the outer-to-inner order." -- which array? s/array/script->objects array/ or similar.

I went fast over js_NewScript.

mrbkap might review the block XDR and other block scope changes, to back me up.

Pretty righteous patch -- with the above, some firefox bloatcycle testing (run resource:///res/bloatcycle.html with and without patch to ensure no regression), and some decompiler-fuzzing, it should go in.

/be
Attachment #270186 - Flags: review?(mrbkap)
Attachment #270186 - Flags: review?(brendan)
Attachment #270186 - Flags: review+
Attached patch implementation v4 (obsolete) — Splinter Review
Changes compared with previous version besides addressing the nits:

1. To assert that cx->tempPool with JSParsedObjectBox instances are not released until the trace list of parsed objects is no longer used I added DEBUG-only JSArenaPool.noReleaseMark and JS_GUARD_LAST_ARENA_ALLOC, JS_REMOVE_ARENA_ALLOC_GUARDS macros. It allows to assert that JS_ARENA_REALEASE does not touch any allocated JSParsedObjectBox.

2. To-be-cloned-at-exec regexps are put to a separated array in JSScript so the emitted index for the regexp can be used directly as a slot index for the clone. It removes cloneIndex from JSRegExp and make the later independent from the script it is defined. This can be used later to share common regexps.

3. Not to bloat JSScript with objects, regexps and trunotes word-sized fields that would be null pointers for most of JSScript instances the patch uses byte-sized offsets from JSScript to access object and try note array descriptors.

This patch is only for references, I will ask for a review of an updated version after I land the patch for bug 349326.
Attachment #270186 - Attachment is obsolete: true
Attachment #270186 - Flags: review?(mrbkap)
Attached patch implementation v5 (obsolete) — Splinter Review
The new version uses JSParseContext, a new struct, for the list of parsed objects and to implements the assertion that the allocated JSParsedObjectBox and JSParseNode  instances are not affected by JS_ARENA_RELEASE calls by the emitter.
Attachment #270456 - Attachment is obsolete: true
Attached patch implementation v6 (obsolete) — Splinter Review
Attachment #270520 - Attachment is obsolete: true
Attachment #270932 - Flags: review?
Attachment #270932 - Flags: review?(mrbkap)
(In reply to comment #17)
> Created an attachment (id=270932) [details]
> implementation v6
> 

The last version is v5 with better comments and renames like GET_INDEX->GET_INDEX16 to better reflect the nature of operations. 
Attached patch implementation v6 for real (obsolete) — Splinter Review
The previous patch also got the fix from the bug 386478 comment 7. So here is a clean version.
Attachment #270932 - Attachment is obsolete: true
Attachment #270935 - Flags: review?
Attachment #270932 - Flags: review?(mrbkap)
Attachment #270932 - Flags: review?
Attachment #270935 - Flags: review?(mrbkap)
Comment on attachment 270935 [details] [diff] [review]
implementation v6 for real

Final comments (I didn't have a look at the change that nicely switched to byte offsets for accessing the script's several literal/object/notes table members, or the JSParseContext changes):

INDEX16 is pretty concrete, so much so that the existing UINT16 macros seem better than adding renamed versions. An alternative is to abstract a bit more and stick with INDEX, to match INDEXBASE. I still prefer that.

Nit: underhang the start of the cast with the second line's first non-whitespace character:

            fun = (JSFunction *)
                          JS_GetPrivate(cx, JS_SCRIPT_OBJECT(script, index));

This is how it looks in the bugzilla diff view, sorry if already correct and I'm misreading a diff -b.

Why not make the tvr a member of the pc? They're paired unavoidably and I don't see any difference in lifetime or purpose.

Nit: in

 * to its address. map->length must be already set to the number of atoms on

s/be already/already be/
s/atoms on/atoms in/

Misspelled "literals":

             * as a segment register for object leterals as well.

Nit about preferring xmlpob to xmlPob.

Not sure this will hold up on x86-64 or other 64-bit architectures that force 64-bit struct alignment:

 * it is sizeof(uint32) as the structure consists of 3 uint32 fields.

r=me with these addressed, full testsuite, firefox startup and firefox resource:///res/bloatcycle.html success.

/be
Attachment #270935 - Flags: review? → review+
(In reply to comment #20)
> Why not make the tvr a member of the pc? They're paired unavoidably and I don't
> see any difference in lifetime or purpose.

I had a version that did exactly that, but then it was necessary to move JSTempValueRooter into jsprvtd.h so it can be included in jsparse.h. But then I started to get feeling that the patch size exceeds a sanity limit...
(In reply to comment #21)
> (In reply to comment #20)
> > Why not make the tvr a member of the pc? They're paired unavoidably and I don't
> > see any difference in lifetime or purpose.
> 
> I had a version that did exactly that, but then it was necessary to move
> JSTempValueRooter into jsprvtd.h so it can be included in jsparse.h. But then I
> started to get feeling that the patch size exceeds a sanity limit...

I'd rather have JSTempValueRooter move, even to jspubtd.h if we can stand that. But jsprvtd.h otherwise.

I have a huge patch coming, so don't worry about patch size sanity ;-).

/be
(In reply to comment #20)
> Not sure this will hold up on x86-64 or other 64-bit architectures that force
> 64-bit struct alignment:
> 
>  * it is sizeof(uint32) as the structure consists of 3 uint32 fields.

It must be a bad compiler if it needs 64 bits alignment for a struct with 3 uint32 fields. Such alignment requirement would still require less efficient access to the second field unless all the fields are forced to be on 64-bit boundary. But then I doubt that SM would even run. Hence I prefer to keep that assert about:

JS_STATIC_ASSERT(sizeof(JSTryNote) == 3 * sizeof(uint32)); 

to detect such bad compiler or compilation options ASAP.
I'm not worried -- the JS_STATIC_ASSERT will cover any future porting problems. Thanks,

/be
Attached patch implementation v7 (obsolete) — Splinter Review
Here is a version of the patch that addresses the nits from comment 20. It passes the test suite for js shell. Now its time for browser/bloat tests.
Attachment #270935 - Attachment is obsolete: true
Attachment #270935 - Flags: review?(mrbkap)
Attachment #270969 - Flags: review?(mrbkap)
Jesse, can you fuzz-test a build with the latest patch here applied to your tree?

/be
Attached patch implementation v8 (obsolete) — Splinter Review
This is v7 sync with CVS tip.
Attachment #270969 - Attachment is obsolete: true
Attachment #271153 - Flags: review?(mrbkap)
Attachment #270969 - Flags: review?(mrbkap)
Attached patch implementation v8b (obsolete) — Splinter Review
The new version fixes bad English is JSParsedObjectBox comments.
Attachment #271153 - Attachment is obsolete: true
Attachment #271154 - Flags: review?(mrbkap)
Attachment #271153 - Flags: review?(mrbkap)
Attachment #271154 - Flags: review?(mrbkap) → review+
With the patch, each of these statements causes a crash [@ Decompile]:

uneval(eval("(function f () /x/g)"))

uneval(eval("({get f () /x/g})"))
(In reply to comment #29)
> With the patch, each of these statements causes a crash [@ Decompile]:
> 
> uneval(eval("(function f () /x/g)"))
> 
> uneval(eval("({get f () /x/g})"))
> 

The fuzzer rules, what else can I say, pointing to my stupid mistake and a hole in the coverage of  the test suite.
Attached patch implementation v9 (obsolete) — Splinter Review
The changes in the new version:

1. It fixes he bug detected by fuzzier in jsopcode.c where I used LOAD_REGEXP both for JSOP_REGEXP and JSOP_OBJECT.

2. It removes code in jsgc.c that called gcThingCallback for objects referenced by atoms.

3. It changes script object and regexp access macros to lvalue style with better assert coverage. In this way the bug detected by the fuzzier triggers the assert.

I will regress-test the new version and ask for a review after that.
Attachment #271154 - Attachment is obsolete: true
Question about resource:///res/bloatcycle.html :

I presume that one wants to compare TraceMalloc results as described in  http://www.mozilla.org/projects/xpcom/MemoryTools.html with and without the patch applied, not XPCOM_MEM_BLOAT_LOG and related tests as the latter do not measure JS malloc usage. Too bad that currently --enable-trace-malloc does not compile for me.

I will file a bug about it later, but in the mean time note that the patch strictly decreases memory usage. In cases when JSScrip + support structure now fatter then previously due to extra space for JSObjectArray the extra bloat is less then the saving comming from removal of at least one JSAtom instance.
Comment on attachment 271359 [details] [diff] [review]
implementation v9

Now the patch passed the testsuite for the shell.
Attachment #271359 - Flags: review?(brendan)
Comment on attachment 271359 [details] [diff] [review]
implementation v9

>+         * Here we should workaround gcThingCallback deficiency of only being
>+         * able to handle GC things, not atoms.

"only" after "handle", not before "being".

> For that it is necessary to
>+         * call the callback on all GC things referenced by atoms. For
>+         * unmarked atoms it is done during the tracing of things the atom
>+         * refer to, but for already marked atoms we have to call the callback
>+         * explicitly.

Avoid passive voice: "Because of this we must call the callback on all .... For unmarked atoms we call when tracing things reached directly from each such atom, but for already-marked atoms we have to call the callback explicitly.

>+         *
>+         * We do not do it currently for compatibility with XPCOM cycle
>+         * collector which ignores JSString * and jsdouble * GC things that
>+         * the atom can refer to.

Want FIXME citing double hashing atoms bug, and maybe also tracing for better cycle collection bug if appropriate.

>+        if (type == JOF_CONST) {
>+            JS_GET_SCRIPT_ATOM(script, index, atom);
>+            v = ATOM_KEY(atom);
>+        } else if (type == JOF_OBJECT) {
>+            JS_GET_SCRIPT_OBJECT(script, index, obj);
>+            v = OBJECT_TO_JSVAL(obj);
>+        } else {
>+            JS_GET_SCRIPT_REGEXP(script, index, obj);
>+            v = OBJECT_TO_JSVAL(obj);
>+        }

Could use outer if-else with inner if-else to set obj, followed by common v setting.

r=me with these nits picked.

/be
Attachment #271359 - Flags: review?(brendan) → review+
Patch to commit with the last addressed.
Attachment #271359 - Attachment is obsolete: true
Attachment #271384 - Flags: review+
Comment on attachment 271384 [details] [diff] [review]
implementation v9b

One last nit I forgot (lost due to bugzilla timeout -- session history doesn't save "Edit Attachment As Comment" data, grr):

>+EmitIndexConstOp(JSContext *cx, JSOp op, uintN slot, uintN index,
>                  JSCodeGenerator *cg)

Shouldn't this be named EmitIndexObjectOp now?

/be
(In reply to comment #36)
> (From update of attachment 271384 [details] [diff] [review])
> One last nit I forgot (lost due to bugzilla timeout -- session history doesn't
> save "Edit Attachment As Comment" data, grr):
> 
> >+EmitIndexConstOp(JSContext *cx, JSOp op, uintN slot, uintN index,
> >                  JSCodeGenerator *cg)
> 
> Shouldn't this be named EmitIndexObjectOp now?

This should be EmitSlotIndexOp since  it is used to emit both JOF_INDEXCONST and JOF_INDEXOBJECT. It also suggests to rename JOF_INDEX(CONST|OBJECT) into JOF_SLOT(CONST|OBJECT).
(In reply to comment #37)
> This should be EmitSlotIndexOp since  it is used to emit both JOF_INDEXCONST
> and JOF_INDEXOBJECT. It also suggests to rename JOF_INDEX(CONST|OBJECT) into
> JOF_SLOT(CONST|OBJECT).

Is it OK?
The name INDEXCONST came from combination of (slot) index and const(atom index), but the new trope is to call the latter "index", so "slot" for the former is ok with me. Even better might be to eliminate CONST in favor of ATOM, so we have ATOM or OBJECT following SLOT.

/be
(In reply to comment #39)
> The name INDEXCONST came from combination of (slot) index and const(atom
> index), but the new trope is to call the latter "index", so "slot" for the
> former is ok with me. Even better might be to eliminate CONST in favor of ATOM,
> so we have ATOM or OBJECT following SLOT.

Separate patch for a followup bug is ok too.

/be
Blocks: 387286
I committed the patch from comment 35 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.155; previous revision: 3.154
done
Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.336; previous revision: 3.335
done
Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.34; previous revision: 3.33
done
Checking in jsarena.h;
/cvsroot/mozilla/js/src/jsarena.h,v  <--  jsarena.h
new revision: 3.26; previous revision: 3.25
done
Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.99; previous revision: 3.98
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.70; previous revision: 3.69
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.151; previous revision: 3.150
done
Checking in jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.96; previous revision: 3.95
done
Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.264; previous revision: 3.263
done
Checking in jsemit.h;
/cvsroot/mozilla/js/src/jsemit.h,v  <--  jsemit.h
new revision: 3.78; previous revision: 3.77
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.199; previous revision: 3.198
done
Checking in jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.37; previous revision: 3.36
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.224; previous revision: 3.223
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.71; previous revision: 3.70
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.357; previous revision: 3.356
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.362; previous revision: 3.361
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.254; previous revision: 3.253
done
Checking in jsopcode.h;
/cvsroot/mozilla/js/src/jsopcode.h,v  <--  jsopcode.h
new revision: 3.49; previous revision: 3.48
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.98; previous revision: 3.97
done
Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.292; previous revision: 3.291
done
Checking in jsparse.h;
/cvsroot/mozilla/js/src/jsparse.h,v  <--  jsparse.h
new revision: 3.44; previous revision: 3.43
done
Checking in jsprvtd.h;
/cvsroot/mozilla/js/src/jsprvtd.h,v  <--  jsprvtd.h
new revision: 3.26; previous revision: 3.25
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.155; previous revision: 3.154
done
Checking in jsregexp.h;
/cvsroot/mozilla/js/src/jsregexp.h,v  <--  jsregexp.h
new revision: 3.26; previous revision: 3.25
done
Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.129; previous revision: 3.128
done
Checking in jsscan.h;
/cvsroot/mozilla/js/src/jsscan.h,v  <--  jsscan.h
new revision: 3.49; previous revision: 3.48
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.149; previous revision: 3.148
done
Checking in jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.37; previous revision: 3.36
done
Checking in jsxdrapi.c;
/cvsroot/mozilla/js/src/jsxdrapi.c,v  <--  jsxdrapi.c
new revision: 1.33; previous revision: 1.32
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.31; previous revision: 1.30
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.160; previous revision: 3.159
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to comment #40)
> (In reply to comment #39)
> > Even better might be to eliminate CONST in favor of ATOM,
> > so we have ATOM or OBJECT following SLOT.
> 
> Separate patch for a followup bug is ok too.

See bug 387286.
Blocks: 386885
test from comment 29:
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-385729.js,v  <--  regress-385729.js
initial revision: 1.1
Flags: in-testsuite+
Depends on: 387994
Depends on: 396326
(In reply to comment #43)
> test from comment 29:
> /cvsroot/mozilla/js/tests/js1_8/extensions/regress-385729.js,v  <-- 

verified fixed 1.9.0 linux/mac*/windows. marking bug verified although this test doesn't cover the overall issue.
Status: RESOLVED → VERIFIED
Depends on: 407024
Depends on: 666481
You need to log in before you can comment on or make changes to this bug.