Closed
Bug 1083456
Opened 10 years ago
Closed 10 years ago
[jsdbg2] Debugger.Memory.prototype.takeCensus doesn't find all the roots
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jimb, Assigned: fitzgen)
References
Details
Attachments
(2 files, 8 obsolete files)
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
Debugger.Memory.prototype.takeCensus does not find objects reachable only via the stack, PersistentRooted instances, or XPCOM objects. It should use JS_TraceRuntime to gather rooted objects in the target zones, and treat those both as things to be counted, and start nodes for the traversal.
Reporter | ||
Comment 1•10 years ago
|
||
It would be nice to have a fake 'root list' JS::ubi::Node type, that could be pre-loaded with a select group of ubi::Nodes and edge names, and reports them as its edges. We could have a utility function that calls JS_TraceRoots to populate such a list, and provide an interface to manually add entries. This would mesh nicely with the BreadthFirst algorithm, which doesn't report start nodes to the handler unless they're, in fact, reachable from themselves --- another reason takeCensus is currently broken.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 2•10 years ago
|
||
Jim, I feel I have a solid understanding of how to get the roots from JS_TraceRuntime and accumulate that into a SimpleEdgeVector. Got a great little FilteringTracer<Predicate, Tracer> template going to build the debuggee zone edge filtering stuff on top of in a nice way. However, I'm getting a bogged down and lost writing the glue code to integrate this into ubi::Node. I'm feeling a bit overwhelmed by all the inheritance, composition, CRTP, traits templates, etc all coming together at once in a giant tempest of C++-y polymorphism. Originally, I was trying to just trace the edges, accumulate them in a vector and pass that to a ubi::Node/Base subclass whose edge range would just iterate over that. Doesn't work because Node shouldn't be extended, just Base, and Base subclasses can't have extra data members. Plus, lifetime of the traced edges gets really wacky since ubi::Node is supposed to be a value type. Then I start thinking: "well, what I really want at a conceptual level is a ubi::Node for the JSRuntime, so why not make a Concrete<JSRuntime> specialization?" This seems pretty good but (a) it forces us to do the edges-filtering by hand everytime we want to use the root node because we can't just use the DebuggeeZoneSetRootNode, and (b) we need to trace the runtime's edges immediately right off the bat (rather than waiting for edges() to be called) so that we get that required nursery eviction out of the way before we start using ubi::Nodes. So, now I'm thinking of creating some kind class that is just a wrapper over the root edges (which can be found by the somewhat-existing FilteringTracer I mentioned above) and then having a Concrete<> specialization for this root-edges-wrapping class. Then you would use it something like this: RootEdgesWrapper edges(getRootDebuggeeEdges(debuggees)); ubi::Node root(&edges); doTraversalsAndStuff(root); I had previously wanted a function that just returned a ubi::Node, but that seems like it doesn't jive well with cleaning up the root edges when we are done. Maybe I can just use SimpleEdgeVector as the hypothetical RootEdgesWrapper class and have a Concrete<SimpleEdgeVector> specialization? Is this making sense? Am I on the right path, or going off into the weeds?
Flags: needinfo?(jimb)
Assignee | ||
Comment 3•10 years ago
|
||
Gah, but if we pre-compute the edges, then we don't know whether to generate names for the edges! Hm...
Assignee | ||
Comment 4•10 years ago
|
||
Maybe it is OK to call JS_TraceRuntime in edges() because it will be before we get any other ubi::Nodes? But will the rooting/hazard analysis complain?
Assignee | ||
Comment 5•10 years ago
|
||
Talked to Jim on irc. Going to do something similar to the RootEdgesWrapper approach I mentioned above.
Flags: needinfo?(jimb)
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21ec39c3fa23 Couldn't think of other good ways to test this stuff. Suggestions welcome. Going to try and write one for cross compartment wrapper roots, but I don't think that should hold up initial review.
Attachment #8517603 -
Flags: review?(jimb)
Assignee | ||
Comment 7•10 years ago
|
||
For js::gc::GCRuntime::markRuntime, traceOrMark defaults to TraceRuntime which skips CCWs (and so my CCW test is failing right now). It makes sense to skip CCWs when tracing every zone, but not when we are only doing a subset. We should probably be more discriminate on how we trace the runtime (and which TraceOrMark flag we pass) based on which version of RootList::init is called.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8517603 -
Attachment is obsolete: true
Attachment #8517603 -
Flags: review?(jimb)
Attachment #8517841 -
Flags: review?(terrence)
Assignee | ||
Comment 10•10 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=06552a7b475e
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8517841 [details] [diff] [review] fictional-root-pt-1.patch Review of attachment 8517841 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testGCMarking.cpp @@ +63,5 @@ > + > + void *thing = wrapper.get(); > + CCWTestTracer trc(cx, &thing, JSTRACE_OBJECT); > + JS_TraceIncomingCCWs(&trc, zones); > + CHECK(trc.numberOfThingsTraced == 1); Ah, I need to `CHECK(trc.okay);` here. Will wait to upload a new patch until after I get the rest of your review comments.
Comment 12•10 years ago
|
||
Comment on attachment 8517841 [details] [diff] [review] fictional-root-pt-1.patch Review of attachment 8517841 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful! ::: js/public/TracingAPI.h @@ +221,5 @@ > + > +// Trace every value within |zones| that is wrapped by a cross-compartment > +// wrapper from a zone that is not an element of |zones|. > +extern JS_PUBLIC_API(void) > +JS_TraceIncomingCCWs(JSTracer *trc, JS::ZoneSet &zones); It should be fine to make |zones| const as we're only reading from it. ::: js/src/jsapi-tests/testGCMarking.cpp @@ +29,5 @@ > + okay(true), > + numberOfThingsTraced(0), > + expectedThingp(expectedThingp), > + expectedKind(expectedKind) > + { }; No ; required after method definition.
Attachment #8517841 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Thanks for the quick review, Terrence! Fixed up and carrying over r+.
Attachment #8517841 -
Attachment is obsolete: true
Attachment #8518261 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
Woops, I messed up generating that last patch...
Attachment #8518261 -
Attachment is obsolete: true
Attachment #8518266 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Adding namespace prefix so certain (non-unified?) builds don't complain. Try push for part 1 only: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43752fd111ab
Attachment #8518266 -
Attachment is obsolete: true
Attachment #8519093 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed,
leave-open
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca9d4c70a7b
Keywords: checkin-needed
Reporter | ||
Comment 18•10 years ago
|
||
Comment on attachment 8519093 [details] [diff] [review] fictional-root-pt-1.patch Review of attachment 8519093 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/UbiNode.h @@ +16,5 @@ > #include "jspubtd.h" > > #include "js/GCAPI.h" > #include "js/HashTable.h" > +#include "js/TracingAPI.h" Reviewing the wrong patch, but: Does UbiNode.h really need to include js/TracingAPI.h any more? If not, could we take that out?
Reporter | ||
Comment 19•10 years ago
|
||
Comment on attachment 8517842 [details] [diff] [review] fictional-root-pt-2.patch Review of attachment 8517842 [details] [diff] [review]: ----------------------------------------------------------------- Lurvely. A few comments; r=me with those addressed. ::: js/public/UbiNode.h @@ +476,5 @@ > +// JS::ubi::RootList rootList(cx); > +// if (!rootList.init(cx, maybeNoGC)) > +// return false; > +// > +// // The AutoCheckCannotGC is gauranteed to exist if init returned true. "guaranteed" @@ +494,5 @@ > + bool init(JSContext *cx, Maybe<AutoCheckCannotGC> &gcp); > + // Find only GC roots in the provided set of |Zone|s. > + bool init(JSContext *cx, ZoneSet &debuggees, Maybe<AutoCheckCannotGC> &gcp); > + // Find only GC roots in the given Debugger object's set of debuggee zones. > + bool init(JSContext *cx, HandleObject debuggees, Maybe<AutoCheckCannotGC> &gcp); Should RootList be MOZ_STACK_CLASS? We need gcp's lifetime to extend beyond the RootList's. For example, code like this is incorrect: RootList rootList(cx); { Maybe<JS::AutoCheckCannotGC> mcgc; ... rootList.init(cx, mcgc) ... } ... use rootList ... One way to help enforce this is to let RootList's constructor take the M<ACCGC> &, use that to initialize a private member of that same type, and then omit the M<ACCGC> & from the init methods' arguments. Then, you would be forced to have the M<ACCGC> before you declare the RootList. Also, we definitely need to explain what's going on with gcp. Perhaps something like: RootList::init itself causes a minor collection, but once the list of roots has been created, GC must not occur, as the referent ubi::Nodes are not stable across GC. The init calls emplace |gcp|'s AutoCheckCannotGC, whose lifetime must extend at least as long as the RootList itself. ::: js/src/vm/Debugger.h @@ +441,5 @@ > > bool hasMemory() const; > DebuggerMemory &memory() const; > > + const GlobalObjectSet::Range allDebuggees() const { return debuggees.all(); } I don't think a 'const' qualifier on GlobalObjectSet::Range matters here, because the result is a copy, not a reference: no harm can come from mutating it. (The 'const' for 'this', of course, is appropriate.) ::: js/src/vm/DebuggerMemory.cpp @@ +767,5 @@ > + Debugger *dbg = memory->getDebugger(); > + > + // Populate census.debuggeeZones and ensure that all of our debuggee globals > + // are rooted so that they are visible in the RootList. > + JS::AutoObjectVector debuggees(cx); The way you're using 'debuggees' here is a nice hack. I think, though, in the long term, we ought to instead let one explicitly add edges to a RootList. Then we could give it a more helpful edge name like "debuggee global", rather than "js::AutoObjectVector.vector". But that can be a follow-up. @@ +787,5 @@ > if (!traversal.init()) > return false; > traversal.wantNames = false; > > + if (!traversal.addStart(JS::ubi::Node(&rootList)) || !traversal.traverse()) Lovely. nit: Could we break the line after the ||, so the central function call of the whole function gets its own line? ::: js/src/vm/UbiNode.cpp @@ +343,5 @@ > +}; > + > +const char16_t Concrete<RootList>::concreteTypeName[] = MOZ_UTF16("RootList"); > + > +EdgeRange *Concrete<RootList>::edges(JSContext *cx, bool wantNames) const { The return type gets its own line in SpiderMonkey.
Attachment #8517842 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Carrying over r=jimb. Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1d677af61f6
Attachment #8517842 -
Attachment is obsolete: true
Attachment #8524014 -
Flags: review+
Assignee | ||
Comment 21•10 years ago
|
||
Carrying over r=jimb. Adding missing include that caused some builds to fail in the last try push. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=816bb79e8e32
Attachment #8524014 -
Attachment is obsolete: true
Attachment #8524167 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
Carrying over r+. Adding missing namespace qualification that caused some failures on some platforms in that last try push. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c60b597ae4d
Attachment #8524167 -
Attachment is obsolete: true
Attachment #8524247 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
Carrying over r=jimb. Sigh, fixing another missing namespace qualification that causes failures on certain builds/platforms on try. New try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=646ee0faee97
Attachment #8524247 -
Attachment is obsolete: true
Attachment #8524290 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open → checkin-needed
Comment 24•10 years ago
|
||
Hi Nick, seems the patch don't apply cleanly: applying fictional-root-pt-1.patch patching file js/public/TracingAPI.h Hunk #1 FAILED at 3 Hunk #2 FAILED at 206 2 out of 2 hunks FAILED -- saving rejects to file js/public/TracingAPI.h.rej patching file js/public/UbiNode.h Hunk #1 FAILED at 11 Hunk #2 FAILED at 129 2 out of 2 hunks FAILED -- saving rejects to file js/public/UbiNode.h.rej patching file js/src/gc/Tracer.cpp Hunk #1 FAILED at 12 Hunk #2 FAILED at 113 2 out of 2 hunks FAILED -- saving rejects to file js/src/gc/Tracer.cpp.rej patching file js/src/gc/Zone.h Hunk #1 FAILED at 10 Hunk #2 FAILED at 253 2 out of 2 hunks FAILED -- saving rejects to file js/src/gc/Zone.h.rej patching file js/src/jsapi-tests/moz.build Hunk #1 FAILED at 32 1 out of 1 hunks FAILED -- saving rejects to file js/src/jsapi-tests/moz.build.rej file js/src/jsapi-tests/testGCMarking.cpp already exists 1 out of 1 hunks FAILED -- saving rejects to file js/src/jsapi-tests/testGCMarking.cpp.rej patching file js/src/vm/DebuggerMemory.cpp Hunk #1 FAILED at 10 Hunk #2 FAILED at 298 2 out of 2 hunks FAILED -- saving rejects to file js/src/vm/DebuggerMemory.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh fictional-root-pt-1.patch Tomcats-MacBook-Pro:mozilla-inbound Tomcat$ could you take a look please, thanks!
Flags: needinfo?(nfitzgerald)
Keywords: checkin-needed
Assignee | ||
Comment 25•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ea3aa3ccec
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 26•10 years ago
|
||
Bad rebase. Bleck. https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5f0844f2f4
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d6178e3be2
https://hg.mozilla.org/mozilla-central/rev/84d6178e3be2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•