Last Comment Bug 1143256 - Consider squeezing another word out of BaseShape
: Consider squeezing another word out of BaseShape
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 All
-- normal (vote)
: mozilla39
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
: 1143890 (view as bug list)
Depends on: 1148383 1148921 1148922
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-13 18:51 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2015-03-30 07:23 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (103.73 KB, patch)
2015-03-19 17:58 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review
clear metadata when removing debugger (8.16 KB, patch)
2015-03-24 08:11 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2015-03-13 18:51:05 PDT
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 1 User image Terrence Cole [:terrence] 2015-03-16 15:02:03 PDT
*** Bug 1143890 has been marked as a duplicate of this bug. ***
Comment 2 User image Nicholas Nethercote [:njn] 2015-03-17 16:20:14 PDT
bhackett, any ideas how we might do this?
Comment 3 User image Brian Hackett (:bhackett) 2015-03-18 09:09:17 PDT
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 User image Terrence Cole [:terrence] 2015-03-18 17:07:28 PDT
Luke suggested putting it in a WeakMap: seems like a good use to me.
Comment 5 User image Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] 2015-03-18 17:16:36 PDT
(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 User image Brian Hackett (:bhackett) 2015-03-19 17:58:03 PDT
Created attachment 8580415 [details] [diff] [review]
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 User image Luke Wagner [:luke] 2015-03-19 21:38:22 PDT
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 User image Brian Hackett (:bhackett) 2015-03-20 04:16:10 PDT
(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 User image Brian Hackett (:bhackett) 2015-03-20 06:34:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c9b899f7d2
Comment 10 User image Luke Wagner [:luke] 2015-03-20 06:48:24 PDT
Yeah, I guess we can just wait and see if anyone reports problems.
Comment 11 User image Ryan VanderMeulen [:RyanVM] 2015-03-20 13:24:47 PDT
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 User image Brian Hackett (:bhackett) 2015-03-24 08:11:37 PDT
Created attachment 8582460 [details] [diff] [review]
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 User image Luke Wagner [:luke] 2015-03-24 10:01:43 PDT
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 User image Brian Hackett (:bhackett) 2015-03-25 09:08:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6465d9a4d0dc
Comment 15 User image Ryan VanderMeulen [:RyanVM] 2015-03-26 11:59:57 PDT
https://hg.mozilla.org/mozilla-central/rev/6465d9a4d0dc

Note You need to log in before you can comment on or make changes to this bug.