SpiderMonkey should support an analog to JSCompartment::objectMetadataCallback for associating metadata with JSString instances. At the moment, SpiderMonkey calls JSCompartment::objectMetadataCallback when a new object is allocated, and uses JSCompartment::objectMetadataTable to associate the new object with its metadata. We need to be able to do the same for strings; in particular, we would like to associate each string with the JavaScript code location that caused it to be allocated. This would allow Debugger.Memory.prototype.takeCensus to break down strings by allocation site.

Comment on attachment 8629094 [details] [diff] [review] Allow the use of pre-barriered GCCellPtrs. Review of attachment 8629094 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! ::: js/public/HeapAPI.h @@ +292,5 @@ > + case JS::TraceKind::String: return f(val.toString(), mozilla::Forward<Args>(args)...); > + case JS::TraceKind::Symbol: return f(val.toSymbol(), mozilla::Forward<Args>(args)...); > + case JS::TraceKind::Script: return f(val.toScript(), mozilla::Forward<Args>(args)...); > + default: > + return F::defaultValue(val); This is not the full set of GC types, so someone calling this with internal pointers is going to totally miss ObjectGroup, Shape, etc. Given that CallbackTracer can give arbitrary typed cells to gecko, this should be able to handle all the various kinds. Look at js/TraceKind.h for the full list. Currently the type forward decls for these are in TracingAPI.h. You'll want to move them to js/TraceKind.h. I think we may ultimately want to fill out GCCellPtr with these internal types, but that's going to make the interface worse for public users, so for now we should just cast to the opaque types manually from val.asCell() here. We can worry about backfilling this later if we decide it's enough use for internal consumers. Also, Null is not a valid GC pointer. The GC APIs generally want to crash aggressively if you call them with nullptr, so please just MOZ_CRASH here, as we discussed on IRC. ::: js/src/gc/Barrier.h @@ +312,5 @@ > + // SpiderMonkey's only use so far for InternalGCMethods<GCCellPtr> is > + // GCCellPtrs appearing as keys in hash tables. That case requires custom > + // post barrier code, since we may need to re-key the hash table entry; > + // these methods aren't sufficient to handle it. So we've left them > + // unimplemented for now. Great comment! ::: js/src/gc/Marking.cpp @@ +384,5 @@ > D(JS::Symbol*) \ > D(js::ObjectGroup*) \ > D(Value) \ > + D(jsid) \ > + D(JS::GCCellPtr) \o/

Revised per feedback.

I found the explanation for why js::WeakMap::markIteratively sets the key to nullptr: https://bugzilla.mozilla.org/show_bug.cgi?id=726687#c8 It prevents the destructor for Key from firing the pre-barrier on the address of a value that's been relocated, and overwritten with a forwarding pointer.

Actually, that can be side-stepped with a minor and plausible change to InternalGCMethods<GCCellPtr>::preBarrier.

Removing from Memory Tools v1 requirements -- not critical to launch

This is the type that will hold allocation metadata about strings.

Should be purely mechanical.

Also purely mechanical.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed27ebd45f23

Comment on attachment 8723951 [details] [diff] [review] Implement StringMetadataMap. Review of attachment 8723951 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsweakmap.h @@ +417,5 @@ > +// is just a thin wrapper around a WeakMap<PreBarrieredString...>, with a more > +// focused interface. > +class StringMetadataMap > +{ > + typedef WeakMap<PreBarrieredString, RelocatableValue> Map; Nit: using Map = ...;

Comment on attachment 8723955 [details] [diff] [review] Add string metadata table to JS::GC::Zone. Review of attachment 8723955 [details] [diff] [review]: ----------------------------------------------------------------- Do we have a bug open for moving the objects metadata to the Zone as well? ::: js/src/gc/RootMarking.cpp @@ +320,5 @@ > c->traceRoots(trc, traceOrMark); > > + for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) { > + zone->traceRoots(trc); > + } Nit: no {} ::: js/src/gc/Zone.cpp @@ +363,5 @@ > + return nullptr; > + } > + } > + > + if (!stringMetadataTable->add(cx, str, metadata)) Should this be a putNew() ? @@ +388,5 @@ > +void > +Zone::traceRoots(JSTracer *trc) { > + if (stringMetadataTable) { > + stringMetadataTable->trace(trc); > + } Nit: no {} ::: js/src/vm/String-inl.h @@ +98,5 @@ > +// must be careful to always use the return value instead of the pointer it > +// passed in. (Because this function is called from performance-sensitive code, > +// we don't take a HandleString; we only root if it's actually needed.) > +template<js::AllowGC allowGC, typename T> > +static MOZ_ALWAYS_INLINE T* MOZ_WARN_UNUSED_RESULT

Comment on attachment 8723955 [details] [diff] [review] Add string metadata table to JS::GC::Zone. Review of attachment 8723955 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me. ::: js/src/gc/Zone.h @@ +466,5 @@ > + private: > + // A map from pointers to JSStrings allocated in this Zone to the metadata > + // JSObjects associated with them when they were allocated, if any. This may > + // be nullptr if no JSStrings in this Zone have metadata attached. > + mozilla::UniquePtr<js::StringMetadataMap> stringMetadataTable; Make this a mozilla::PersistentRooted<mozilla::UniquePtr<js::StringMetadataMap>>> and remove the traceRoots method.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #13) > > + if (!stringMetadataTable->add(cx, str, metadata)) > > Should this be a putNew() ? No, this is a StringMetadataTable, not a HashMap. That implementation should use putNew; but I think Terrence would like it redesigned anyway. > ::: js/src/vm/String-inl.h > @@ +98,5 @@ > > +// must be careful to always use the return value instead of the pointer it > > +// passed in. (Because this function is called from performance-sensitive code, > > +// we don't take a HandleString; we only root if it's actually needed.) > > +template<js::AllowGC allowGC, typename T> > > +static MOZ_ALWAYS_INLINE T* > > MOZ_WARN_UNUSED_RESULT Good catch, thanks! Other nits fixed.

(In reply to Terrence Cole [:terrence] from comment #14) > Make this a > mozilla::PersistentRooted<mozilla::UniquePtr<js::StringMetadataMap>>> and > remove the traceRoots method. You meant, js::PersistentRooted<js::StringMetadataMap>, right? This is a little troublesome. - In order to use PersistentRooted with StringMetadataMap, Zone.h needs to be able to see the definition of StringMetadataMap, to check that it has a trace member function. A forward declaration won't do. - This in turn means that we need full definitions for StringMetadataMap's members, including WeakMap. - WeakMap, being a template, needs to write out its methods inline (we'd like to avoid having to explicitly specialize, right?). WeakMap's constructor needs a full definition for JSCompartment. - jscompartment.h is already #including Zone.h.

Comment on attachment 8723951 [details] [diff] [review] Implement StringMetadataMap. Review of attachment 8723951 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine, modulo the comments I had about ObjectMetadataMap -- specifically, these probably belong with the Metadata sub-system and not with weakmaps.

Thanks for the feedback, folks!