1143256 - Consider squeezing another word out of BaseShape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Nicholas Nethercote [inactive]]

bhackett, any ideas how we might do this?

Comment 3

[Brian Hackett [Laid off!]]

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.

Comment 4

[Terrence Cole [:terrence]]

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

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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!

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

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 7

[Luke Wagner [:luke]]

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?

Comment 8

[Brian Hackett [Laid off!]]

(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.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c9b899f7d2

Comment 10

[Luke Wagner [:luke]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Brian Hackett [Laid off!]]

Attachment: clear metadata when removing debugger

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 13

[Luke Wagner [:luke]]

Comment on attachment 8582460 [details] [diff] [review]
clear metadata when removing debugger

Review of attachment 8582460 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work figuring that out.

Comment 14

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6465d9a4d0dc

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6465d9a4d0dc