This is a spin-off of bug 377884 to separate the issue of using trace API to discover edges beween JS things versus using trace also for reference counting. While the edge discovery, which is the focus of bug bug 377884, is straightforward with the tracing API, using it also for the reference counting directly may cause performance degradation, see bug 377884 comment 9. Thus it is better to address that in a separated bug.

I have a partial patch for this I think, but it's not working yet. I probably need to first merge in the changes from bug 379455.

This works, if used with the patches for bug 340212 and bug 379220. Note that we need to protect against too much recursion, so we can't just call JS_TraceChildren from within the trace callback (it was nice that the GC took care of this for us when we used the gcThingCallback approach). I used a nsDeque for storing children that need to be traced.

(In reply to comment #2) > Created an attachment (id=264002) [details] > v1 > > This works, if used with the patches for bug 340212 and bug 379220. Note that > we need to protect against too much recursion, so we can't just call > JS_TraceChildren from within the trace callback (it was nice that the GC took > care of this for us when we used the gcThingCallback approach). I used a > nsDeque for storing children that need to be traced. See bug 379973 for an alternative solution.

Some food for thoughts about faster tracing and refcounting of JS things. From the cycle collector point of view there are 2 types of JS roots. The first type is xpcom roots or xpcom objects with refcounter 1 or more that refer to some JS thing. The bug 379220 exposed some of them, but there are others like top scope holders. The second type is JS-execution roots that come from native or script stack frames. There are no other types of roots since if none of the threads run the JS code, then the only roots that are left should come from xpcom objects. By definition, if xpcom object is reachable from JS execution roots, it can not be garbage. So anything reachable form JS-execution roots can be excluded from the refcounting as it can not contribute to garbage. This suggests that the initial tracing should first trace only the execution roots and then mark, not ref count, the reachable things. After that the xpcom roots are traced and only JS things that are not yet marked should be the subject of refcounting and garbage cycle detection.

(In reply to comment #4) > This suggests that the initial tracing should first trace only the execution > roots and then mark, not ref count, the reachable things. After that the xpcom > roots are traced and only JS things that are not yet marked should be the > subject of refcounting and garbage cycle detection. But roots like the global object of a context can be part of cycles, so this requires a good bit of care.

Updated to trunk.

(In reply to comment #4) > This suggests that the initial tracing should first trace only the execution > roots and then mark, not ref count, the reachable things. After that the xpcom > roots are traced and only JS things that are not yet marked should be the > subject of refcounting and garbage cycle detection. I don't think there's currently an API that allows us to do this? And what about comment 5?

This gives about a 5% slowdown in cycle collection times (although it speeds up shutdown collection by about 2-3%).

> (In reply to comment #4) > And what about comment 5? Right, the proposed optimization in comment 4 only makes sense when there is a deep JS execution stack. When the cycle collector runs without any JS stack code executed, there are no roots that can not form cycles.

(In reply to comment #8) > This gives about a 5% slowdown in cycle collection times (although it speeds up > shutdown collection by about 2-3%). So extra tracing is not that cheap. At this point I suggest to add a refcount field to JSObject/JSXML and see how that would help the cycle collector. The savings from bug 392263 reduces the overhead of JSObject by 7 bytes. So it should be OK to use some of these savings for refcount. The situation with JSXML should be similar. The details of the proposal are: 1. SM gains a new compilation option. When it is set, JSOBject/JSXML and any other GC things that can form cycles gain a refcount field. 2. The refcount is calculated during GC and exposed via some API that replaces gcThingCallback. 3. As an optimization another API is provided to delay finalization in JSGC until the cycle collector stops looking for garbage. It should allow to finalize JS objects rooted only through XPCOM garbage cycles without waiting for the following GC.

(In reply to comment #10) > The savings from bug 392263 reduces the overhead of JSObject by 7 bytes. That was wrong. The savings per JSObject from that bug is 3 bytes. Still withe bug fixed refcounted JSObject should not be that bad.

I don't think we should store refcounts in JSObjects. We should try to avoid having to compute JS object reference counts in order to collect cycles, rather. Cc'ing roc and sicking. /be

I haven't analyzed what we're doing here but Jonas suggested that my proposal here: http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/b00bb58076064e71 for integrating Python's cycle collector with mark-and-sweep GC could be adapted here (exploiting the map of C++-to-JS references that XPConnect has).

(In reply to comment #12) > I don't think we should store refcounts in JSObjects. We should try to avoid > having to compute JS object reference counts in order to collect cycles, > rather. Cc'ing roc and sicking. If we would have an algorithm that does not need reference counts for JSObjects, then the same algorithm can be applied to XPCOM objects themselves. Of cause, GC is one such algorithm, but it requires to know the set roots. The cycle collector does not need to know this set as it able to deduce the roots itself. That is, it is able to find objects referenced by something outside the XPCOM graph based on reference counts. I guess this automatic root detections is what makes the algorithm inherently more complex then GC. As i wrote in comment 4, when there is JS code executed, one does not need to refcount anything reachable from JS stack frames. But when there is no JS code executed, then there are no JS roots and JS GC does not collect them only because they directly or indirectly referenced by XPCOM nodes. Then from the cycle collector point of view JS objects are just nodes in the graph and should be treated as other nodes. Currently that requires to know the reference count for the nodes. Hence comes the suggestion about an extra field to store the counter and avoid memory allocation during cycle collection. As far as I can see the only specialty of JS objects that the cycle collector can take advantage of is the fact that in this case of no JS running JS objects are precisely not roots. Maybe some optimizations are possible based on that?

The only way JS and XPCOM objects connect is via XPConnect. Do we really need to model each JS object as a ref-counted node, or could we instead use the XPConnect-maintained wrappers for JS objects, and compress or elide paths through the JS heap altogether? Recall that graydon started the cycle collector before the JS tracing API was generalized from the mark phase and hooks. So he had a very simple gcThingCallback to help ref-count JS objects. But with tracing, we could find all paths that might close cycles from XPCOM back to XPCOM via JS. /be

(In reply to comment #15) > The only way JS and XPCOM objects connect is via XPConnect. Do we really need > to model each JS object as a ref-counted node, or could we instead use the > XPConnect-maintained wrappers for JS objects, and compress or elide paths > through the JS heap altogether? Assuming that the execution JS roots are excluded from the picture, that is, assuming that that the cycle collector knows that any node reachable from JS stack frames is not a garbage, this question can be rephrased in the following way: Can the cycle collector take advantage of knowledge that a particular node is a non-root? I.e., can it use the fact that the node is referenced only by some other nodes and not by the outside world? In particular, can the collector exclude such nodes from the picture somehow or does it need to know the reference counts for the nodes?

(In reply to comment #15) > The only way JS and XPCOM objects connect is via XPConnect. Do we really need > to model each JS object as a ref-counted node, or could we instead use the > XPConnect-maintained wrappers for JS objects, and compress or elide paths > through the JS heap altogether? I think we had a similar discussion already: see https://bugzilla.mozilla.org/show_bug.cgi?id=333078#c27.

(In reply to comment #15) > The only way JS and XPCOM objects connect is via XPConnect. I don't think this is true, there are XPCOM objects that lock/root JS objects directly (see http://mxr.mozilla.org/seamonkey/search?string=HoldScriptObject for example). The cycle collector deals with unknown edges because not all XPCOM objects participate in it, that's why it only collects a cycle if it can match up the edges into an object (represented by the refcount) with the edges it knows about (from traversal). If *all* the edges into JS objects (plain and XPConnect roots) are known to it we might be able to skip refcounting of JS objects. But I don't think that's a possibility (C++ code in addons can hold JS roots for example).

I haven't thought this through very well yet, so this might not work. Suppose we make sure that all JS roots that are created by our C++ code are known to XPConnect. That means there are three types of JS objects: 1) objects that are one or more roots with at least one "unknown" 2) objects that are one or more roots but none of them an "unknown" 3) objects that are not a root In the cycle collector we currently record two "refcounts" for each object, the number of incoming edges into the object that we get from traversing the graph and the real refcount. We set the refcounts of the type 1 JS roots to 0/1. Currently we start traversing the graph only from XPCOM "roots", we need to extend that to start traversing from type 1 JS roots. We set the refcounts of any other JS object that we traverse to 1/1. We'll reach type 2 from traversing XPConnect XPCOM objects (wrappers, etc) and type 3 by traversing type 1 and 2. The rest of the cycle collector remains as is. Objects of type 1 (and anything reachable from them) will be marked black and not be collected. Objects of type 2 that don't have an incoming edge from a black C++ object, and anything reachable from them but not connected to a type 1 object, will be marked white and will be collected. This means we need to make sure to scan all objects that are of type 1 (we need a way to enumerate those) and all XPConnect objects that hold a type 2 JS object. Type 2 should probably somehow include JS contexts. If this works it would avoid doing a traversal of the whole runtime to refcount each object, which attachment 280267 [details] [diff] [review] currently does.

(In reply to comment #19) > In the cycle collector we currently record two "refcounts" for each object, the > number of incoming edges into the object that we get from traversing the graph > and the real refcount. We set the refcounts of the type 1 JS roots to 0/1. I do not thing that it would work since for XPCOM objects referenced by JS objects one would need to know the number of JS edges that points into the object. AFAICS that requires at least traversing the JS graph and assigning refcounts to its nodes. So here is an alternative proposal that should work and will remove the need for gcThingCallback. When the cycle collector needs to know reference counts for JS nodes searching for the garbage, it runs JS_GC in the following way. 1. The cycle collector makes sure that when JS_GC asks to trace application-specific roots, the code does not trace grey nodes. That is, it does not trace grey XPConnect or other XPCOM objects holding JS GC things. 2. When JS GC hits JS object holding XPCOM reference, that XPCOM object immediately turned black. 1-2 ensures that grey nodes does not root anything in JS world while the real JS roots turns grey nodes black. 3. The cycle collector sets the JS GC marking callback (JS GC calls it right before it begins the finalization) to do the reference counting of all JS Objects reachable from grey nodes that were not marked during the previous JS_GC phase. For that the callback uses the tracing API together with that JS_IsAboutToBeFinalized call. 4. With reference counts for JS objects available, the cycle collector completes its cycle while still running inside the JS GC marking callback. 5. Before the cycle collector returns from the callback it calls normal JS tracing code for any grey node that has not become white and which points to JS things. Then the cycle collector exits the callback letting JS_GC to finalize all dead GC things.

(In reply to comment #20) > I do not thing that it would work since for XPCOM objects referenced by JS > objects one would need to know the number of JS edges that points into the > object. AFAICS that requires at least traversing the JS graph and assigning > refcounts to its nodes. We need to traverse them once (record the edges), which the cycle collector already does, but we wouldn't need to do a separate traversal of the whole runtime to record actual refcounts. We just need to know whether a JS object is black or white. Objects that are "unknown" roots or reachable from an unknown root should be black, the cycle collector will color the roots black because of the refcount imbalance (0/1) and will use its normal flooding algorithm to color the reachable nodes black too. Known roots should be colored the same as the XPConnect object that holds them, the flooding algorithm will flood from the XPConnect object and color them correctly too because their refcounts will be in balance (1/1).

(In reply to comment #21) > We need to traverse them once (record the edges) And this will record edges from JS objects to XPCOM objects, just like it does currently.

(In reply to comment #21) > Objects that are > "unknown" roots or reachable from an unknown root should be black, Note that the the above proposal from comment 20 allows to exclude all the "unknown" objects from the picture very early leaving only grey JS objects to consider at tep 3. I forgot to mention that but the step 2 assumes that the flooding algorithm is applied to the nodes turning black during the JS marking. Now all those grey JS objects would have a balanced counter since by the construction they are only reachable from the grey XPCOM objects. Hence they can only become black only via flooding after the corresponding XPCOM turns black. So the only information that is necessary from grey JS objects is knowledge of XPCOM objects (which are necessary grey) reachable through them. And the tracing can do it without any reference counting. Hm, it seems the reference counting for JS is avoided?

(In reply to comment #19) > This means we need to make sure to scan all objects that are of type 1 (we need > a way to enumerate those) and all XPConnect objects that hold a type 2 JS > object. I guess the comment 20 explains how to do exactly that. (In reply to comment #19) > This means we need to make sure to scan all objects that are of type 1 (we need > a way to enumerate those) and all XPConnect objects that hold a type 2 JS > object. Type 2 should probably somehow include JS contexts. The speciality of JSContext is that JS roots stored in it is of type 1 when it is running and is of type 2 when it is not. That is, when some scripts are executed against that JSContext instance, then all the roots are on purpose and anything reachable from them must be black. But when JSContext is not running, then JSObject instances stored in JSContext roots should not participate in the normal cycle collection. Now, there is no explicit support in the tracing API for this differentiation, by a simple workarounds like setting JSContext.globalObject to null before tracing or JS_GC calls should be enough to do the trick. The rest of roots should be empty when the context is not running.

I think our algorithm is very similar, but you're proposing to combine traversal and GC to color unknown roots and objects hanging from them black. Let me try to summarize how I understand your proposal: 1) Run a JS GC starting from the non-XPConnect roots. If we reach XPCOM objects we mark those as "non-flooded" black. 2) From the JS GC callback (for JSGC_MARK_END) we trace the XPConnect roots, anything unmarked is added to the cycle collector as grey. 3) Still from the callback, the cycle collector does all its other coloring (and unlinking?). This will flood the non-flooded black XPCOM objects. If grey JS objects are flood-colored black we also mark them. 4) Return from the callback to let the GC finalize. This will finalize white JS objects. We'll need to figure out a way to pass the cycle collector to the GC tracing code that reaches XPCOM objects. Note that IIRC XPConnect keeps JSContexts alive when the corresponding XPConnect context is alive, so they should participate in cycle collection in that case to avoid cycles through the XPConnect context. I'll try to write a patch.

If we want to make this algorithm closer to what roc proposes we'd need to separate marking and sweeping in the JS_GC. That would also avoid the complex interleaving of cycle collection and GC that the current proposal will cause.

(In reply to comment #25) > I think our algorithm is very similar, but you're proposing to combine > traversal and GC to color unknown roots and objects hanging from them black. > > Let me try to summarize how I understand your proposal: > 1) Run a JS GC starting from the non-XPConnect roots. If we reach XPCOM objects > we mark those as "non-flooded" black. The assigning of the black color must be flooded one. That is, when JS GC sees XPConnect node, the cycle collector should black everything reachable from it and if that would lead to unmarked JS Objects, they must be recursively marked. To exclude XPConnect objects from tracing it would be necessary in void XPCJSRuntime::TraceJS(JSTracer* trc, void* data), http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcjsruntime.cpp#253 , to check if the tracer comes from JS_GC() invoked by the cycle collector. If so, then skip TraceJS calls for any objects participating in the cycle collection. JS_IsGCMarkingTracer(trc) allows to check if the tracer comes from JS_GC() call and to check for the cycle collection asome global flag can be used. > 2) From the JS GC callback (for JSGC_MARK_END) we trace the XPConnect roots, > anything unmarked is added to the cycle collector as grey. Yes. > 3) Still from the callback, the cycle collector does all its other coloring > (and unlinking?). This will flood the non-flooded black XPCOM objects. If grey > JS objects are flood-colored black we also mark them. For that one would need just to call JS_CallTracer(trc, obj, kind) with the trc for JS_GC(). Note thta there is no currently API to get one, but as hack JSContext.runtime->gcMarkingTracer gives exactly that. > 4) Return from the callback to let the GC finalize. This will finalize white JS > objects. Yes. > > We'll need to figure out a way to pass the cycle collector to the GC tracing > code that reaches XPCOM objects. As an alternative an API to do JS_GC in steps can be provided. I.e it is not that dificult to split JS_GC into the set of prepare/mark/finalize/finish functions. > Note that IIRC XPConnect keeps JSContexts alive when the corresponding > XPConnect context is alive, so they should participate in cycle collection in > that case to avoid cycles through the XPConnect context. The only problem is JSContext.globalObject object. I guess if xpconnet would set it to null after calling to JS api, that would solve the problem. > I'll try to write a patch. >

(In reply to comment #26) > If we want to make this algorithm closer to what roc proposes we'd need to > separate marking and sweeping in the JS_GC. That would also avoid the complex > interleaving of cycle collection and GC that the current proposal will cause. > I will try to prepare something like that.

To summarize a general idea here: After non XPCOM JS roots and everything reachable through them are excluded from the picture, the remaining JS objects would only be reachable from grey XPCOM nodes, there would be no other links to them. As such their reference counts will always be balanced. For this reason the only thing the cycle collector needs to know is a list of XPCOM objects reachable through this grey JS set to properly update the counters for the objects on the list. Thus simple traversal of the grey JS set should discover this list, there is no need to reference count.

Here is my current thinking on the extra JS API to simplify life for the cycle collector. JS_GC(cx) should be equivalent to calling: JSGCRun data; JS_BeginGCRun(cx, &data); JS_DoGCMarking(&data); JS_EndGCRun(&data); Then the user of the API should be allowed to do extra JS GC marking/tracing before and after calling JS_DoGCMarking as necessary. In addition, an API would be provided so when JS_DoGCMarking invokes various TraceJS methods in XPConnect code, the code can learn that TraceJS is invoked in response to the cycle collector activity and take appropriate actions. The usage for the API would like: JS_BeginGCRun(cx, &data); <Exclude grey XPConnect nodes from the JS root set> JS_DoGCMarking(&data); <From the remaining grey XPConnect nodes trace JS objects to discover grey XPCOM rechable through grey JS set> <Proceed with cycle collection> JS_EndGCRun(&data); This assumes that whenever the cycle collector recursively blacken a node, it will also recursively mark via calling JS_CallTracer(markingTracer) all JS objects reachable through newly black XPCOM nodes. Is such schema sufficient?

Nits on naming: JS_{Begin,End}GC should be enough (no Run required); the parameter could be JSGCData data; JS_GCMarkPhase might be better than JS_DoGCMarking. /be

I have not found a good way to split JS_GC so far. The problem comes from various recursive JS GC calls or GC running on another thread cases. In those cases the GC does not run but rather asks the already running GC instance to restart itself. Thus JS_GC may return without doing anything. Plus there are cases when JS GC restarts on its own if it thinks that the finalization phase generated more garbage. So spliting JS_GC() is not straightforward as I initially hoped. Thus I suggest for now not use JS_GC but rather implement the schema from comment 30 in terms of tracing API. That is, do something like the following: 1. Use JS_TraceRuntime to flood-black any XPCOM object reachable via JS references from non-grey JS roots. "Grey GC roots" means here any grey XPConnect entity that points to a JS GC thing. 2. Use the tracing API to find the list of XPCOM objects referenced from the remaining XPConnect grey roots and use that list to calculate the reference count balance. This tracing should exclude any JS thing that where reached from non-grey roots at the phase 1. 3. When an xpconnect object is flood-colored black, use the tracing to flood color any XPCOMs reachable via JS refs. Again, the tracing should exclude any JS thing that has already participated in black-coloring. Note that the grey roots should also includes any global object set with JS_SetGlobalObject when the corresponding JS context is not running. When the JS context is running, then the global object must be flood-colored black as it reachable from the executed JS code. AFAICS this specialty of the global object is not addressed by the current code and may even deserve a separated bug.

This is based on the proposal from comment 20, with some tweaks. Needs a lot more testing, but it doesn't introduce new shutdown leaks when starting up with about:blank and shutting down (took me a while to get there). I also haven't done performance measurements yet. Need to figure out if the solution for JSContexts is sufficient. Also probably need to figure out safe unlinking for wrappers, since white JS objects will be finalized when cycle collection ends, so the wrappers need to null out their references to those JS objects.

I tried the patch with the same pages as I did for attachment 280267 [details] [diff] [review] (see comment 8). With the latest patch I see a reduction in cycle collection times between 0% and 30%. In addition, shutdown collection time is reduced by a factor of 5 to 6. This patch still needs a bit of work and it is risky, but I do think we'll want this for 1.9 (and it should fix bug 386912 which is already blocking1.9+).

This makes XPConnect know about all content objects that hold JS things, but doesn't change the cycle collection algorithm. Still gotta add some comments to nsContentUtils.h.

Igor/Crowder can you guys help out here? Like to get this kind of thing in before b1 but the change looks big and scary - so anything we can do to help this along would be great.

I do not see problems with the last patch, but given that in all the cases NS_HOLD_JS_OBJECTS is used as NS_HOLD_JS_OBJECTS(this, class) it should be possible to avoid the hashtable to store the objects and use less expensive double-linked list with next/prev stored in the object itself. See, for example, XPCRootSetElem from xpcprivate.h and its subclasses.

Yeah, I could do that. I do have a question for you. If you look at attachment 285090 [details] [diff] [review] you'll see that I've made JSContext independent objects in the cycle collector. This is because we need to make js_TraceRuntime not trace all the global (and other?) objects held by the contexts, but rather trace them from whatever object is holding the context. Is making js_TraceRuntime only trace contexts if there's no gcExtraRootsTraceOp hook ok? I also still need to solve the issue where we should trace the context that's currently running, if any, from js_TraceRuntime. Any clues on how to find out which contexts are "running"?

(In reply to comment #39) > it should be possible to avoid the hashtable to store the objects and use less > expensive double-linked list with next/prev stored in the object itself. Although that's not a clear win, since not all of those objects hold script objects. Just starting up and shutting down there were a little less than 600 of 1800 that did, but we would add 3 pointers to all 1800.

(In reply to comment #41) > Although that's not a clear win, since not all of those objects hold script > objects. Just starting up and shutting down there were a little less than 600 > of 1800 that did, but we would add 3 pointers to all 1800. The third pointer is the tracer, right? Then the hashtable wins unless one uses singly-linked list per class since the overhead of the hashtable is on average about sizeof(ObjectHolder) * 2 or 6 words per object.

(In reply to comment #40) > I do have a question for you. If you look at attachment 285090 [details] [diff] [review] you'll see that > I've made JSContext independent objects in the cycle collector. This is because > we need to make js_TraceRuntime not trace all the global (and other?) objects > held by the contexts, but rather trace them from whatever object is holding the > context. > Is making js_TraceRuntime only trace contexts if there's no > gcExtraRootsTraceOp hook ok? All the roots is JSContext besides the global are real as they root things only when JSContext is inside a request (inside JS_BeginRequest/JS_EndRequest pair). These roots must be traced. So the problem is the global object. It should be traced only when the thing that holds JSContext is traced unless the context is in a request. A possible solution for that would be to enumerate all interesting JSContext instances, clear their global objects unless JSContext is in a request, do the tracing, and restore the globals. An alternative is to check in the tracer callback that we called from js_TraceRuntime and skip the tracing for the global object when the context is not in a request. Yet the third way is to add a request callback to SpiderMonkey that would set/clear the global and eliminate JS_SetGlobalObject calls.

(In reply to comment #43) >An alternative is to check in the tracer > callback that we called from js_TraceRuntime and skip the tracing for the > global object when the context is not in a request. That does not work since the tracer callback does not know JSContext * holding the object. A better way would be to add an option to JS_TraceRuntime to skip context tracing and then trace JSContext instances only if they are inside a request or running.

A question about HoldJSObjects: when it is possible that HoldJSObjects would be called for the same script object holder twice? Wouldn't that also require to call DropJSObjects twice and add a counter to the hashtable of object holders?

It's not an issue for all the callers in the patch. Ideally any callers just call it on themselves and they should call DropJSObjects before forgetting about any JS objects they hold.

s/any JS objects/all JS objects/

This makes XPConnect aware of most objects that hold JS objects. It removes rooting of those JS objects and instead traces them from the XPConnect tracing hook. The patch doesn't change the cycle collector algorithm itself, but is preparation for that.

(In reply to comment #46) > It's not an issue for all the callers in the patch. Ideally any callers just > call it on themselves and they should call DropJSObjects before forgetting > about any JS objects they hold. But why HoldJSObjects needs to check newHolder? I.e. when it happens that HoldJSObjects is called for something that has already held JS objects?

Because we cache the XPConnect service in nsContentUtils for as long as it holds JS object "holders" that nsContentUtils added. So we need to know the number of objects we added, not the number of times we added them.

And it's exactly because objects only remove themselves once, but can add themselves multiple times. We want to increase our count of JS holders for the first add and decrease it for the (one and only) removal.

(In reply to comment #51) > And it's exactly because objects only remove themselves once, but can add > themselves multiple times. This what I have not got. If an object wants to add itself multiple times, then it does not know if it was added or not. It means that the object does not know if it holds JS things. On the other hand the code AFAICS calls HoldJSObjects when it receives JS things and calls DropJSObjects when it clears JS things. So objects must know if they already called HoldJSObjects and can avoid the second call. What have I missed?

This API is used from objects that can hold script objects from other languages. Currently that means Python, which refcounts its objects and so needs to call HoldScriptObject for each script object that it wants to hold. For JS HoldScriptObject calls could be avoided if the object has already been added once, but I'd rather not special-case the callers of HoldScriptObject for JS.

(In reply to comment #53) > This API is used from objects that can hold script objects from other > languages. Ok, I got it. For universal scripting support HoldScriptObject takes 2 arguments, the container and the scripted thing. Thus a container with scripted things must call it for each thing it holds so, for example, Python bindings can increase the reference count for each thing. It mean that a container with multiple JS things would be added multiple times to the JS trace set and HoldJSObjects must be prepared for that. But why then DropScriptObjects does not take a scripted thing as an argument? How then Python bindings would decrease the reference counter for each thing previously referenced by the container?

(In reply to comment #54) > But why then DropScriptObjects does not take a scripted thing as an argument? > How then Python bindings would decrease the reference counter for each thing > previously referenced by the container? Because it gets the container and a tracer and it can use those to get all the script objects that the container holds and decrease the reference counter for each. See nsContentUtils::DropScriptObject.

(In reply to comment #55) > > How then Python bindings would decrease the reference counter for each thing > > previously referenced by the container? > > Because it gets the container and a tracer and it can use those to get all the > script objects that the container holds and decrease the reference counter for > each. See nsContentUtils::DropScriptObject. But why it is not possible to do the same when adding things to the container? I.e. what would prevent Python bindings to use the passed tracer to increase the reference counts for all objects?

(In reply to comment #56) > But why it is not possible to do the same when adding things to the container? > I.e. what would prevent Python bindings to use the passed tracer to increase > the reference counts for all objects? Not all objects are added at once, so you'd need to keep track which were already added before. I think I'll change HoldScriptObject to take an argument aWasHoldingObjects which it can use to avoid the call to HoldJSObjects, since all callers know whether they already called HoldScriptObject before.

(In reply to comment #57) > Not all objects are added at once, so you'd need to keep track which were > already added before. So the difference between HoldScriptObject and DropScriptObjects comes from the fact that script things can be added one by one to a container but they can only be removed from the container all at once, right? > I think I'll change HoldScriptObject to take an argument aWasHoldingObjects > which it can use to avoid the call to HoldJSObjects, since all callers know > whether they already called HoldScriptObject before. That would be nice.

(In reply to comment #58) > So the difference between HoldScriptObject and DropScriptObjects comes from the > fact that script things can be added one by one to a container but they can > only be removed from the container all at once, right? Yes (we currently don't have any callers in the tree that need to remove just one script object).

(In reply to comment #59) > Yes (we currently don't have any callers in the tree that need to remove just > one script object). Thanks for explaining this, it clarified the reasons behind HoldScriptObject and DropScriptObjects asymmetry.

Igor, feel free to review this if you have (more) time ;-).

Comment on attachment 285587 [details] [diff] [review] v2.4 (just the tracing) >Index: content/base/public/nsContentUtils.h >=================================================================== >+ static nsresult HoldScriptObject(PRUint32 aLangID, void* aObject); >+ PR_STATIC_CALLBACK(void) DropScriptObject(PRUint32 aLangID, void *aObject, >+ void *aClosure); >+ An assert in HoldScriptObject(PRUint32 aLangID, void* aObject) that aLangID != nsIProgrammingLanguage::JAVASCRIPT and similarly in DropScriptObject would clarify the code.

Comment on attachment 285587 [details] [diff] [review] v2.4 (just the tracing) Looks good, r+sr=jst. Found a couple of minor issues to look into... Nit of the day: - In nsXULElement.h: PRPackedBool mHasIdAttribute:1; PRPackedBool mHasClassAttribute:1; PRPackedBool mHasStyleAttribute:1; + PRPackedBool mHoldsScriptObject : 1; Loose the spaces around the : to match the above style. - In nsJSEnvironment.cpp, nsJSArgArray::nsJSArgArray(): - for (PRUint32 i = 0; i < argc; ++i) { - if (argv) + if (argv) { + for (PRUint32 i = 0; i < argc; ++i) Seems like the if (argv) check is a pointless check here, unless we're protecting against callers that pass in an arg count, but no arguments? Maybe not something to fix here, but probably worth an XXX comment... Similar issue in nsJSArgArray::ReleaseJSObjects() and in the trace hook too. - In nsIXPCScriptable.idl: +[ptr] native JSTracerPtr(JSTracer); I don't see this used in this interface file, is this needed?

Comment on attachment 285587 [details] [diff] [review] v2.4 (just the tracing) +void +nsAutoGCRoot::Shutdown() +{ + NS_RELEASE(sJSRuntimeService); This should be an NS_IF_RELEASE(). I applied this patch and ran with it, and on the first start I crashed here with sJSRuntimeService being null. This happened during EM restart, which I'm assuming can happen w/o any nsAutoGCRoot's being used.

This is what I landed. Contains some bustage fixes, the change from comment 64 and a change to make us refcount GCX_STRING/GCX_DOUBLE objects. Since the trace callback is used for marking we need to call it on those objects, which makes them show up in the cycle collector. If we don't refcount them they'll trigger the assertion from bug 386912, so we'll temporarily refcount them too and that might make cycle collection slightly slower. With the next stage refcounting will be removed and that overhead will disappear (they'll still show up in the cycle collector).

(In reply to comment #63) > Seems like the if (argv) check is a pointless check here, unless we're > protecting against callers that pass in an arg count, but no arguments? Maybe > not something to fix here, but probably worth an XXX comment... Callers do pass in null and an arg count, so I left the check and added a comment. Also removed the now unneeded JSAutoRequest (we're not calling into the JS engine anymore). > - In nsIXPCScriptable.idl: > > +[ptr] native JSTracerPtr(JSTracer); > > I don't see this used in this interface file, is this needed? It's used in that interface file, in the trace function in interface nsIXPCScriptable.

I've added a comment in nsXPConnect::Collect that hopefully explains how we deal with JS objects in cycle collection. Let me know if it helps or how to improve it. Most of the changes in nsCycleCollector.cpp are to be able to do cycle collection from the JS GC callback (XPCCycleCollectGCCallback in nsXPConnect), to have nodes that are not refcounted (but marked/unmarked) and to have purple nodes that are not C++. Igor: could you take a look at the change in jsgc.c and let me know if you're ok with that?

(In reply to comment #68) > Igor: could you take a look at the change in jsgc.c and let me know if you're > ok with that? the change, - if (acx->globalObject) + if (acx->globalObject && acx->requestDepth > 0) JS_CALL_OBJECT_TRACER(trc, acx->globalObject, "global object"); would introduce possible GC hazards to other embeddings like js shell. A quick way to fix is to add a new option to GC that can be set via JS_SetGCParameter and then use in the patch something like: + if (acx->globalObject && (!cx->runtime->gcIgnoreNonRequestGlobal || acx->requestDepth > 0))

I think this patch caused bug 401612.

So it looks like igor and jst already reviewed earlier versions of this patch. Which part did you want me to review? Or was that review request intended for somebody else?

Er, ok, I see now that the patch was split starting with v2.1, and v2.6 is the other half of the split from the "(just the tracing)" parts.

Given the possibility of regressions I suggest to have one patch per commit in the bug. I.e. lets create a new bug for the following patches optionally renaming this to better reflect the nature of the committed part.

I second Igor's suggestion -- whenever I've committed multiple patches over time for one bug, I've regretted it. I still have a bug on my list that suffers from that "fixed, but not fixed" status. /be

I filed bug 401687 for the next patch. I'll attach a new patch there once I figure out why the current patch added new shutdown leaks.