nsXPConnect::Traverse is still missing some JS edges, using tracing would probably make it find those.

Should probably share the code between NoteContextChild and NoteJSChild. This also still traverses atoms directly instead of treating them as separate objects.

Comment on attachment 261948 [details] [diff] [review] Wip >Index: dom/src/base/nsJSEnvironment.cpp >=================================================================== >+void >+NoteContextChild(JSTracer *trc, void *thing, uint32 kind) >+{ >+ if (kind == JSTRACE_ATOM) { >+ JSAtom *atom = (JSAtom *)thing; >+ jsval v = ATOM_KEY(atom); >+ if (!JSVAL_IS_PRIMITIVE(v)) { >+ thing = JSVAL_TO_GCTHING(v); >+ kind = JSTRACE_OBJECT; >+ } >+ } >+ >+ if (kind == JSTRACE_OBJECT || kind == JSTRACE_NAMESPACE || >+ kind == JSTRACE_QNAME || kind == JSTRACE_XML) { >+ ContextCallbackItem *item = NS_STATIC_CAST(ContextCallbackItem*, trc); >+ item->cb->NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, thing); >+ } >+} You do not need to deal with atoms explicitly here, a better way to express this would be: >+void >+NoteContextChild(JSTracer *trc, void *thing, uint32 kind) >+{ >+ if (kind == JSTRACE_OBJECT || kind == JSTRACE_XML) { >+ ContextCallbackItem *item = NS_STATIC_CAST(ContextCallbackItem*, trc); >+ item->cb->NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, thing); >+ } else { >+ JS_TraceChildren(trc, thing, kind); >+ } >+} This hardcode the fact that JSXMLQName and JSXMNamespace contains only JSString and JSObject and as such can not form loops itself. Thus we can safely recurse into them excluding them from ref counting. On the other hand JSXML must be refcounted not only because it can form pure XML loops but also because XML tree can have arbitrary depth so we must protect from stack overflow. I aslo think that an API should be provided to avoid that explicit (kind == JSTRACE_OBJECT || kind == JSTRACE_XML) check. >+void >+NoteJSChild(JSTracer *trc, void *thing, uint32 kind) >+{ >+ if(kind == JSTRACE_ATOM) >+ { >+ JSAtom *atom = (JSAtom *)thing; >+ jsval v = ATOM_KEY(atom); >+ if(!JSVAL_IS_PRIMITIVE(v)) >+ { >+ thing = JSVAL_TO_GCTHING(v); >+ kind = JSTRACE_OBJECT; >+ } >+ } >+ >+ if(kind == JSTRACE_OBJECT || kind == JSTRACE_NAMESPACE || >+ kind == JSTRACE_QNAME || kind == JSTRACE_XML) >+ { >+ ContextCallbackItem *item = NS_STATIC_CAST(ContextCallbackItem*, trc); >+ item->cb->NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, thing); > } > } The same as above. > >-nsresult >+static uint8 GCTypeToTraceKindMap[GCX_NTYPES] = { >+ JSTRACE_OBJECT, /* GCX_OBJECT */ >+ JSTRACE_STRING, /* GCX_STRING */ >+ JSTRACE_DOUBLE, /* GCX_DOUBLE */ >+ JSTRACE_STRING, /* GCX_MUTABLE_STRING */ >+ JSTRACE_FUNCTION, /* GCX_PRIVATE */ >+ JSTRACE_NAMESPACE, /* GCX_NAMESPACE */ >+ JSTRACE_QNAME, /* GCX_QNAME */ >+ JSTRACE_XML, /* GCX_XML */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 0 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 1 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 2 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 3 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 4 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 5 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 6 */ >+ JSTRACE_STRING, /* GCX_EXTERNAL_STRING + 7 */ >+}; Notice that it is necessary to descend only into 2 things, JSTRACE_OBJECT and JSTRACE_XML. Since each things is at least 2-word aligned, the pointer to it contains effectively 3 spare bits to use. We can use one to distinguish between object/xml case and use thing | typebit as a key for hashtable. Alternatively a separated hashtable can be used for JSXML. This allows to avoid any call to js_GetGCThingFlags.

Fixing bug 340212 is required before we can trust that the tracing enumerate all the things outside GC marking phase. That also pointed a problem for the proper accounting of JSContext edges. The autoroots stored in XP Connect thread data are in fact roots referenced by JSContext based on their usage. Yet even with the patch for bug 340212 landed they would be accounted as roots stored in JSRuntime*. Probably it would be necessary either to switch the autoroots to use JSTempValueRooter (which is properly accounted as an edge from JSContext) or add yet another callback.

(In reply to comment #3) > You do not need to deal with atoms explicitly here, a better way to express > this would be: > > >+void > >+NoteContextChild(JSTracer *trc, void *thing, uint32 kind) > >+{ > >+ if (kind == JSTRACE_OBJECT || kind == JSTRACE_XML) { > >+ ContextCallbackItem *item = NS_STATIC_CAST(ContextCallbackItem*, trc); > >+ item->cb->NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, thing); > >+ } else { > >+ JS_TraceChildren(trc, thing, kind); > >+ } > >+} > > This hardcode the fact that JSXMLQName and JSXMNamespace contains only JSString > and JSObject and as such can not form loops itself. Thus we can safely recurse > into them excluding them from ref counting. On the other hand JSXML must be > refcounted not only because it can form pure XML loops but also because XML > tree can have arbitrary depth so we must protect from stack overflow. I'm not sure I understand this code. If two things hold on to a JSXMLQName and the JSXMLQName holds on to a JSObject, the JSXMLQName will be traversed twice, so JS_TraceChildren will be called twice on the JSObject even though it only has a refcount of 1 (gcThingCallback will only have been called). What am I missing?

(In reply to comment #5) > If two things hold on to a JSXMLQName and > the JSXMLQName holds on to a JSObject, the JSXMLQName will be traversed twice, > so JS_TraceChildren will be called twice on the JSObject even though it only > has a refcount of 1 (gcThingCallback will only have been called). The idea is that one can exclude JSXMLQName from the graph of things that the cycle collector builds and replace any reference to JSXMLQName by references to things JSXMLQname contains. This works since JSXMLQName contains only finite number of references to different things (in fact JSXMLQName refers to exactly one JSObject) and those references are not JSXMLQname instances so JSXMLQname can not form a cycle with itself. In the example above such removal of JSXMLQname builds a graph where the nodes that originally pointed to JSXMLQname point instead to JSObject that JSXMLQname refers. The same reasoning is applicable to JSXMLObjec and JSAtom so theyr can also be excluded. On the other hand JSXML can not be excluded since it can contain unbounded number of children and can form cycles with itself.

(In reply to comment #6) > In the example above such removal of JSXMLQname builds a graph where the nodes > that originally pointed to JSXMLQname point instead to JSObject that JSXMLQname > refers. But then the gcThingCallback needs to transparently traverse JSXMLQname too. Traversal and the callback need to be symmetric.

(In reply to comment #7) > (In reply to comment #6) > > In the example above such removal of JSXMLQname builds a graph where the nodes > > that originally pointed to JSXMLQname point instead to JSObject that JSXMLQname > > refers. > > But then the gcThingCallback needs to transparently traverse JSXMLQname too. > Traversal and the callback need to be symmetric. The idea is to get rid of gcThingCallback at all and use tracing exclusively both to build refcounts and to to discover the edges.

So if I understand correctly XPCOM objects would use the API from bug 379220 to keep JS objects alive. Instead of running the JS GC and use gcThingCallback, we'd call JS_TraceRuntime to get refcounts for JS objects and we'd use tracing to reconstruct the graph (like in attachment 261948 [details] [diff] [review]). If that's correct, then we would duplicate a bunch of work between the JS GC (which we'll still need to run) and the refcounting trace?

(In reply to comment #9) > So if I understand correctly XPCOM objects would use the API from bug 379220 to > keep JS objects alive. Instead of running the JS GC and use gcThingCallback, > we'd call JS_TraceRuntime to get refcounts for JS objects and we'd use tracing > to reconstruct the graph (like in attachment 261948 [details] [diff] [review]). That is exactly the idea. > If that's correct, then > we would duplicate a bunch of work between the JS GC (which we'll still need to > run) and the refcounting trace? This would not duplicate the work and add only small overhead. The current code with thingCallback callback activated is doing: for each reachable thing in the graph loop mark thing calculateRefCounts end loop After the change the code would do: for each reachable thing in the graph loop calculateRefCounts end loop and later during GC: for each reachable thing in the graph loop mark thing end loop So the extra overhead can come only from the enumerating code in the loop or, in other words, from the pure graph tracing. This is cheap compared with refcount updates since the latter require to query/push each node into a hashtable. Moreover, this overhead can be eliminated completely if we would allow for a special tracer to take over GC marking so GC in this case would rely on the tracer to set the marking bits. It can in fact be faster than the current code since the tracer, for example, would not need to call GetGCThingFlags on nodes it already have in the cache.

(In reply to comment #10) > (In reply to comment #9) > > So if I understand correctly XPCOM objects would use the API from bug 379220 to > > keep JS objects alive. Note that bug 379220 is just an optimization of root storage and is independent from this bug. One should be able to use tracing to discover the edges and reconstruct ref counts with or without that optimization.

(In reply to comment #11) > Note that bug 379220 is just an optimization of root storage and is independent > from this bug. One should be able to use tracing to discover the edges and > reconstruct ref counts with or without that optimization. Actually it is not just an optimization since the current tracing code calls the tracer only once per locked thing. Now I understand why it is wrong as the lock count is used as a storage for reference count from xpcom objects to JSObject and repeated gcThingCallback calls transfers that knowledge into the ref counting hashtable. Now the bug 379220 removes LockGCThing calls effectively producing a workaround for the problem above but the right fix is to change the tracing also to call the trace function repeatedly.

(In reply to comment #10) > So the extra overhead can come only from the enumerating code in the loop or, > in other words, from the pure graph tracing. This is cheap compared with > refcount updates I'm not convinced that it is a cost we want to pay. So either we make the refcounting code do GC marking or we need to deal with the JSTRACE_NAMESPACE and JSTRACE_QNAME types like I do in attachment 261948 [details] [diff] [review]. Or is there only ever one edge into each namespace and QName object? I'd like to fix this bug (and bug 375063) quite soon though. (In reply to comment #12) > Now the bug 379220 removes LockGCThing calls effectively producing a workaround > for the problem above but the right fix is to change the tracing also to call > the trace function repeatedly. I'm fine with doing bug 379220 for XPConnect (replacing the JS_LockGCThing calls). There are other callers of JS_LockGCThing though and we'll still need to support those, I think, so calling the trace function repeatedly seems like the right thing in that case.

(In reply to comment #13) > (In reply to comment #10) > > So the extra overhead can come only from the enumerating code in the loop or, > > in other words, from the pure graph tracing. This is cheap compared with > > refcount updates > > I'm not convinced that it is a cost we want to pay. Note that the cost can be negative when the cycle collector finds and breaks cycles that keep JS things from GC. In this case the following GC would avoid tracing that part of the graph. Taking into account the fact that xpconnect does not need to trace strings such savings can offset the cost of separated xpc trace phase. So I suggest to try and measure.

Igor: right now we're mostly concerned about fixing the remaining leaks (like bug 375063). A number of comments you made about the current patch seem to be valid when we start using tracing for the refcounting, but don't matter with the current refcounting code. Regarding the atoms, I think I need to traverse them transparently, because that is how the gcThingCallback works too. JSTRACE_NAMESPACE and JSTRACE_QNAME need to be handled as separate objects (not traversed transparently), because they are refcounted separately too. Make sense? Could you have a quick look at the patch and let me know what issues you still see with it if we do not change refcounting?

(In reply to comment #15) > Could you have a quick look at the patch and let me know what issues you still > see with it if we do not change refcounting? From a quick look now and more longer one few weeks ago I do not see any particular problems with that patch. What I see as a problem is the fact that various auto roots in xpconnect are per thread, not per per JSContext or XPCContext. So the cycle collector would see them as edges from runtime, not context. But I am not sure if it matters in practice.

(In reply to comment #16) > What I see as a problem is the fact that various auto roots in xpconnect are > per thread, not per per JSContext or XPCContext. So the cycle collector would > see them as edges from runtime, not context. I meant that this still would a problem even with the patch applied.

I get a linker error with this patch, linking libxpconnect.so: nsXPConnect.o: In function `nsXPConnect::Traverse(void*, nsCycleCollectionTraversalCallback&)': /builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:685: undefined reference to `js_GetGCThingFlags'

Comment on attachment 264519 [details] [diff] [review] v1 r=jst. Might be good if Igor could give this one a once-over as well.

Comment on attachment 264519 [details] [diff] [review] v1 >Index: js/src/xpconnect/src/nsXPConnect.cpp >=================================================================== ... >-nsresult >+static uint8 GCTypeToTraceKindMap[GCX_NTYPES] = { >+ JSTRACE_OBJECT, /* GCX_OBJECT */ >+ JSTRACE_STRING, /* GCX_STRING (unused) */ >+ JSTRACE_DOUBLE, /* GCX_DOUBLE (unused) */ >+ JSTRACE_STRING, /* GCX_MUTABLE_STRING (unused) */ >+ JSTRACE_FUNCTION, /* GCX_PRIVATE (unused) */ >+ JSTRACE_NAMESPACE, /* GCX_NAMESPACE */ >+ JSTRACE_QNAME, /* GCX_QNAME */ >+ JSTRACE_XML /* GCX_XML */ >+ // We don't care about JSTRACE_STRING, so stop here >+}; ... >- JSObject *obj = NS_STATIC_CAST(JSObject*, p); >- JSClass* clazz = OBJ_GET_CLASS(cx, obj); >+ uint8 ty = *js_GetGCThingFlags(p) & GCF_TYPEMASK; Since the patch starts to use js_GetGCThingFlags, its declaration/definition in jsgc.[hc] should be altered from extern uint8 * js_GetGCThingFlags(void *thing); to extern JS_FRIEND_API(uint8 *) js_GetGCThingFlags(void *thing); and similarly in jsgc.c for the definition to address the linkage error from comment 20. ... >+ JS_TRACER_INIT(&trc, cx, NoteJSChild); >+ JS_TraceChildren(&trc, p, GCTypeToTraceKindMap[ty]); Use here js_CallValueTracerIfGCThing(&trc, (jsval)p) and remove GCTypeToTraceKindMap. This requires JS_FRIEND_API to be applied to js_CallValueTracerIfGCThing in jsgc.[hc] as well. r=+ with this changes.

(In reply to comment #22) > >+ JS_TRACER_INIT(&trc, cx, NoteJSChild); > >+ JS_TraceChildren(&trc, p, GCTypeToTraceKindMap[ty]); > > Use here js_CallValueTracerIfGCThing(&trc, (jsval)p) and remove > GCTypeToTraceKindMap. I don't think that's the same, js_CallValueTracerIfGCThing calls the tracer for p, we want to call the tracer for children of p.

(In reply to comment #23) > > Use here js_CallValueTracerIfGCThing(&trc, (jsval)p) and remove > > GCTypeToTraceKindMap. > > I don't think that's the same, js_CallValueTracerIfGCThing calls the tracer for > p, we want to call the tracer for children of p. Yes, I was wrong about it. So the patch just needs to add FRIEND to the js_GetGCThingFlags.

Comment on attachment 265953 [details] [diff] [review] v1 sr=dbaron

Checked in. Rlk didn't change, Lk dropped from about 1.93MB to 1.81MB.