375270 - GC: separating traversal and marking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

[This is a spin-off of bug 372960 comment 21]

The current code in SpiderMonkey that implements the marking phase of GC effectively consists of 2 independent parts:

1. The traversal of GC-related children of GC-controllable things like gcthings, atoms and JSScopeProperty instances.

2. The marking of GC-related things.

If we can separate the traversal from marking making GC just a one of the user of the marking code, it would allow to reuse the traversal code in the XPCOM cycle collector (see bug 372960 for the example of code duplication).

Another advantage would be the ability to print a graph of live GC-related things without running GC.

Comment 1

[Igor Bukanov]

Attachment: Implementation v0.1

Here is completely untested prototype.

Comment 2

[Peter Van der Beken [:peterv]]

CC'ing graydon, he did work on a patch to record the JS graph so he might have some thoughts. The patch seems to recurively walk JS objects, which probably means we can't use it during traversal but need to record the edges first.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> CC'ing graydon, he did work on a patch to record the JS graph so he might have
> some thoughts. The patch seems to recurively walk JS objects, which probably
> means we can't use it during traversal but need to record the edges first.

The idea behind the patch is to call a callback on each GC-thing child that is stored in a particular entity (like JSContext, other GC things etc.) It is up to the callback implementation to do the recursion via a call js_GCTraverseChildren. In particular, the default implementation, representing the marking phase of GC) does not recurse on low C stack and rather put the GC thing to a queue to traverse later.

Note also that the next version of the patch would remove the exposure of atoms to traversal callback, only GC thinks will be visible to simplify code both in SM and XPConnect.

Comment 4

[Igor Bukanov]

Attachment: Implementation v0.2

In this version the traversal callback does not need to deal with atoms and API like js_TraverseRuntime and js_TraverseContext are added. 

The patch also contains a new code to dump GC heap independently from running GC.

This is completely untested again.

Comment 5

[Igor Bukanov]

Attachment: Implementation v0.3

The new versions adds to the js shell heap() function to dump the current heap. The function is available only if GC_MARK_DEBUG is defined. In principle it is possible to define it whenever DEBUG is defined, but that would cause name calculations during GC marking phase. I will need to check if this would slow down the debug build significantly.

Comment 6

[Igor Bukanov]

Attachment: Implementation v1

The version that does not regress and should allow to implement in a relatively straightforward way. Note that currently there is no integration with XPConnext as  there the implementations of various mark hooks there heavily mix traversal and marking

Comment 7

[Peter Van der Beken [:peterv]]

Attachment 259721 [details] [diff] doesn't compile for me:

js/src/jsgc.c: In function 'js_GC':
js/src/jsgc.c:2849: error: 'acx' undeclared (first use in this function)
js/src/jsgc.c:2849: error: (Each undeclared identifier is reported only once
js/src/jsgc.c:2849: error: for each function it appears in.)
js/src/jsgc.c:2942: error: 'iter' undeclared (first use in this function)

Probably need |JSContext *acx, *iter;| at the top of js_GC.

Comment 8

[Igor Bukanov]

Attachment: Implementation v2

My initial hope that it would be possible to just reuse the current implementations of JSClass.mark does not work as is. The reason for that is an obvious fact that a mark implementation, besides traversing through the object graph, can alter the state (like storing marking bits for its private data). 

Thus it is necessary to mark those JSClass.mark that do not change the state besides calling JS_MarkGCThing when the traversal is not from GC marking phase. So in the new version of the patch I added JSCLASS_MARK_IS_TRAVERSE to denote JSClass.mark functions that are aware of the traversal. Currently only SpiderMonkey classes are marked with the flag meaning the non-GC traversal would never go into XPConnect objects, but I will cover that in a separated xpconnect patch.

Another change is elimination of GC_MARK_DEBUG. In the new version when DEBUG is defined the traversal callback receives a special function to print the details  about a gc thing location in the object graph. Thus even with DEBUG a potentially heavy sprintf is run only when the traversal callback asks for it. The the function "heap" that I added to jsshell in the previous patch is available in any DEBUG build.

The patch compiles with JS_THREADSAFE defined.

Comment 9

[Igor Bukanov]

Attachment: Implementation v3

Here is a less over engineered implementation with hopefully better names. It also provides a more useful version of heap function in js shell which is now defined as:

heap(graphTop, fileName, maxDepth, thingToIgnore)

graphTop: either a value to start the graph or null to indicate full runtime dump
fileName: name of the file for the report or null for stdout
maxDepth: null for no recursion checks or the value to limit how deep to recurse int the graph
thingToIgnore: value to ignore during the traversal

Here is an example demonstrating the utility of the function:

js> heap(Math, null, 2, this)
heap(Math, null, 2, this)
GLOBAL 0x8505420
08506120 object 0x8506140 Math via top(Math @ 0x08506120).
08502280 string SQRT1_2 via top(Math @ 0x08506120).id 'SQRT1_2'(string).
08502270 string SQRT2 via top(Math @ 0x08506120).id 'SQRT2'(string).
08502260 string PI via top(Math @ 0x08506120).id 'PI'(string).
08502250 string LN10 via top(Math @ 0x08506120).id 'LN10'(string).
08502240 string LN2 via top(Math @ 0x08506120).id 'LN2'(string).
08502230 string LOG10E via top(Math @ 0x08506120).id 'LOG10E'(string).
08502220 string LOG2E via top(Math @ 0x08506120).id 'LOG2E'(string).
08502210 string E via top(Math @ 0x08506120).id 'E'(string).
08502200 string tan via top(Math @ 0x08506120).id 'tan'(string).
085021f8 string sqrt via top(Math @ 0x08506120).id 'sqrt'(string).
085021f0 string sin via top(Math @ 0x08506120).id 'sin'(string).
085021e8 string round via top(Math @ 0x08506120).id 'round'(string).
085021e0 string random via top(Math @ 0x08506120).id 'random'(string).
085021d8 string pow via top(Math @ 0x08506120).id 'pow'(string).
085021d0 string min via top(Math @ 0x08506120).id 'min'(string).
085021c8 string max via top(Math @ 0x08506120).id 'max'(string).
085021c0 string log via top(Math @ 0x08506120).id 'log'(string).
085021b8 string floor via top(Math @ 0x08506120).id 'floor'(string).
085021b0 string exp via top(Math @ 0x08506120).id 'exp'(string).
085021a8 string cos via top(Math @ 0x08506120).id 'cos'(string).
085021a0 string ceil via top(Math @ 0x08506120).id 'ceil'(string).
08502198 string atan2 via top(Math @ 0x08506120).id 'atan2'(string).
08502190 string atan via top(Math @ 0x08506120).id 'atan'(string).
08502188 string asin via top(Math @ 0x08506120).id 'asin'(string).
08502180 string acos via top(Math @ 0x08506120).id 'acos'(string).
08502178 string abs via top(Math @ 0x08506120).id 'abs'(string).
08501e28 string toSource via top(Math @ 0x08506120).id 'toSource'(string).
08505500 object 0x8505520 Object via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).
08501f18 string __lookupSetter__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__lookupSetter__'(string).
08501f10 string __lookupGetter__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__lookupGetter__'(string).
08501f08 string __defineSetter__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__defineSetter__'(string).
08501f00 string __defineGetter__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__defineGetter__'(string).
08501ef8 string propertyIsEnumerable via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'propertyIsEnumerable'(string).
08501ef0 string isPrototypeOf via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'isPrototypeOf'(string).
08501ee8 string hasOwnProperty via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'hasOwnProperty'(string).
08501ee0 string unwatch via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'unwatch'(string).
08501ed8 string watch via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'watch'(string).
08501d98 string eval via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'eval'(string).
08501e40 string valueOf via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'valueOf'(string).
08501e38 string toLocaleString via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'toLocaleString'(string).
08501e30 string toString via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'toString'(string).
08501d88 string __count__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__count__'(string).
08501e00 string __parent__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__parent__'(string).
08501e08 string __proto__ via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id '__proto__'(string).
08501d80 string constructor via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).id 'constructor'(string).
08505520 object 0x85078a0 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).constructor(Function @ 0x08505520).
08505540 object 0x85078b8 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).toSource(Function @ 0x08505540).
08505560 object 0x85078d0 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).toString(Function @ 0x08505560).
08505580 object 0x85078e8 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).toLocaleString(Function @ 0x08505580).
085055a0 object 0x8507900 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).valueOf(Function @ 0x085055a0).
085055c0 object 0x8507918 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).eval(Function @ 0x085055c0).
085055e0 object 0x8507930 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).watch(Function @ 0x085055e0).
08505600 object 0x8507948 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).unwatch(Function @ 0x08505600).
08505620 object 0x8507960 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).hasOwnProperty(Function @ 0x08505620).
08505640 object 0x8507978 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).isPrototypeOf(Function @ 0x08505640).
08505660 object 0x8507990 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).propertyIsEnumerable(Function @ 0x08505660).
08505680 object 0x85079a8 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).__defineGetter__(Function @ 0x08505680).
085056a0 object 0x85079c0 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).__defineSetter__(Function @ 0x085056a0).
085056c0 object 0x85079d8 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).__lookupGetter__(Function @ 0x085056c0).
085056e0 object 0x85079f0 Function via top(Math @ 0x08506120).__proto__(Object @ 0x08505500).__lookupSetter__(Function @ 0x085056e0).
08506140 object 0x8508118 Function via top(Math @ 0x08506120).toSource(Function @ 0x08506140).
08501ed0 string call via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'call'(string).
08501ec8 string apply via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'apply'(string).
08501de8 string name via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'name'(string).
08501dd0 string length via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'length'(string).
08501d70 string caller via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'caller'(string).
08501d60 string arity via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'arity'(string).
08501d58 string arguments via top(Math @ 0x08506120).toSource(Function @ 0x08506140).id 'arguments'(string).
08508118 private 0x8508118 via top(Math @ 0x08506120).toSource(Function @ 0x08506140).private().
08505440 object 0x8507888 Function via top(Math @ 0x08506120).toSource(Function @ 0x08506140).__proto__(Function @ 0x08505440).
08506160 object 0x8508130 Function via top(Math @ 0x08506120).abs(Function @ 0x08506160).
08508130 private 0x8508130 via top(Math @ 0x08506120).abs(Function @ 0x08506160).private().
08506180 object 0x8508148 Function via top(Math @ 0x08506120).acos(Function @ 0x08506180).
08508148 private 0x8508148 via top(Math @ 0x08506120).acos(Function @ 0x08506180).private().
085061a0 object 0x8508160 Function via top(Math @ 0x08506120).asin(Function @ 0x085061a0).
08508160 private 0x8508160 via top(Math @ 0x08506120).asin(Function @ 0x085061a0).private().
085061c0 object 0x8508178 Function via top(Math @ 0x08506120).atan(Function @ 0x085061c0).
08508178 private 0x8508178 via top(Math @ 0x08506120).atan(Function @ 0x085061c0).private().
085061e0 object 0x8508190 Function via top(Math @ 0x08506120).atan2(Function @ 0x085061e0).
08508190 private 0x8508190 via top(Math @ 0x08506120).atan2(Function @ 0x085061e0).private().
08506200 object 0x85081a8 Function via top(Math @ 0x08506120).ceil(Function @ 0x08506200).
085081a8 private 0x85081a8 via top(Math @ 0x08506120).ceil(Function @ 0x08506200).private().
08506220 object 0x85081c0 Function via top(Math @ 0x08506120).cos(Function @ 0x08506220).
085081c0 private 0x85081c0 via top(Math @ 0x08506120).cos(Function @ 0x08506220).private().
08506240 object 0x85081d8 Function via top(Math @ 0x08506120).exp(Function @ 0x08506240).
085081d8 private 0x85081d8 via top(Math @ 0x08506120).exp(Function @ 0x08506240).private().
08506260 object 0x85081f0 Function via top(Math @ 0x08506120).floor(Function @ 0x08506260).
085081f0 private 0x85081f0 via top(Math @ 0x08506120).floor(Function @ 0x08506260).private().
08506280 object 0x8508208 Function via top(Math @ 0x08506120).log(Function @ 0x08506280).
08508208 private 0x8508208 via top(Math @ 0x08506120).log(Function @ 0x08506280).private().
085062a0 object 0x8508220 Function via top(Math @ 0x08506120).max(Function @ 0x085062a0).
08508220 private 0x8508220 via top(Math @ 0x08506120).max(Function @ 0x085062a0).private().
085062c0 object 0x8508238 Function via top(Math @ 0x08506120).min(Function @ 0x085062c0).
08508238 private 0x8508238 via top(Math @ 0x08506120).min(Function @ 0x085062c0).private().
085062e0 object 0x8508250 Function via top(Math @ 0x08506120).pow(Function @ 0x085062e0).
08508250 private 0x8508250 via top(Math @ 0x08506120).pow(Function @ 0x085062e0).private().
08506300 object 0x8508268 Function via top(Math @ 0x08506120).random(Function @ 0x08506300).
08508268 private 0x8508268 via top(Math @ 0x08506120).random(Function @ 0x08506300).private().
08506320 object 0x8508280 Function via top(Math @ 0x08506120).round(Function @ 0x08506320).
08508280 private 0x8508280 via top(Math @ 0x08506120).round(Function @ 0x08506320).private().
08506340 object 0x8508298 Function via top(Math @ 0x08506120).sin(Function @ 0x08506340).
08508298 private 0x8508298 via top(Math @ 0x08506120).sin(Function @ 0x08506340).private().
08506360 object 0x85082b0 Function via top(Math @ 0x08506120).sqrt(Function @ 0x08506360).
085082b0 private 0x85082b0 via top(Math @ 0x08506120).sqrt(Function @ 0x08506360).private().
08506380 object 0x85082c8 Function via top(Math @ 0x08506120).tan(Function @ 0x08506380).
085082c8 private 0x85082c8 via top(Math @ 0x08506120).tan(Function @ 0x08506380).private().
08502208 double 2.71828 via top(Math @ 0x08506120).E(double).
08502218 double 1.4427 via top(Math @ 0x08506120).LOG2E(double).
08502228 double 0.434294 via top(Math @ 0x08506120).LOG10E(double).
08502238 double 0.693147 via top(Math @ 0x08506120).LN2(double).
08502248 double 2.30259 via top(Math @ 0x08506120).LN10(double).
08502258 double 3.14159 via top(Math @ 0x08506120).PI(double).
08502268 double 1.41421 via top(Math @ 0x08506120).SQRT2(double).
08502278 double 0.707107 via top(Math @ 0x08506120).SQRT1_2(double).
js>

Comment 10

[Igor Bukanov]

Attachment: Implementation v4

The new patch exposes sufficient information through jsapi.h to allow to switch xpconnect js marking code to a more generic traversal code (it will be covered in a separated patch). In the new patch JS_MarkGCThing is allowed to be called only when the traversal is a part of GC marking phase. In other cases embeddings should call JS_TraverseGCThing. 

For classes with JSCLASS_MARK_IS_TRAVERSE their mark member should be in fact JSTraverseOp with the signature:

typedef void
(* JS_DLL_CALLBACK JSTraverseOp)(JSGCTraversal *gct, JSObject *obj);

In addition, the patch switches XML to override JSClass.mark, not JSObjectOps.mark for simpler code and changes xml_traverse (former xml_mark) to trim arrays only when called as a part of GC marking traversal.

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> The new patch exposes sufficient information through jsapi.h to allow to switch
> xpconnect js marking code to a more generic traversal code (it will be covered
> in a separated patch).

Ok, so that will allow us to do away with most of nsXPConnect::Traverse. Cool.

I used v3 to write a patch to fix bug 375063 (calling js_TraverseContext from the nsJContext traversal code), works great.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 260015 [details] [diff] [review]
Implementation v4

Looks good. Lots of fussing about names below, they are important to get right in APIs so please bear with me.

* DumpHeap => Heap to match "heap" function name, or else use "dumpHeap" as name
* "GLOBAL %p\n", thingToIgnore -- what does "GLOBAL" signify here?
* js_NewGCHeapDumper return value not null checked
* odd that heap's first arg can be true false 42 etc. (int or boolean) to turn off heap traversal ye
t also not traverse from a given gc-thing; perhaps instead fail with diagnostic if args would result in an empty dump file
* JS_GetGCMarkingTraversal uses "Marking" instead of "Mark" unlike other APIs/macros -- how come?  * Might want "marking" as the modifier for "traversal" throughout, even though longer -- otherwise ambiguous (is "mark" a verb or an adjective?)
* JSGCTraversal comments
  * struct FIXME comments of course
  * open brace on same line as struct keyword
  * member function pointers should be typdef'ed in jspubtd.h with DLL magic decorators  * Nit: members should be indented (with * declarator mode if pointer) to line up as other struct members (ignoring JSPrincipals) are in js*.h
* _DETAILES misspelled
* I prefer "allAtoms" to "assumeAllLocked" in jsatom.c TraverseArgs, etc. Shorter, avoids spreading
the bad "locked" metaphor (should have called it pinned, once)
* js_TraceWatchPoints makes me want to use "trace" instead of "traverse" as the metaphor -- shorter
and I think more precise/common in GC jargon; "trace" can be verb or noun, so no "traversal" variati
on needed
* "assmbled" misspelled
* Slight preference for gct to always be JSGCTraversal * (modulo Trace renaming), implying JSGCDumpT
r* => gcdt for the var name -- being systematic helps grep-ability and maintenance
* Spell bufsize consistently (s/bufSize/bufsize/g I think -- better to avoid camelCaps for short/abb
reviated/canonical compound names)
* Nit:
        end = (obj->map->ops->mark && (IS_GC_MARK_TRAVERSAL(gct)
                                       || obj->map->ops->mark == js_Mark))
  looks better with a line break after the &&, and indentation accordingly afterward.
* Isn't the above condition reducing the ability to customize JSObjectOps.mark for non-marking purposes?  * I'm asking why not just change JSObjectOps.mark to JSObjectOps.trav^Hce ;-)  * IOW let's start bending the API compatibility with JSObjectOps, which was never fully public    * because it depends on jsobj.h to be usefully implemented or varied/extended from native default

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: review comments, unmangled by copy/paste from vim

Sorry, not sure why some newlines disappeared.

/be

Comment 14

[Igor Bukanov]

Attachment: Implementation v5

The patch addresses the issues from the attachment 260049 [details] except FIXME comments: I prefer to write them when the patch will finalize.

The patch replaces "mark" in JSObjectOps by "trace" where the default implementation (js_TraceObject function) also marks the slot array. It allowed to move JSObject-specific code to jsobj.c. Similarly I moved js_TraceAtom to jsatom.c.

In addition the patch adds debug-only 

extern JS_PUBLIC_API(void)
JS_PrintTracedThingInfo(char *buf, size_t bufSize, JSGCTracer *gti,
                        void *thing, uintN type, JSBool details);

so an embedding can easily build own tracer with reasonable debug info without decoding the meaning of type.

This also raised a question: should type be GCX_TYPE? 

Perhaps it is better to replace that with a enum that matches the tags of jsval plus few internal numbers for xml things. Then JS_TraceGCThing/GC_TRACE will be modified to require that the type parameter and the callback invocation code would assert the type against the corresponding GCX_TYPE. In this way non-gc traversal would not need to to call js_GetGCThingFlags at all.

Comment 15

[Igor Bukanov]

Comment on attachment 260082 [details] [diff] [review]
Implementation v5

I new version is coming: this version did incomplete Traverse->Trace replace.

Comment 16

[Brendan Eich [:brendan]]

s/\<gti\>/gct/g everywhere? Also look out for remnant bufSize spellings.

I didn't quite see the GCX_* reordering motivation, although I got a sense of what you were trying to achieve -- could you explain it more? Thanks,

/be

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> I didn't quite see the GCX_* reordering motivation, although I got a sense of
> what you were trying to achieve -- could you explain it more? Thanks,

This comes from the following observation: during XPCOM refcounting or debug tracing one would not need to call js_GetGCThingsFlags if GC_TRACE/JS_TraceGCThing (GC_MARK/JS_MarkGCThing) would include a type argument to be passed to the tracing callback.

Moreover, if that type argument would be decoupled from GCX_TYPE, then XPConnect would not need to include jsgc.h, jsobj.h and jsxml.h. Similarly, one would be able to write a debug dump tracer without the need to include these headers as long as SpiderMonkey provides a debug-only helper to sprint info about the thing based on its type.

Effectively from a traversal callback implemention point of view the type argument is an abstract entity that should be passed to JS_TraverseGCThingChildren when the callback decides to enumerate the children of the thing it receives. Plus it can be used to extract DEBUG-only strings about the thing. 

On the other hand for implementations of JSClass.mark/trace hook the type is rather concrete since the implementations need to know what to pass to GC_TRACE when enumerating things stored in JSObject. But since the only things that an embedding can pass to GC_MARK is JSObject*, JSString* or jsdouble*, the trace type constants exactly for these 3 things should be exposed is jsapi.h. 

Farther in future SpiderMonkey can even provide an API to register a new traversal types so en embedding callback can make to participate non-GC-things in the tracing. For example, it would be really nice if the tracing can go into XPCOM nodes (using already present traverse code) so a dumping tracer would print the real graph of all reachable things.

Comment 18

[Igor Bukanov]

Attachment: Implementation v6

Summary of changes:

1. I dropped GC prefix from the tracing function/macros and renamed JSGCTracer *gct into JSTracer *trc as the GC is just a particular user of the tracing code.

2. JSClass.marl/trace implementations should call JS_Traceing/JS_TRACING/JS_DETAILED_TRACING to call the tracing callback on each of their child refrences.

2. A trace callback implementation calls JS_TraceChildren(trc, thing) to trace things referenced by thing. In general the convention is that "Tracing" means to call the tracing callback while "Trace" like in "TraceSomething"  means to enumerate references stored in Something. Hopefully this does not screw English too much but I am really open to suggestions about better naming here.

3. I added JS_TRACING_VALUE/JS_TRACING_OBJECT/JS_TRACING_STRING/JS_TRACING_DOUBLE type-safe macros wrapping JS_Tracing to simplify the transition to the typed call to JS_Trace (see comment 17).

4. I changed the code to call JS_Tracing (replacing current js_MarkGCThing) only when the thing is not null to avoid useless null checks in JS_Tracing.

5. I added JS_PrintTraceThingInfo that a tracing callback implementation can call for debugging. It allows to write a dumping tracer that refers only to jsapi.c

6. JS_Tracing/JS_TraceChildren are implemented directly in jsgc.c to avoid introducing extra stack frame which can be precious. Note that there is no tail call elimination during the GC marking tracing, but that can be implemented in a generic way later if necessary: one would just need to delay JS_TraceChildren invocation for that in JS_Tracing.

[ Note that the patch also includes the last proposed fix for 375999 to provide cvs diff, not quilt diff.]

Comment 19

[Igor Bukanov]

The current proposal for farther steps:

1. Finalize the patch (especially agree on the naming!)

2. Extend it to include xpconnect code so JSClass.mark/trace implementations there is switched to the new code without touching the XPCOM traversal implementation.

3. Switch XPCOM traversal implementating to use JSTracer.

Comment 20

[Igor Bukanov]

Attachment: Implementation v7

This is the previous patch plus couple of small fixes.

Comment 21

[Igor Bukanov]

Attachment: Type Trace extra

This a quilt patch of untested "typed tracing" prototype build on top of the patch the previous comments.

It adds to jsapi.c JSTK_OBJECT, JSTK_DOUBLE and JSTK_DOUBLE constants to demote traced thing kind. A few extra JSTK_ constants are defined in jsgc.h. They are not supposed to be accessed by embeddings. The constants are feed to the tracing callback and to JS_TraceChildren. The latter uses the constants to call the corresponding js_TraceSomething function. 

The extra constants include JSTK_ATOM which is integrated with the marking code even if it is not a gc thing. Now dumpHeap(Math, null) shows more precise map that includes atoms:

js> dumpHeap(Math, null, null, this)
dumpHeap(Math, null, null, this)
0960a120 Math  960a140
09613740 atom SQRT1_2	via top(Math @ 0x0x960a120)
09606280 string SQRT1_2	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613740)
09613708 atom SQRT2	via top(Math @ 0x0x960a120)
09606270 string SQRT2	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613708)
09613a20 atom PI	via top(Math @ 0x0x960a120)
09606260 string PI	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613a20)
096139f0 atom LN10	via top(Math @ 0x0x960a120)
09606250 string LN10	via top(Math @ 0x0x960a120).id(atom @ 0x0x96139f0)
096139c0 atom LN2	via top(Math @ 0x0x960a120)
09606240 string LN2	via top(Math @ 0x0x960a120).id(atom @ 0x0x96139c0)
09613990 atom LOG10E	via top(Math @ 0x0x960a120)
09606230 string LOG10E	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613990)
09613958 atom LOG2E	via top(Math @ 0x0x960a120)
09606220 string LOG2E	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613958)
09613928 atom E	via top(Math @ 0x0x960a120)
09606210 string E	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613928)
09613870 atom tan	via top(Math @ 0x0x960a120)
09606200 string tan	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613870)
09613840 atom sqrt	via top(Math @ 0x0x960a120)
096061f8 string sqrt	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613840)
09613810 atom sin	via top(Math @ 0x0x960a120)
096061f0 string sin	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613810)
096137e0 atom round	via top(Math @ 0x0x960a120)
096061e8 string round	via top(Math @ 0x0x960a120).id(atom @ 0x0x96137e0)
096137b0 atom random	via top(Math @ 0x0x960a120)
096061e0 string random	via top(Math @ 0x0x960a120).id(atom @ 0x0x96137b0)
09613790 atom pow	via top(Math @ 0x0x960a120)
096061d8 string pow	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613790)
096136d8 atom min	via top(Math @ 0x0x960a120)
096061d0 string min	via top(Math @ 0x0x960a120).id(atom @ 0x0x96136d8)
096136a8 atom max	via top(Math @ 0x0x960a120)
096061c8 string max	via top(Math @ 0x0x960a120).id(atom @ 0x0x96136a8)
09613630 atom log	via top(Math @ 0x0x960a120)
096061c0 string log	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613630)
09613600 atom floor	via top(Math @ 0x0x960a120)
096061b8 string floor	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613600)
096135d0 atom exp	via top(Math @ 0x0x960a120)
096061b0 string exp	via top(Math @ 0x0x960a120).id(atom @ 0x0x96135d0)
096135a0 atom cos	via top(Math @ 0x0x960a120)
096061a8 string cos	via top(Math @ 0x0x960a120).id(atom @ 0x0x96135a0)
09613548 atom ceil	via top(Math @ 0x0x960a120)
096061a0 string ceil	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613548)
096134d0 atom atan2	via top(Math @ 0x0x960a120)
09606198 string atan2	via top(Math @ 0x0x960a120).id(atom @ 0x0x96134d0)
096134a0 atom atan	via top(Math @ 0x0x960a120)
09606190 string atan	via top(Math @ 0x0x960a120).id(atom @ 0x0x96134a0)
09613458 atom asin	via top(Math @ 0x0x960a120)
09606188 string asin	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613458)
09613428 atom acos	via top(Math @ 0x0x960a120)
09606180 string acos	via top(Math @ 0x0x960a120).id(atom @ 0x0x9613428)
09610e68 atom abs	via top(Math @ 0x0x960a120)
09606178 string abs	via top(Math @ 0x0x960a120).id(atom @ 0x0x9610e68)
09608da0 atom toSource	via top(Math @ 0x0x960a120)
09605e28 string toSource	via top(Math @ 0x0x960a120).id(atom @ 0x0x9608da0)
09609500 Object  9609520	via top(Math @ 0x0x960a120)
0960fcd0 atom __lookupSetter__	via top(Math @ 0x0x960a120).__proto__(Object @ 0x0x9609500)
...

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 260233 [details] [diff] [review]
Type Trace extra

Quick comments:

>     if (thing)
>-        JS_TRACING_GCTHING(trc, thing, name);
>+        JS_TRACING(trc, thing, js_GetGCThingTraversalKind(thing), name);

TRACE is the verb, TRACING would be a gerund (noun), so JS_TRACE and JS_DETAILED_TRACE are the right macro names.

>+/* Trace kinds to pass to JS_Tracing. */
>+
>+#define JSTK_OBJECT     0
>+#define JSTK_DOUBLE     1
>+#define JSTK_STRING     2
>+
>+

Nit: extra empty line above.

Nit: TK has long provenance in "ToolKit" land. Perhaps just JSTRACE_OBJECT or JSTRC_OBJECT, etc.?

>+#define JSVAL_IS_TRACEABLE(v)   (JSVAL_IS_GCTHING(v) && !JSVAL_IS_NULL(v))
>+#define JSVAL_TO_TRACEABLE(v)   (JSVAL_TO_GCTHING(v))
>+#define JSVAL_TRACE_KIND(v)     (JSVAL_TAG(v) >> 1)

Cool -- could these strongly-coupled jsval tests be enforced with JS_STATIC_ASSERTs?

>--- .pc/typed_trace.patch/js/src/jsatom.h	2007-03-31 16:37:05.000000000 +0200
>+++ js/src/jsatom.h	2007-03-31 16:43:49.000000000 +0200
>@@ -340,18 +340,22 @@ js_FreeAtomState(JSContext *cx, JSAtomSt
>  * that no more contexts will be created.  Thus we minimize garbage during
>  * context-free episodes on a runtime, while preserving atoms created by the
>  * JS_Intern*String APIs for the life of the runtime.
>  */
> extern void
> js_FinishAtomState(JSAtomState *state);
> 
> /*
>- * Atom garbage collection hooks.
>+ * Atom tracing and garbage collectio hooks.

Typo.

>+static uint8 GCTypeToTraceKindMap[GCX_NTYPES] = {
>+

Nit: extra empty line.

Looks good otherwise -- the trace-kind abstraction from gc-thing flags seems worth it as you surmised.

/be

Comment 23

[Brendan Eich [:brendan]]

I'll review a consolidated patch tomorrow.

/be

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> TRACE is the verb, TRACING would be a gerund (noun), so JS_TRACE and
> JS_DETAILED_TRACE are the right macro names.

I would like to emphasis the difference between calling the tracing callback and descending into some entity to enumerate the things it stores. My initial hope was that tracing-versus-trace would do the trick. But my assumptions about the meaning of words in English was wrong, so I need some help to pickup the right names. 

So what about using "trace" to mean "call the tracing callback" while using "track" like in "TrackSomething" to mean "enumerate the children references calling 'trace' on each of them"?

Alternatively usings JS_CallTraceCallback, JS_CALL_TRC may work, but then stuff like jS_CALL_OBJECT_TRC or JS_CALL_TRC_OBJECT meaning "call tracing callback with JSObject as argument" looks awkward. 

Comment 25

[Brendan Eich [:brendan]]

How about TRACE_CB or TRACED? TRC is a bit too cyber-cruddy for JS_LONG_MACRO_NAME typical names.

/be

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> How about TRACE_CB or TRACED? TRC is a bit too cyber-cruddy for
> JS_LONG_MACRO_NAME typical names.

Would JS_TRACED have a "already done" stigma when in fact the callback will later call JS_TraceChildren? I am also thinking about JS_CallTracer/JS_CALL_TRACER/JS_CALL_STRING_TRACER/JS_CALL_OBJECT_TRACER or alternatively JS_NotifyTracer/JS_NOTIFY_TRSACER/JS_NOTIFY_OBJECT_TRACER. 

Comment 27

[Igor Bukanov]

Here is yet another possibility to address the naming puzzle. What about keeping JS_TRACE/JS_Trace for the callback and using TraceSomethingThings like TraceScriptThings, TraceContextThing, fun_trace_thing etc.?

Comment 28

[Igor Bukanov]

Another name suggestion: what about using 
JS_NoteTrace
JS_NOTE_TRACE
JS_NOTE_DETAILED_TRACE
JS_NOTE_OBJECT_TRACE
JS_NOTE_STRING_TRACE
JS_NOTE_DOUBLE_TRACE

to denote the calls requiring the callback attention?

Comment 29

[Brendan Eich [:brendan]]

Some precedent for CALL_... in jsscope.h. It's concrete and I think that's always a good thing (NOTE is a bit abstract for no particular reason; confusing in light of source note nouns too).

/be

Comment 30

[Igor Bukanov]

Attachment: Implementation v8

Here is a patch against CVS head that includes that typed tracing extra with the following changes:

1. Method that calls tracing callback are named JS_CALL_TRACER/JS_CallTracer while TraceSomething is kept to mean descend into Something and enumerate edges there that point to other traceable things.

2. A cleanup of debugging names that are passed to JS_CallTracer to emphasis that the name is the name of the edge, not the object it points to.

3. Various small fixes for bugs/typos revealed by the regression testing against js/tests.

Comment 31

[Igor Bukanov]

Attachment: Implementation v9

Compared with the previous patch, this one does not assume that obj->map->ops->trace can not be null. Instead the patch trace all the slots for null tracer. It allows to ensure a source compatibility with liveconnect code.

Comment 32

[Igor Bukanov]

Attachment: Implementation v10

Amother set of small fixes to remove warnings that compiling xpconnect switched to use trace api in JSClass implementations produces. For some reason g++ does not like the asserts in JS_SET_TRACING_DETAILS from the previous patch, so I rmoved them.

Comment 33

[Igor Bukanov]

Attachment: Implementation v11

Couple more small changes to fix a bug and a warning.

Comment 34

[Igor Bukanov]

Attachment: Switching JSClass.mark in xpconnect to trace.

This is just for references/backup. The patch switches all JSClass.mark in xpconnect to be tracing operations. It does not touch the XPCOM traversal/JS GC integration.

I am typing this in a debug browser build running with both patches applied :)

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 260428 [details] [diff] [review]
Switching JSClass.mark in xpconnect to trace.

>+        if (JS_IsGCMarkingTracer(trc))

Nit: in this file (xpcwrappednativejsops.cpp) prevailing style wants if(.


>+JS_STATIC_DLL_CALLBACK(void)
>+XPC_WN_TearOff_Trace(JSTracer *trc, JSObject *obj)
>+{
>+}
>+

Is this needed?

Looks great otherwise, based on the previous big patch.

/be

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 260426 [details] [diff] [review]
Implementation v11

DumpHeap: "first argument is not null or heap-allocated thing" -- need "a" before "heap-allocated thing".
DumpHeap: no point in *rval = JSVAL_VOID, that's the default r.v.

Tracing API FIXME comments -- best to edit the docs at http://developer.mozilla.org/ and minimize docs in jsapi.h comments

JS_PrintTraceThingInfo -- rogue "gti" left over (grep -w gti for others?)

Nit: TraceLockedAtoms initializes JSAtomState *state and has a blank line before it, not after

jscntxt.h: the *gcMarkingTracer declarator is not indented to the same starting column as all the other member declarators

jsfun.c nit:  js_ArgumentsClass initializer has     (JSMarkOp)args_or_call_trace,  NULL -- NULL doesn't line up with other rows, so no need for more than one space before it

Should JS_CallTracer invoke JS_TraceChildren for the (kind == JSTRACE_ATOM) case, or go directoy to js_TraceAtom?

In JS_CallTracer, the two assertions:

    JS_ASSERT(cx->runtime->gcMarkingTracer == trc);
    JS_ASSERT(cx->runtime->gcLevel > 0);

could be moved up to just after the

    if (!IS_GC_MARKING_TRACER(trc)) {
        trc->callback(trc, thing, kind);
        goto out;
    }

FIXME comments in jspubtd.h

jssscope.h nit: #define TRACE_ID(trc, id) js_TraceId(trc, id) -- the alignment with the next line's macro definition is lost

Great patch, I'll stamp a version that fixes the above.

/be

Comment 37

[Igor Bukanov]

Attachment: Implementation v12

This is the previous patch with nits addressed and that calls gcThingCallback during tracing of locked things only when this is GC marking. The patch does not include the documentation. That will be the next version.

In addition the patch fixes the following gcThingCallback discrepancy. In the current CVS tip js_MarkAtom calls gcThingCallback for marked atoms only when atom is object. Yet for unmarked atoms gcThingCallback is called whenever the atom helds a generic GC thing. Since the patch made this discrepancy very obvious, I fixed that to call gcThingCallback for marked atoms whenever the unmraked case will do.

The locked thing tracing code also raised an interesting question. Currently for locked things gcThingCallback is called as many times as the thing is locked. Yet the patch calls the tracer only once and I think this is the right thing to do even  for gcThingCallback.

The reason for that that is that the reference to the locked thing must be held somewhere as otherwise there is no point to lock things. Thus the proper reference count for a locked thing is 1 + N where 1 reflects the fact that JSRuntime stores it through the locking table and N is the number of times the thing is referenced by other entities. This N can have very little if any correlations with the number of times the thing is locked. Hopefully after this patch it landed it would be possible to get rid of gcThingCallback altogether and stop worrying  about this.

Another problem is that the patch would not call the tracer for locked JSString* and jsdouble* as they are not stored in the locking table but rather marked through GCF_LOCK. Perhaps it is time to remove GCF_LOCK all together and always use the hashtable for that?

Comment 38

[Igor Bukanov]

Attachment: Switching JSClass.mark in xpconnect to trace v2

This is the version of xpconnect changes with the nits addressed. This is given here just for references as I will attach the version for a review in a separated bug after resolving this one.

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #37)
> In addition the patch fixes the following gcThingCallback discrepancy. In the
> current CVS tip js_MarkAtom calls gcThingCallback for marked atoms only when
> atom is object. Yet for unmarked atoms gcThingCallback is called whenever the
> atom helds a generic GC thing. Since the patch made this discrepancy very
> obvious, I fixed that to call gcThingCallback for marked atoms whenever the
> unmraked case will do.

Will the XPConnect-hosted implementation of gcThingCallback do the right thing for non-object atoms?

> The locked thing tracing code also raised an interesting question. Currently
> for locked things gcThingCallback is called as many times as the thing is
> locked. Yet the patch calls the tracer only once and I think this is the right
> thing to do even  for gcThingCallback.

See bug 367779 and please explain how this won't regress that bug.

> Another problem is that the patch would not call the tracer for locked
> JSString* and jsdouble* as they are not stored in the locking table but rather
> marked through GCF_LOCK. Perhaps it is time to remove GCF_LOCK all together and
> always use the hashtable for that?

It would be ok to do that in my book; it would make gcThingCallback general, instead of being the gcObjectCallback that it is. OTOH, we don't need more than what it is for the cycle collector. Just want to make sure we don't regress CC or add too much cost.

/be

Comment 40

[Igor Bukanov]

(In reply to comment #39)
> Will the XPConnect-hosted implementation of gcThingCallback do the right thing
> for non-object atoms?

The gcThingCallback is already called for strings and numbers and that change to call the callback for any GC thing stored in the atom was for consistency.   In any case, in a new patch I will reveret that to make sure that the patch does not affect xpconnect in any way. 

> > Currently
> > for locked things gcThingCallback is called as many times as the thing is
> > locked. Yet the patch calls the tracer only once and I think this is the right
> > thing to do even  for gcThingCallback.
> 
> See bug 367779 and please explain how this won't regress that bug.

The patch does not change the number of times gcThingCallback is called for locked things, in that text I was just speculating that the need to call gcThingCallback as many times as number of locks to the thing is probably wrong.

> it would make gcThingCallback general,
> instead of being the gcObjectCallback that it is. OTOH, we don't need more than
> what it is for the cycle collector.

Again, gcThingCallback is already called for JSString and jsdouble, but not always. But I would not change that in the patch as changing the frequency of gcThingCallback calls is clearly for another bug.

Comment 41

[Igor Bukanov]

Attachment: Implementation v13

The new patch that 

1. adds comments
2. makes sure that gcThingCallback is called the same number of times as before the patch
3. uses JS_DEF_CLASS_TRACE macro to convert JSTraceOp to JSMarkOp when initializing JSClass.mark. With GCC 4.0 macro ensures that its argument confirms to JSTraceOp during compilation or linking time. That allowed to catch a bug in the previous version of the patch where I forgot to remove from fun_trace an extra void *arg argiument.

Comment 42

[Peter Van der Beken [:peterv]]

(In reply to comment #40)
> (In reply to comment #39)
> > See bug 367779 and please explain how this won't regress that bug.
> 
> The patch does not change the number of times gcThingCallback is called for
> locked things, in that text I was just speculating that the need to call
> gcThingCallback as many times as number of locks to the thing is probably
> wrong.

It'd still be good to know why you think that the refcount for locked things should be 1 + N and not L + N (L: number of times locked, N: references from other entities). The cycle collector needs to know about all the edges to an object. If two XPCOM objects lock the same JS object we need to record a refcount of 2 for that object, because it means that both XPCOM objects hold a "strong reference" to the JS object. Recording a refcount of 1 will mean ignoring 1 of the 2 edges, potentially unlinking a cycle even though one of the objects in the cycle is still held by an object outside that cycle. That would be bad.

Comment 43

[Peter Van der Beken [:peterv]]

(In reply to comment #37)
> The reason for that that is that the reference to the locked thing must be held
> somewhere as otherwise there is no point to lock things. Thus the proper
> reference count for a locked thing is 1 + N where 1 reflects the fact that
> JSRuntime stores it through the locking table and N is the number of times the
> thing is referenced by other entities. This N can have very little if any
> correlations with the number of times the thing is locked.

Locking means holding a strong reference, treating all locks to an object as one strong reference means that you're ignoring all but one of the strong references (edges in the reference graph). See also comment 42. I think this change would break cycle collection.

Comment 44

[Igor Bukanov]

(In reply to comment #42)
> It'd still be good to know why you think that the refcount for locked things
> should be 1 + N and not L + N (L: number of times locked, N: references from
> other entities).

In fact I think the refcount for locked things should be just 0 + N. One presumably calls JS_LockGCThing to invoke JS_UnlockGCThing in future. As such the thing must be stored in some other places that gives N references. When the number of references drops to 0, the thing should be unlocked to allows for GC to collect it. 

Now, as I understand the bug 367779, currently there is no storage for thing's reference count so the code just uses JS_LockGCThing as a way to store the counting information while rooting the thing. As a temporary solution this is in fact very reasonable.

What I hope for is that the trace patch would allow to remove JS_LockGCThing calls from xpconnect altogether. The idea is to put all XPCOM objects referenced  by JS objects into a bag. Then when the cycle collector needs to get an accurate  reference count for objects in that bag, it would call JS_TraceRuntime to recover the JS-part of the reference count for objects from the bug and to record all the necessary edge information. Clearly with this schema the JS GC must be informed to root JS GC things referenced by any object from the bug, but it is the only rooting that has to be done. No calls like JS_LockGCThing/JS_AddNamedRoot etc. would be necessary.

Comment 45

[Peter Van der Beken [:peterv]]

(In reply to comment #44)
> The idea is to put all XPCOM objects
> referenced  by JS objects into a bag.

> Clearly with this
> schema the JS GC must be informed to root JS GC things referenced by any object
> from the bug, but it is the only rooting that has to be done.

I don't understand how the two are related: first you talk about XPCOM objects referenced from JS, later you talk about JS objects referenced from XPCOM?

Comment 46

[Igor Bukanov]

(In reply to comment #45)
> I don't understand how the two are related: first you talk about XPCOM objects
> referenced from JS, later you talk about JS objects referenced from XPCOM?

Very good point. Here is an updated idea.

There are 2 sets of XPCOM objects, the one referes to JS things and the ones that are referenced by JS things. 

The objects from the first set must be enumerated during JS GC marking phase to mark JS things they refer to. The objects from the second set must be marked to indicate that they have an incomplete reference count as they do not know how many times they are referenced through JS graph (to be precise, they do not know how many times their JS wrappers are referenced). 

The tracing patch in the current form should allow to recover the missing refcounts via calling JS_TraceRuntime without running JS GC. The only problem is that it would add artificial edges from runtime to things put into global root set via AddNameRoot or locked via LockGCThings when the things are referenced by XPCOM objects (object from the first set by definition). Thus a proper solution would require to stop using AddNameRoot/LockGCThings and rather integrate enumeration of XPCOM objects from the first set with GC marking / tracing.

Comment 47

[Brendan Eich [:brendan]]

Comment 46 sketches a world where we don't use GC global roots or locks. That's a fine goal, but it probably doesn't fit in Mozilla 1.9 (Mozilla 2 definitely wants something like it, but with Tamarin's MMgc). But we still need the cycle collector until we actually do away with XPCOM ref-counted allocations and put both the DOM and JS (and DOM-like native objects) under one memory manager.

I'll review v13 now.

/be

Comment 48

[Brendan Eich [:brendan]]

Comment on attachment 261595 [details] [diff] [review]
Implementation v13

s/The new code/All new code/

Typo: bellow

(size_t)-1 indicating that name alone => ... that the name

Next line: a index => an index

Typo: Idicates

Question: is &method necessary for both definitions of JS_DEF_CLASS_TRACE?

I would prefer JS_CLASS_TRACE -- the DEF_ does little more than take up horizontal real estate.

r=me with nits picked.

/be

Comment 49

[Igor Bukanov]

Attachment: Implementation v14

Addressing the nits and fixing JS_CLASS_TRACE macro not to use & when converting method to JSmarkOp as it only necessary to feed to typeof operator.

Comment 50

[Igor Bukanov]

I committed the patch from comment 49 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.136; previous revision: 3.135
done
Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.313; previous revision: 3.312
done
Checking in jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.143; previous revision: 3.142
done
Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.93; previous revision: 3.92
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.59; previous revision: 3.58
done
Checking in jscntxt.c;
/cvsroot/mozilla/js/src/jscntxt.c,v  <--  jscntxt.c
new revision: 3.107; previous revision: 3.106
done
Checking in jscntxt.h;
/cvsroot/mozilla/js/src/jscntxt.h,v  <--  jscntxt.h
new revision: 3.144; previous revision: 3.143
done
Checking in jsdbgapi.c;
/cvsroot/mozilla/js/src/jsdbgapi.c,v  <--  jsdbgapi.c
new revision: 3.89; previous revision: 3.88
done
Checking in jsdbgapi.h;
/cvsroot/mozilla/js/src/jsdbgapi.h,v  <--  jsdbgapi.h
new revision: 3.31; previous revision: 3.30
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.83; previous revision: 3.82
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.184; previous revision: 3.183
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.213; previous revision: 3.212
done
Checking in jsgc.h;
/cvsroot/mozilla/js/src/jsgc.h,v  <--  jsgc.h
new revision: 3.66; previous revision: 3.65
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.344; previous revision: 3.343
done
Checking in jsiter.c;
/cvsroot/mozilla/js/src/jsiter.c,v  <--  jsiter.c
new revision: 3.59; previous revision: 3.58
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.333; previous revision: 3.332
done
Checking in jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.63; previous revision: 3.62
done
Checking in jspubtd.h;
/cvsroot/mozilla/js/src/jspubtd.h,v  <--  jspubtd.h
new revision: 3.78; previous revision: 3.77
done
Checking in jsregexp.c;
/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.146; previous revision: 3.145
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.61; previous revision: 3.60
done
Checking in jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.44; previous revision: 3.43
done
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.140; previous revision: 3.139
done
Checking in jsscript.h;
/cvsroot/mozilla/js/src/jsscript.h,v  <--  jsscript.h
new revision: 3.34; previous revision: 3.33
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.141; previous revision: 3.140
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.154; previous revision: 3.153
done
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision: 3.19; previous revision: 3.18
done

Comment 51

[Peter Van der Beken [:peterv]]

Isn't this wrong:

+#define JSTRACE_DOUBLE  1
+#define JSTRACE_STRING  2

given:

#define GCX_STRING              1               /* JSString */
#define GCX_DOUBLE              2               /* jsdouble */

Comment 52

[Igor Bukanov]

(In reply to comment #51)
> Isn't this wrong:
> 
> +#define JSTRACE_DOUBLE  1
> +#define JSTRACE_STRING  2
> 
> given:
> 
> #define GCX_STRING              1               /* JSString */
> #define GCX_DOUBLE              2               /* jsdouble */

The idea behind JSTRACE_ is to pass enough information to JS_TraceChildren so it can cast to appropriate type and enumerate references without calling GetGCThingFlags, see comment 17. The values for the constants were selected to allow to quickly extract them from the tag of jsval, see JSVAL_TRACE_KIND macro, they are unrelated to GCX_ values.

Comment 53

[Roy Tam]

Igor:
Your changes to 1.8.0 branch ( http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=igor%25mir2.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-12+11%3A31&maxdate=2007-04-16+11%3A31&cvsroot=%2Fcvsroot )
breaks the build, I got this error message:

c:/moz180\mozilla\js\src\jsxml.c(5610) : warning C4047: 'return' : 'JSXML *' differs in levels of indirection from 'int'
c:/moz180\mozilla\js\src\jsxml.c(8050) : warning C4090: 'function' : different 'const' qualifiers
c:/moz180\mozilla\js\src\jsxml.c(8050) : warning C4047: 'function' : 'JSAtom *' differs in levels of indirection from 'JSObject ** '
c:/moz180\mozilla\js\src\jsxml.c(8050) : error C2198: 'js_GetClassPrototype' : too few arguments for call through pointer-to-function
make[4]: *** [jsxml.obj] Error 2

Please check if it works for you or not. Thank you.

Comment 54

[Igor Bukanov]

(In reply to comment #53)
> Igor:
> Your changes to 1.8.0 branch (
> http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=igor%25mir2.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-12+11%3A31&maxdate=2007-04-16+11%3A31&cvsroot=%2Fcvsroot
> )
> breaks the build, I got this error message:


This is for bug 373082, not this bug.

Comment 55

[Roy Tam]

(In reply to comment #54)
> (In reply to comment #53)
> > Igor:
> > Your changes to 1.8.0 branch (
> > http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=igor%25mir2.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-12+11%3A31&maxdate=2007-04-16+11%3A31&cvsroot=%2Fcvsroot
> > )
> > breaks the build, I got this error message:
> 
> 
> This is for bug 373082, not this bug.
> 

No I'm not talking about the warning messages.
the js_GetClassPrototype function in js_GetXMLFunction cause error when compiling:

c:/moz180\mozilla\js\src\jsxml.c(8094) : error C2198: 'js_GetClassPrototype' :
too few arguments for call through pointer-to-function

/** jsxml.c Version 3.50.2.15.2.37 Source - line 8091 **/
    xml = (JSXML *) JS_GetPrivate(cx, obj);
    if (HasSimpleContent(xml)) {
        /* Search in String.prototype to implement 11.2.2.1 Step 3(f). */
/* This -> */        ok = js_GetClassPrototype(cx, js_String_str, &tvr.u.object);
        if (!ok)
            goto out;
        JS_ASSERT(tvr.u.object);
        ok = OBJ_GET_PROPERTY(cx, tvr.u.object, id, vp);
    }

Comment 56

[Igor Bukanov]

To Roy Tam:  please use bug 373082 to discuss issues introduced with the patch fixing the bug on 1.8.0 branch!

Comment 57

[Igor Bukanov]

(In reply to comment #47 by Brendan)
> Comment 46 sketches a world where we don't use GC global roots or locks. That's
> a fine goal, but it probably doesn't fit in Mozilla 1.9 (Mozilla 2 definitely
> wants something like it, but with Tamarin's MMgc). But we still need the cycle
> collector until we actually do away with XPCOM ref-counted allocations and put
> both the DOM and JS (and DOM-like native objects) under one memory manager.

I am not proposing to get away from GC global roots. The goal rather is to allow better (with simpler code and improved efficiency) integration of JS things with the cycle collector.

For example, in places where XPConnect calls AddRoot/LockThings, a more efficient implementation can use an extra word in objects it needs to root to put them on a singly-linked list. Then the JS would be notified about the list nodes through extra tracing API as one proposed in the last patch for bug 340212.

Another possible optimization is to store JSObject and JSXML refcounts in preallocated fields reducing the overhead of their counting. 

But that is for later, for now I would like to switch JSClass instances in xpconnect to the tracing API, bug 377884, make sure that JS_TraceRunrtime  traces all XPConnect things, bug 340212, and help to switch the cycle collector integration to the tracing API, bug 377884. Hopefully it would allow not rely on include <jsgc.h> in xpconnect code.

Comment 58

[Boris Zbarsky [:bzbarsky]]

So... this broke GC_MARK_DEBUG builds (which means all of mine, since it's pretty much impossible to do leak debugging without GC_MARK_DEBUG in my experience).  See bug 378255.

Comment 59

[Igor Bukanov]

(In reply to comment #58)
> So... this broke GC_MARK_DEBUG builds (which means all of mine, since it's
> pretty much impossible to do leak debugging without GC_MARK_DEBUG in my
> experience).  See bug 378255.

With the patch landed one can get the dump just in generic DEBUG build via calling a pair of js_NewGCHeapDumper/js_FreeGCHeapDumper, see http://lxr.mozilla.org/seamonkey/source/js/src/jsgc.h#210 and their usage in http://lxr.mozilla.org/seamonkey/source/js/src/js.c#1352 where js shell defines dumpHeap function. The functions allow to dump heap without running GC.

Right now the dump is incomplete but if you need a complete one right now, just add patches from bug 377884 and bug 340212.

Comment 60

[Igor Bukanov]

Attachment: Fix for invalid fprintf args in DumpHep from js.c

The patch adds missing fileName argument to fprintf in DumpHeap.

Comment 61

[Igor Bukanov]

Attachment: Fix for invalid fprintf args in DumpHep from js.c v2

In the new version I replaced gc: in the error message by dumpheap:

Comment 62

[Igor Bukanov]

I committed the patch for js.c from comment 61 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.137; previous revision: 3.136
done
 

Comment 63

[timeless]

js> help()
JavaScript-C 1.7 pre-release 3 2007-04-01
toint32        evalcx(s[, o])         Evaluate s in optional sandbox object o
    if (s == '' && !o) return new o with eager standard classes
    if (s == 'lazy' && !o) return new o with lazy standard classes
zsh: segmentation fault (core dumped)  ./SunOS5.11_i86pc_DBG.OBJ/js

shell_functions and shell_help_messages have this nice comment:
/* NOTE: These must be kept in sync with the above. */

Which you didn't do :(.

Comment 64

[timeless]

Attachment: fix help()

Comment 65

[Igor Bukanov]

Comment on attachment 262486 [details] [diff] [review]
fix help()

I should remember next time to update the help when adding js functions.

Comment 66

[Igor Bukanov]

To timeless:

Will you commit the js.c fix?

Comment 67

[timeless]

Comment on attachment 262486 [details] [diff] [review]
fix help()

mozilla/js/src/js.c 	3.138