[jsdbg2] Expose an API to track tenuring on Debugger.Memory

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(7 attachments, 14 obsolete attachments)

4.40 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
12.12 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
16.75 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
12.17 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
10.38 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
56.64 KB, patch
Details | Diff | Splinter Review
14.04 KB, patch
jonco
: review+
Details | Diff | Splinter Review
It would be awesome if we had something like the allocations log, but for tenuring.

I think we can expose all these things:

* Allocation site / stack
* [[class]] name
* constructor display name, if the object was new'd
* timestamp of tenuring and/or minor gc number
* byte size (before and after tenuring?)

Anything else?

One potential catch is that if we didn't save a stack for the allocation of the object that is being tenured (because we weren't tracking allocations or the object wasn't sampled) then we can't ever go back and get it again. I suppose that's probably ok, since the API can just expose whatever data it has and the frontend can just encourage/force sampling allocation stacks with P=1.

Do we want to expose a log, or some kind of buckets+counts aggregation reusing the census machinery? I'd maybe lean towards a log, since you can use a log to create the latter.
Assignee: nobody → nfitzgerald
Created attachment 8625728 [details] [diff] [review]
Part 0: Document the Debugger.Memory's tenure promotions log
Attachment #8625728 - Flags: review?(sphink)
Created attachment 8625729 [details] [diff] [review]
Part 1: Debugger should maintain a set of debuggee zones and Zones should maintain a list of debuggers
Created attachment 8625730 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap
Created attachment 8625731 [details] [diff] [review]
Part 3: Expose the tenure promotions log on Debugger.Memory
Comment on attachment 8625730 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap

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

::: js/src/gc/Marking.cpp
@@ +1872,5 @@
>      overlay->forwardTo(dst);
>      insertIntoFixupList(overlay);
>  
> +    auto* dbgs = zone->getDebuggers();
> +    if (MOZ_UNLIKELY(dbgs)) {

Can we afford a branch here?
Attachment #8625730 - Flags: feedback?(terrence)
Attachment #8625730 - Flags: feedback?(sphink)
Comment on attachment 8625730 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap

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

I'm still not sure how helpful this is going to be: ~40% of our minor collections are due to full store buffer, which is not going to get you much interesting lifetime info. It's not much code though and I'm willing to be persuaded. :-)

::: js/src/gc/Marking.cpp
@@ +1872,5 @@
>      overlay->forwardTo(dst);
>      insertIntoFixupList(overlay);
>  
> +    auto* dbgs = zone->getDebuggers();
> +    if (MOZ_UNLIKELY(dbgs)) {

This should be fine -- the goal is to tenure few things. However, I'd prefer not to have debugger internals here. How about adding zone->hasDebuggers() and dispatching to zone->logPromoteToTenured(...) so that debugger details are not cluttering up our tenuring tracer?
Attachment #8625730 - Flags: feedback?(terrence) → feedback+
Running into an issue where I can't get the tenured object's metadata. My working hypothesis is that the objectMetadataTable has not had its keys updated, and so we can't find the metadata for the promoted object anymore. However, I can't figure out how/when the objectMetadataTable's keys are updated.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #7)
> Running into an issue where I can't get the tenured object's metadata. My
> working hypothesis is that the objectMetadataTable has not had its keys
> updated, and so we can't find the metadata for the promoted object anymore.
> However, I can't figure out how/when the objectMetadataTable's keys are
> updated.

GCRuntime::endMarkingZoneGroup calls
markWeakReferencesInCurrentGroup() ends up calling
GCRuntime::markWeakReferences calls
WeakMapBase::markCompartmentIteratively for each compartment, which loops through c->gcWeakMapList and calls
<actual weakmap class>::markIteratively calls
markValue on the value and checks to see if the key was moved, in which case it calls
entryMoved() calls
rekeyFront()

The key moves when the object (key) is marked, but the metadata table isn't updated until you do the iterative marking stuff detailed above.
Attachment #8625730 - Flags: feedback?(sphink) → feedback+
Comment on attachment 8625728 [details] [diff] [review]
Part 0: Document the Debugger.Memory's tenure promotions log

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

Sounds great to me! I think this is useful data, though I guess I'm not entirely sure how actionable it will be. "Arrange for the lifetimes to be shorter" might be doable; "wish that the store buffer were larger" isn't. But it'd be interesting to see.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +111,5 @@
>  
> +`trackingTenurePromotions`
> +:   A boolean value indicating whether this `Debugger.Memory` instance is
> +    observing promotions from the nursery to the tenured heap. It is an accessor
> +    property has both a getter and setter: assigning to it enables or disables

s/has/with/?
Attachment #8625728 - Flags: review?(sphink) → review+
Created attachment 8626556 [details] [diff] [review]
Part 0: Document the Debugger.Memory's tenure promotions log
Attachment #8625728 - Attachment is obsolete: true
Attachment #8626556 - Flags: review+
Created attachment 8626685 [details] [diff] [review]
Part 0: Document the Debugger.Memory's tenure promotions log
Attachment #8626556 - Attachment is obsolete: true
Attachment #8626685 - Flags: review+
Created attachment 8626688 [details] [diff] [review]
Part 1: Debugger should maintain a set of debuggee zones and Zones should maintain a list of debuggers
Attachment #8625729 - Attachment is obsolete: true
Attachment #8626688 - Flags: review?(sphink)
Created attachment 8626694 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap
Attachment #8625730 - Attachment is obsolete: true
Attachment #8626694 - Flags: review?(sphink)
Created attachment 8626695 [details] [diff] [review]
Part 3: Expose the tenure promotions log on Debugger.Memory
Attachment #8625731 - Attachment is obsolete: true
Attachment #8626695 - Flags: review?(sphink)
Created attachment 8626697 [details] [diff] [review]
Part 4: Test the tenuring log
Attachment #8626697 - Flags: review?(sphink)
Comment on attachment 8626688 [details] [diff] [review]
Part 1: Debugger should maintain a set of debuggee zones and Zones should maintain a list of debuggers

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

::: js/src/gc/Zone.h
@@ +236,2 @@
>    private:
> +    DebuggerVector* dbgs;

debuggers

::: js/src/vm/Debugger.cpp
@@ +3264,3 @@
>       */
> +
> +    bool inGlobalDbgs = false;

inGlobalDebuggers

@@ +3264,5 @@
>       */
> +
> +    bool inGlobalDbgs = false;
> +    bool inDebuggees = false;
> +    bool inZoneDbgs = false;

inZoneDebuggers, please

@@ +3265,5 @@
> +
> +    bool inGlobalDbgs = false;
> +    bool inDebuggees = false;
> +    bool inZoneDbgs = false;
> +    bool inDebuggeeZones = false;

If I'm not mistaken, this list is in a different order than your comment, which is confusing. 1=inDebuggees, 2=inGlobalDbgs. 3 and 4 look right.

@@ +3273,2 @@
>  
> +    auto* globalDbgs = GlobalObject::getOrCreateDebuggers(cx, global);

I don't like the "Dbgs" abbreviations because it is ambiguous between Debuggees and Debuggers.

@@ +3312,5 @@
> +        debuggees.remove(global);
> +    if (inZoneDbgs)
> +        zoneDbgs->popBack();
> +    if (inDebuggeeZones)
> +        debuggeeZones.remove(zone);

I'm ok with this logic setup, but it does seem kind of clunky and error-prone. Now that we have lambdas, would it be possible to do something like

template <typename Undo>
class AutoTransaction {
  bool committed;
  AutoTransaction() : committed(false) {}
  ~AutoTransaction() { if (!committed) Undo(); }
  void commit { committed = true; }
};

and then

.
.
.
if (!debuggees.put(global)) {
  ReportOutOfMemory(cx);
  return false;
}
auto undoDebuggees = [&]{ debuggees.remove(global); };
AutoTransaction<decltype(undoDebuggees)> debuggeesGuard;

...then just before the end...

globalDebuggersGuard.commit();
debuggeesGuard.commit();
zoneDebuggersGuard.commit();
debuggeeZoneGuard.commit();
return true;

Note that I have barely used lambdas, and I am far from a template master, so I don't know if that's even possible. But the code here makes me nervous. For example, you're unconditionally doing ReportOutOfMemory(cx) on error. Is that right in all cases? Or might eg Debugger::addAllocationsTracking already call that, and you'll be double-reporting or something?

Ugh. I just nerd sniped myself with this, and went ahead an implemented it. Turns out there's a standards-track proposal. See bug 1180299. I don't think you need to block this bug on that, though.

@@ +3337,5 @@
> +        if (*p == dbg)
> +            break;
> +    }
> +    MOZ_ASSERT(p != vec->end());
> +    return p;

This makes me think we ought to use <algorithm> or have mozilla::Find, but never mind...

@@ +3376,5 @@
> +    auto* gv = global->getDebuggers();
> +    Debugger** gp = findDebuggerInVector(this, gv);
> +
> +    auto *zv = global->zone()->getDebuggers();
> +    Debugger** zp = findDebuggerInVector(this, zv);

Here's a quarter. Go buy yourself some more letters.

I know they're gonna be long, but even if it's debuggerPointerInZoneVector, I'd rather have that than zp.

Though honestly, I'm not sure if the temporaries are helping much.

@@ +3381,4 @@
>  
>      /*
> +     * The relation must be removed from up to five places: *gv and
> +     * debuggees for sure, and possibly the compartment's debuggee set.

"The relation must be removed from up to five places: place 1, place 2, and possibly place 3." :-)

@@ +3391,2 @@
>       */
> +    gv->erase(gp);

You've already asserted it exists, so I'd rather read

  globalDebuggers->erase(findDebuggerInVector(this, globalDebuggers))

@@ +3397,5 @@
>  
> +    if (!recomputeDebuggeeZoneSet())
> +        CrashAtUnhandlableOOM("Debugger::removeDebuggeeGlobal");
> +    if (!debuggeeZones.has(global->zone()))
> +        zv->erase(zp);

Same here.

::: js/src/vm/Debugger.h
@@ +308,5 @@
>                                double when);
>      void emptyAllocationsLog();
>  
>      /*
> +     * Recompute the set of debuggee zones based on thes set of debuggee

s/thes/the/
Attachment #8626688 - Flags: review?(sphink) → review+
Blocks: 1180536
Created attachment 8629787 [details] [diff] [review]
Part 1: Debugger should maintain a set of debuggee zones and Zones should maintain a list of debuggers
Attachment #8626688 - Attachment is obsolete: true
Attachment #8629787 - Flags: review+
Created attachment 8629788 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap
Attachment #8626694 - Attachment is obsolete: true
Attachment #8626694 - Flags: review?(sphink)
Attachment #8629788 - Flags: review?(sphink)
Blocks: 1180763
Comment on attachment 8629788 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap

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

::: js/src/gc/Zone.h
@@ +236,5 @@
>    private:
>      DebuggerVector* debuggers;
>  
> +    using LogTenurePromotionQueue = js::Vector<JSObject*, 0, js::SystemAllocPolicy>;
> +    LogTenurePromotionQueue awaitingTenureLogging;

Vector<JSObject*> scares me. But it's enough levels of indirection away that the hazard analysis won't notice, so you're all good.

::: js/src/vm/Debugger.cpp
@@ +1714,5 @@
> +
> +    tenurePromotionsLog.insertBack(entry);
> +    if (tenurePromotionsLogLength >= maxTenurePromotionsLogLength) {
> +        js_delete(tenurePromotionsLog.getFirst());
> +        tenurePromotionsLogOverflowed = true;

This makes me wish I'd landed js::Deque when I wrote it ages ago. This linked list stuff hurts me.

Hm... can this either have a comment // dtor removes elt from list
or switch to js_delete(tenurePromotionsLog.popFirst())
unless that's considered nonidiomatic. From what I can tell, they should do the exact same amount of work in a non-debug build, just in a different order.

@@ +1781,5 @@
> +void
> +Debugger::emptyTenurePromotionsLog()
> +{
> +    while (!tenurePromotionsLog.isEmpty())
> +        js_delete(tenurePromotionsLog.getFirst());

Same comment as above. This looks even scarier!

@@ +2535,5 @@
> +     * Mark every entry in the promoted to tenured heap log.
> +     */
> +    for (TenurePromotionsEntry* e = tenurePromotionsLog.getFirst(); e; e = e->getNext()) {
> +        if (e->frame)
> +            TraceEdge(trc, &e->frame, "tenure promotions log SavedFrame");

Nothing locally depends on this, but could this assert that none of these are in the nursery?
Attachment #8629788 - Flags: review?(sphink) → review+
Comment on attachment 8626695 [details] [diff] [review]
Part 3: Expose the tenure promotions log on Debugger.Memory

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

Scary, but I couldn't find anything wrong.

::: js/src/js.msg
@@ +410,5 @@
>  MSG_DEF(JSMSG_DEBUG_VARIABLE_NOT_FOUND,0, JSEXN_TYPEERR, "variable not found in environment")
>  MSG_DEF(JSMSG_DEBUG_WRAPPER_IN_WAY,    3, JSEXN_TYPEERR, "{0} is {1}{2}a global object, but a direct reference is required")
>  MSG_DEF(JSMSG_NOT_CALLABLE_OR_UNDEFINED, 0, JSEXN_TYPEERR, "value is not a function or undefined")
>  MSG_DEF(JSMSG_NOT_TRACKING_ALLOCATIONS, 1, JSEXN_ERR, "Cannot call {0} without setting trackingAllocationSites to true")
> +MSG_DEF(JSMSG_NOT_TRACKING_TENURIZATIONS, 1, JSEXN_ERR, "Cannot call {0} without setting trackingTenurePromotions to true")

That word is a little too wacky for me. s/TENURIZATIONS/TENURINGS/ maybe? Or TENURES, but TENURINGS sounds a little better to me.

::: js/src/vm/Xdr.h
@@ +28,5 @@
>   * this wiki page:
>   *
>   *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
>   */
> +static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 294;

Did Merlin write this part of the patch? I think you're supposed to *increment* this.
Attachment #8626695 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #20)
> Comment on attachment 8626695 [details] [diff] [review]
> Part 3: Expose the tenure promotions log on Debugger.Memory
> 
> Review of attachment 8626695 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Scary, but I couldn't find anything wrong.

Er... that came out wrong. It's not scary that I couldn't find anything wrong. The patch is scary. But I couldn't find anything wrong with it. (Which doesn't mean all that much, but whatever.)
Comment on attachment 8626697 [details] [diff] [review]
Part 4: Test the tenuring log

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

Hm. Do these still work if you do eg gczeal(2,4) at the beginning? (or run with --gc-zeal=2,4)) I guess they should, because the log accumulates until you drain it?

(This would just be a manual check; I'm not proposing actually changing these tests.)
Argh. Premature review post.
Comment on attachment 8626697 [details] [diff] [review]
Part 4: Test the tenuring log

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

Nice set of tests!

::: js/src/jit-test/tests/debug/Memory-tenurePromotionsLog-05.js
@@ +7,5 @@
> +  `
> +  this.allocTemp = function allocTemp() {
> +    gc();
> +    this.dbg.memory.trackingTenurePromotions = true;
> +    var o = new Uint8Array(1);

I guess we'll know pretty quickly if we ever start nursery-allocating short Uint8Arrays.

You could make this slightly more robust by making the array bigger. 50KB, maybe?
Attachment #8626697 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #19)
> @@ +2535,5 @@
> > +     * Mark every entry in the promoted to tenured heap log.
> > +     */
> > +    for (TenurePromotionsEntry* e = tenurePromotionsLog.getFirst(); e; e = e->getNext()) {
> > +        if (e->frame)
> > +            TraceEdge(trc, &e->frame, "tenure promotions log SavedFrame");
> 
> Nothing locally depends on this, but could this assert that none of these
> are in the nursery?

We don't hold a reference to the actual thing that got tenured, this is the SavedFrame stack of where the tenured thing was allocated. It seems unlikely (impossible?) that the SavedFrame stack would not also be tenured if the thing is tenured, but it doesn't seem like something to assert either way. Were you thinking that we were holding a reference to the tenured thing and tracing that here? If that was the case, then I would agree that asserting it is not in the nursery would make sense.
(In reply to Steve Fink [:sfink, :s:] from comment #20)
> ::: js/src/vm/Xdr.h
> @@ +28,5 @@
> >   * this wiki page:
> >   *
> >   *  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode
> >   */
> > +static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 294;
> 
> Did Merlin write this part of the patch? I think you're supposed to
> *increment* this.

Rebasing + subtrahends = :-/
(In reply to Steve Fink [:sfink, :s:] from comment #22)
> Comment on attachment 8626697 [details] [diff] [review]
> Part 4: Test the tenuring log
> 
> Review of attachment 8626697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm. Do these still work if you do eg gczeal(2,4) at the beginning? (or run
> with --gc-zeal=2,4)) I guess they should, because the log accumulates until
> you drain it?
> 
> (This would just be a manual check; I'm not proposing actually changing
> these tests.)

Tested manually, all pass.
Created attachment 8630521 [details] [diff] [review]
Part 0: Document the Debugger.Memory's tenure promotions log
Attachment #8626685 - Attachment is obsolete: true
Attachment #8630521 - Flags: review+
Created attachment 8630522 [details] [diff] [review]
Part 1: Debugger should maintain a set of debuggee zones and Zones should maintain a list of debuggers
Attachment #8629787 - Attachment is obsolete: true
Attachment #8630522 - Flags: review+
Created attachment 8630523 [details] [diff] [review]
Part 2: Add Debugger infrastructure for logging promotions to the tenured heap
Attachment #8629788 - Attachment is obsolete: true
Attachment #8630523 - Flags: review+
Created attachment 8630524 [details] [diff] [review]
Part 3: Expose the tenure promotions log on Debugger.Memory
Attachment #8626695 - Attachment is obsolete: true
Attachment #8630524 - Flags: review+
Created attachment 8630525 [details] [diff] [review]
Part 4: Test the tenuring log
Attachment #8626697 - Attachment is obsolete: true
Attachment #8630525 - Flags: review+
Sigh. Messed up the rebase and lost a FMT in the minor gc fprintf so that try push is pretty bad. Here's the fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd569092022
The failures I'm seeing on SM(cgc) are the result of the tenured object's SavedFrame object representing its allocation site being within a relocated arena after a compacting GC.

We trace the owning Debugger and the log entries and their SavedFrame objects once before sweeping (like usual) and then we figure out where to relocate everything and do a second trace with the compacting GC's moving tracer. For some reason by the time we do this second trace, the SavedFrame object that is inside an arena that will be relocated is not marked as being forwarded to anywhere (at least according to `js::gc::IsForwarded(T*)` and `js::gc::RelocationOverlay::isForwarded()`).

So it seems like the bug is within the process of figuring out what and where to relocate, but I haven't dug in to that very deep yet.
Ok, so after some gdb deep diving, what I'm seeing is:

1. Debugger::trace which also does TraceEdge(myThing)
2. Relocate a bunch of arenas that do not contain myThing
3. Debugger::trace which also does TraceEdge(myThing)
4. Relocate a few more arenas and do RelocateCell(myThing) and all that 
   overlay->forwardTo goodness w/ magic_ set to Relocated and newLocation is set as
   well.
5. KERN_PROTECTION_FAILURE at address: <old addr of myThing before forwarding> when
   draining the tenure log

Notably, I would expect there to be a Debugger::trace between (4) and (5). Not sure
if there is a whole Debugger::trace call missing or if (3) is supposed to come after
(4). Either way, seems like I'm getting warmer...
Created attachment 8631154 [details] [diff] [review]
rollup.patch

This is a rollup of parts 0 - 5. Applies on the following git commit:

commit 67ce0a43c0a107d4e1fcad679262685f262e4ba6
Merge: 24d2c94 5d6e37b
Author: Ryan VanderMeulen <ryanvm@gmail.com>
Date:   Tue Jul 7 13:25:39 2015 -0400

    Merge b2g-inbound to m-c. a=merge

STR for hitting the funky behavior described in previous comment is:

  $ JS_GC_ZEAL=14 ./js/src/jit-test/jit_test.py $OBJDIR/dist/bin/js debug/Memory-tenurePromotionsLog-01.js -o
Flags: needinfo?(sphink)
Created attachment 8631250 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log
Attachment #8631250 - Flags: review?(sphink)
Attachment #8631250 - Flags: review?(sphink) → review+
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #25)
> (In reply to Steve Fink [:sfink, :s:] from comment #19)
> > @@ +2535,5 @@
> > > +     * Mark every entry in the promoted to tenured heap log.
> > > +     */
> > > +    for (TenurePromotionsEntry* e = tenurePromotionsLog.getFirst(); e; e = e->getNext()) {
> > > +        if (e->frame)
> > > +            TraceEdge(trc, &e->frame, "tenure promotions log SavedFrame");
> > 
> > Nothing locally depends on this, but could this assert that none of these
> > are in the nursery?
> 
> We don't hold a reference to the actual thing that got tenured, this is the
> SavedFrame stack of where the tenured thing was allocated. It seems unlikely
> (impossible?) that the SavedFrame stack would not also be tenured if the
> thing is tenured, but it doesn't seem like something to assert either way.
> Were you thinking that we were holding a reference to the tenured thing and
> tracing that here? If that was the case, then I would agree that asserting
> it is not in the nursery would make sense.

Oh! Yes, sorry, I didn't notice it wasn't the original object. And it's good that you're not holding onto the original object, since that would keep a lot of stuff alive for a potentially long time.

Please ignore me. Carry on.
Comment on attachment 8631250 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log

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

Er, wait. Sorry to revoke that r+, but I realized that we still didn't handle the zone graph.

Everything else in Debugger::markCrossCompartmentEdges is properly accounted for in the zone graph because they're all ObjectWeakMaps, which have ObjectValueMap::findZoneEdges to make sure the pointed-to stuff doesn't get marked and swept into oblivion while a delegate is still alive and pointing. Sadly, I think you'll need to do something similar here: arrange for debuggeeZone->gcZoneGroupEdges.put(debuggerZone) to be called as part of GCRuntime::findZoneGroups. (I *think* I got that the right way around.)

ni? jonco here because I think he's more familiar with this stuff and might know a simpler way.
Attachment #8631250 - Flags: review+
Flags: needinfo?(jcoppeard)
sfink is correct.  The correct way round is debuggee -> debugger, the important thing being that it's the opposite way from that in JSCompartment::findOutgoingEdges() (and the same as that in Debugger::findCompartmentEdges()), which ensures that debuggers and their debuggees are swept in the same zone group.

Does the set of debuggee zones for a debugger cover all the zones that contain keys in the debugger's weak maps (i.e. Debugger::scripts, source, objects and environments)?

If so I think we can remove the zone tracking infrastructure in DebuggerWeakMap and the current conents of Debugger::findCompartmentEdges(), which would be a great simplification.
Flags: needinfo?(jcoppeard)
Blocks: 1182157
(In reply to Jon Coppeard (:jonco) from comment #42)
> Does the set of debuggee zones for a debugger cover all the zones that
> contain keys in the debugger's weak maps (i.e. Debugger::scripts, source,
> objects and environments)?
> 
> If so I think we can remove the zone tracking infrastructure in
> DebuggerWeakMap and the current conents of Debugger::findCompartmentEdges(),
> which would be a great simplification.

I *think* so, but would like to put it off to a follow up: bug 1182157
Created attachment 8631745 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log
Attachment #8631250 - Attachment is obsolete: true
Attachment #8631745 - Flags: review?(sphink)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #44)
> Created attachment 8631745 [details] [diff] [review]
> Part 5: Fix a GC hazard and properly mark cross compartment edges in the
> tenure promotions log

Difference from last version: added the following to Zone::findOutgoingEdges:

+    if (hasDebuggers()) {
+        for (auto range = getDebuggers()->all(); !range.empty(); range.popFront())
+            finder.addEdgeTo(range.front()->toJSObject()->zone());
+    }
Comment on attachment 8631745 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log

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

jonco, really just requesting review for the jsgc.cpp changes, though of course you're free to look at anything or everything else!

::: js/src/jsgc.cpp
@@ +4399,5 @@
>      gcZoneGroupEdges.clear();
> +
> +    if (hasDebuggers()) {
> +        for (auto range = getDebuggers()->all(); !range.empty(); range.popFront())
> +            finder.addEdgeTo(range.front()->toJSObject()->zone());

Ok, you're gonna hate me. I'm going to punt this over to jonco after all, because I'm not sure about the answer to 2 questions:

1. Should this edge be added if the debugger is already marked black? (Probably just an optimization to avoid it, and a meaningless one, since debuggees and debuggers always get lumped into one component anyway. But maybe looking up edges is slow or something?)

but more importantly,

2. Does this need an ->isGCMarking() guard on it? I think it does, since otherwise you'd never be able to GC the debuggee zone separately from the debugger (and all of its other debuggees). I think the intent, at least, is to treat all edges going from the Debugger to the Debuggee as live, but still allow GC of everything not reachable from those or any other root. And everything else seems to have that check. (I don't even know what would happen if you had a zone edge to a zone that isn't being marked.)
Attachment #8631745 - Flags: review?(sphink) → review?(jcoppeard)
Comment on attachment 8631745 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log

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

::: js/src/jsgc.cpp
@@ +4399,5 @@
>      gcZoneGroupEdges.clear();
> +
> +    if (hasDebuggers()) {
> +        for (auto range = getDebuggers()->all(); !range.empty(); range.popFront())
> +            finder.addEdgeTo(range.front()->toJSObject()->zone());

In answer to the questions above:

1. Yes edges should be marked regardless of whether the debugger is marked.

2. Yes, we only want to add the edge if the debugger zone isGCMarking().

Also, please move this code to Debugger::findCompartmentEdges().

If you like, you could also move the call to Debugger::findCompartmentEdges() from JSCompartment::findOutgoingEdges() to Zone::findOutgoingEdges() and rename it to findZoneEdges().  We're needlessly calling this for every compartment at the moment.

This looks good though.  Did you do a try run yet?
Attachment #8631745 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #47)

Thanks

> This looks good though.  Did you do a try run yet?

Not with the zone graph related changes yet. Will do one once I've addressed your comments.
Created attachment 8632127 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log
Attachment #8631745 - Attachment is obsolete: true
Attachment #8632127 - Flags: review?(jcoppeard)
Here is an interdiff from the last version of the patch: https://pastebin.mozilla.org/8839176
Comment on attachment 8632127 [details] [diff] [review]
Part 5: Fix a GC hazard and properly mark cross compartment edges in the tenure promotions log

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

Great, r=me for the GC bits.  Thanks for fixing the existing issues too.
Attachment #8632127 - Flags: review?(jcoppeard) → review+
Blocks: 1182699
Depends on: 1184389
You need to log in before you can comment on or make changes to this bug.