Once bug 805052 lands, BaseShape will have 4 bytes of padding on 32-bit to get it up to a multiple of Cell size. So moving something out of it (there was mention of metadata or compartment?) might be worth it.

bhackett, any ideas how we might do this?

Either the metadata or the compartment pointer can be removed. I think it would be better to remove the metadata pointer because it's nice to know the compartment associated with a shape and because having the metadata be part of the shape is weird and causes all sorts of problems when we reshape objects.

Luke suggested putting it in a WeakMap: seems like a good use to me.

(In reply to Terrence Cole [:terrence] from comment #4) > Luke suggested putting it in a WeakMap: seems like a good use to me. This should work for the Debugger's user of metadata (to optionally record allocation site). We won't be fragmenting shapes anymore either! Yay!

Use a weak map for object metadata. There are already two essentially identical object->object weak maps in the VM, and since doing the barriers for these maps is such a PITA I commoned all these up as a wrapper for ObjectValueMap (an object->value weak map).

Comment on attachment 8580415 [details] [diff] [review] patch Review of attachment 8580415 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! ::: js/src/vm/SavedStacks.cpp @@ +1196,3 @@ > > + if (!Debugger::onLogAllocationSite(cx, frame, PRMJ_Now())) > + CrashAtUnhandlableOOM("SavedStacksMetadataCallback"); IIUC, these actually have a chance at OOMing (esp in increasingly-tight 32-bit), especially since, when memory profiling is enabled, we'll be allocating a lot of saved stacks and other stuff. Perhaps best to keep the old fallible API?

(In reply to Luke Wagner [:luke] from comment #7) > IIUC, these actually have a chance at OOMing (esp in increasingly-tight > 32-bit), especially since, when memory profiling is enabled, we'll be > allocating a lot of saved stacks and other stuff. Perhaps best to keep the > old fallible API? The problem here is that some objects will not be correctly initialized after JSObject::create or whichever call initially creates them. If such objects aren't filled in and the GC tracer finds them (even if nothing points to the object, this will happen if the object was allocated during an incremental GC) then the object's trace hook might crash. The old style of storing the metadata didn't have this problem because the metadata was part of the shape, which was constructed before the new object was allocated. An alternative would be to just ignore failure in the metadata hook or in adding a weak map entry, but that seems worse since if we OOM we would be left with incomplete information and no knowledge that the OOM happened. And since this hook is only used for debugging, a controlled browser crash on low memory should be fine.

Yeah, I guess we can just wait and see if anyone reports problems.

Backed out for hitting frequent browser_perf-refresh.js leaks. https://hg.mozilla.org/integration/mozilla-inbound/rev/324071d6d325 Bisection: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=debug dt2&fromchange=847f9159b3e9&tochange=d3c9b899f7d2 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=7861551&repo=mozilla-inbound

In principle the weak map keeps the same amount of stuff live as the direct pointers via the shape, but in practice there are cases where things in the weak map can live longer --- some tracers, especially the cycle collector I think, mark everything in each weak map. I think this causes the windows to sometimes live long enough to be treated like a leak, even if they aren't. The attached patch goes on top of the earlier one and clears out the metadata weak map when removing the debugger for a compartment. The metadata callback is only used by the debugger, a situation which should continue in the future, and if there are multiple users of the callback then they would need to coordinate carefully anyways. This patch also has some cleanup to the metadata code, and removes the unused API that sets an object's metadata directly. This doesn't seem to be leaking in the way the earlier patch did: https://treeherder.mozilla.org/#/jobs?repo=try&revision=641bb58091b5 (This try revision is for a slightly different version of the patch that had a rooting analysis hazard)

Comment on attachment 8582460 [details] [diff] [review] clear metadata when removing debugger Review of attachment 8582460 [details] [diff] [review]: ----------------------------------------------------------------- Nice work figuring that out.