Closed Bug 1137844 Opened 5 years ago Closed 5 years ago

[jsdbg2] Add a Debugger.Memory.prototype.onGarbageCollection hook

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(5 files, 21 obsolete files)

8.52 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.87 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
4.33 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
27.53 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
5.55 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
It should also probably use the same timestamp format that the allocations log uses, for sanity.

We probably shouldn't expose some JSON string either, but a normal object or something.
This is what is currently generated by the GC stats API:

console.log: CC: {
  "timestamp": 1425072846618091,
  "duration": 181,
  "max_slice_pause": 5,
  "total_slice_pause": 19,
  "max_finish_gc_duration": 0,
  "max_sync_skippable_duration": 0,
  "suspected": 13656,
  "visited": {
    "RCed": 13630,
    "GCed": 7521
  },
  "collected": {
    "RCed": 7652,
    "GCed": 2480
  },
  "waiting_for_gc": 2480,
  "zones_waiting_for_gc": 0,
  "short_living_objects_waiting_for_gc": 171,
  "forced_gc": 0,
  "forget_skippable": {
    "times_before_cc": 23,
    "min": 1,
    "max": 6,
    "avg": 2,
    "total": 54,
    "removed": 57915
  }
}

console.log: GC: {
  "timestamp": 1425072847575531,
  "max_pause": 41.2,
  "total_time": 61.6,
  "zones_collected": 1,
  "total_zones": 52,
  "total_compartments": 429,
  "minor_gcs": 28,
  "store_buffer_overflows": 25,
  "mmu_20ms": 0,
  "mmu_50ms": 17,
  "scc_sweep_total": 3.8,
  "scc_sweep_max_pause": 3.8,
  "nonincremental_reason": "none",
  "allocated": 58,
  "added_chunks": 11,
  "removed_chunks": 16,
  "slices": [
    {
      "slice": 0,
      "pause": 20.4,
      "when": 0,
      "reason": "EAGER_ALLOC_TRIGGER",
      "page_faults": 0,
      "start_timestamp": 1425072847385700,
      "end_timestamp": 1425072847406171,
      "times": {
        "mutator_running": 0,
        "begin_callback": 0,
        "wait_background_thread": 0,
        "mark_discard_code": 1.5,
        "purge": 0,
        "mark": 17.1,
        "mark_roots": 0,
        "mark_cross_compartment_wrappers": 0,
        "mark_rooters": 0,
        "mark_runtimeremoved_wide_data": 0,
        "mark_embedding": 0,
        "mark_compartments": 0,
        "unmark": 1,
        "mark_delayed": 0,
        "sweep": 0,
        "mark_during_sweeping": 0,
        "mark_types_during_sweeping": 0,
        "mark_incoming_black_pointers": 0,
        "mark_weak": 0,
        "mark_incoming_gray_pointers": 0,
        "mark_gray": 0,
        "mark_gray_and_weak": 0,
        "finalize_start_callback": 0,
        "sweep_atoms": 0,
        "sweep_symbol_registry": 0,
        "sweep_compartments": 0,
        "sweep_discard_code": 0,
        "sweep_inner_views": 0,
        "sweep_cross_compartment_wrappers": 0,
        "sweep_base_shapes": 0,
        "sweep_initial_shapes": 0,
        "sweep_type_objects": 0,
        "sweep_breakpoints": 0,
        "sweep_regexps": 0,
        "sweep_miscellaneous": 0,
        "sweep_type_information": 0,
        "sweep_type_tables_and_compilations": 0,
        "free_type_arena": 0,
        "sweep_object": 0,
        "sweep_string": 0,
        "sweep_script": 0,
        "sweep_shape": 0,
        "sweep_jit_code": 0,
        "finalize_end_callback": 0,
        "deallocate": 0,
        "compact": 0,
        "compact_move": 0,
        "compact_update": 0,
        "compact_update_cells": 0,
        "end_callback": 0,
        "all_minor_gcs": 0,
        "minor_gcs_to_evict_nursery": 1.1,
        "trace_heap": 0
      }
    },
    {
      "slice": 1,
      "pause": 41.2,
      "when": 148.5,
      "reason": "API",
      "page_faults": 0,
      "start_timestamp": 1425072847534299,
      "end_timestamp": 1425072847575510,
      "times": {
        "mutator_running": 0,
        "begin_callback": 0,
        "wait_background_thread": 0,
        "mark_discard_code": 0,
        "purge": 0,
        "mark": 28,
        "mark_roots": 0,
        "mark_cross_compartment_wrappers": 0,
        "mark_rooters": 0,
        "mark_runtimeremoved_wide_data": 0,
        "mark_embedding": 0,
        "mark_compartments": 0,
        "unmark": 0,
        "mark_delayed": 0,
        "sweep": 11.4,
        "mark_during_sweeping": 2.5,
        "mark_types_during_sweeping": 0,
        "mark_incoming_black_pointers": 0,
        "mark_weak": 0.3,
        "mark_incoming_gray_pointers": 0,
        "mark_gray": 2,
        "mark_gray_and_weak": 0,
        "finalize_start_callback": 0.1,
        "sweep_atoms": 0,
        "sweep_symbol_registry": 0,
        "sweep_compartments": 4.7,
        "sweep_discard_code": 0.6,
        "sweep_inner_views": 0,
        "sweep_cross_compartment_wrappers": 2.6,
        "sweep_base_shapes": 0.4,
        "sweep_initial_shapes": 0.6,
        "sweep_type_objects": 0.6,
        "sweep_breakpoints": 0.1,
        "sweep_regexps": 0,
        "sweep_miscellaneous": 0.8,
        "sweep_type_information": 2,
        "sweep_type_tables_and_compilations": 0,
        "free_type_arena": 0,
        "sweep_object": 1,
        "sweep_string": 0,
        "sweep_script": 0.3,
        "sweep_shape": 0.9,
        "sweep_jit_code": 0.3,
        "finalize_end_callback": 0.6,
        "deallocate": 0,
        "compact": 0,
        "compact_move": 0,
        "compact_update": 0,
        "compact_update_cells": 0,
        "end_callback": 0.1,
        "all_minor_gcs": 0,
        "minor_gcs_to_evict_nursery": 1.1,
        "trace_heap": 0
      }
    }
  ],
  "totals": {
    "mutator_running": 0,
    "begin_callback": 0,
    "wait_background_thread": 0,
    "mark_discard_code": 0,
    "purge": 0,
    "mark": 0,
    "mark_roots": 0,
    "mark_cross_compartment_wrappers": 0,
    "mark_rooters": 0,
    "mark_runtimeremoved_wide_data": 0,
    "mark_embedding": 0,
    "mark_compartments": 0,
    "unmark": 0,
    "mark_delayed": 0,
    "sweep": 0,
    "mark_during_sweeping": 0,
    "mark_types_during_sweeping": 0,
    "mark_incoming_black_pointers": 0,
    "mark_weak": 0,
    "mark_incoming_gray_pointers": 0,
    "mark_gray": 0,
    "mark_gray_and_weak": 0,
    "finalize_start_callback": 0,
    "sweep_atoms": 0,
    "sweep_symbol_registry": 0,
    "sweep_compartments": 0,
    "sweep_discard_code": 0,
    "sweep_inner_views": 0,
    "sweep_cross_compartment_wrappers": 0,
    "sweep_base_shapes": 0,
    "sweep_initial_shapes": 0,
    "sweep_type_objects": 0,
    "sweep_breakpoints": 0,
    "sweep_regexps": 0,
    "sweep_miscellaneous": 0,
    "sweep_type_information": 0,
    "sweep_type_tables_and_compilations": 0,
    "free_type_arena": 0,
    "sweep_object": 0,
    "sweep_string": 0,
    "sweep_script": 0,
    "sweep_shape": 0,
    "sweep_jit_code": 0,
    "finalize_end_callback": 0,
    "deallocate": 0,
    "compact": 0,
    "compact_move": 0,
    "compact_update": 0,
    "compact_update_cells": 0,
    "end_callback": 0,
    "all_minor_gcs": 0,
    "minor_gcs_to_evict_nursery": 0,
    "trace_heap": 0
  }
}
Does it make sense to expose all of this same data with the Debugger.Memory hook?

Should we organize it differently?

Are we worried about allocating some objects and running JS after every GC (when Debuggers are observing compartments in the GC'd zone)?

Is "totals" supposed to be the sum of each of those properties from each slice? If so, those numbers look wrong...

---------------

Initially, devtools just wants to be able to graph GC start and end, as well as differentiate incremental and minor GCs from major GCs caused by too much allocation. And then we would use the allocations log with this data to show the user which allocations are triggering GCs.

However, we definitely want to do more down the line, but it isn't clear to me exactly what that "more" is, and which bits of data we will need for it. Ideas welcome!

Would it make sense to show mark and sweep counts to web/app/fx-frontend devs? We would obviously need to have some kind of developer education story around the runtime of GC based on these values.

Maybe we should only expose the bare minimum needed for our initial plans and then add more as we need it? The other end of the spectrum would be to just dump everything we have here and we can let the frontend figure it out down the line.
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Flags: needinfo?(jimb)
TOO MANY NUMBERS SO CONFUSING

That looks like a very implementation-fitted set of statistics. I'm worried about coupling the devtools and any other consumers so tightly to the SpiderMonkey implementation.

Debugger in general tries to be more high-level than that, although we certainly compromise when it comes to performance measurements. I think it would be nice to specify the data we know devtools actually want, and do whatever processing is necessary on the SpiderMonkey side of the interface, rather than making toolkit/devtools/server know the meanings of all those numbers.
Flags: needinfo?(jimb)
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> Does it make sense to expose all of this same data with the Debugger.Memory
> hook?
> 
> Should we organize it differently?
> 
> Are we worried about allocating some objects and running JS after every GC
> (when Debuggers are observing compartments in the GC'd zone)?

Yes, but it's probably not enough volume to be worried about, unless it happens during our "gc until the heap is empty" shutdown GC's.

> Is "totals" supposed to be the sum of each of those properties from each
> slice? If so, those numbers look wrong...

No, it is the total time taken. We mark some tables in parallel, so ideally totals will be less than the sum on those phases. 

> ---------------
> 
> Initially, devtools just wants to be able to graph GC start and end, as well
> as differentiate incremental and minor GCs from major GCs caused by too much
> allocation. And then we would use the allocations log with this data to show
> the user which allocations are triggering GCs.

Minor GC's are counted in stats, but do not generate stats themselves, other than debug printing via JS_GC_PROFILE_NURSERY=<mintime> at the moment. We'll probably need to add some new infrastructure here, regardless.

> However, we definitely want to do more down the line, but it isn't clear to
> me exactly what that "more" is, and which bits of data we will need for it.
> Ideas welcome!
> 
> Would it make sense to show mark and sweep counts to web/app/fx-frontend
> devs? We would obviously need to have some kind of developer education story
> around the runtime of GC based on these values.
> 
> Maybe we should only expose the bare minimum needed for our initial plans
> and then add more as we need it? The other end of the spectrum would be to
> just dump everything we have here and we can let the frontend figure it out
> down the line.

I'm just going to quote Jim here.

(In reply to Jim Blandy :jimb from comment #3)
> TOO MANY NUMBERS SO CONFUSING
> 
> That looks like a very implementation-fitted set of statistics. I'm worried
> about coupling the devtools and any other consumers so tightly to the
> SpiderMonkey implementation.

Our current measurements and output are designed to get as much raw, actionable data as possible into the affected user's clipboard as possible. They are not really designed to be comprehensible to anyone who is not either Bill, or has a fair bit of knowledge, time, and a copy of js/src/ handy.

> Debugger in general tries to be more high-level than that, although we
> certainly compromise when it comes to performance measurements. I think it
> would be nice to specify the data we know devtools actually want, and do
> whatever processing is necessary on the SpiderMonkey side of the interface,
> rather than making toolkit/devtools/server know the meanings of all those
> numbers.

I agree. We need to totally change the interface anyway, so I think it makes sense to move carefully.
Flags: needinfo?(terrence)
"Me too."

If we're going expose highly internals-dependent data, it should be clearly quarantined into an "extra_info" auxiliary object or something. (And I actually think something like that isn't too crazy if it's gated by a pref or you have to install an addon or something. Buyer beware.) But the stats structures need to be free to mutate dramatically. Phases will get merged, split, and backgrounded. I'd definitely go for some minimal information, exposing things only as needed.

Minor GCs do show up in the stats somewhat in addition to the raw count of minor GCs. The time spent in PHASE_MARK_ROOTS is accumulated:

    { PHASE_MARK_ROOTS, "Mark Roots", PHASE_MULTI_PARENTS },
        { PHASE_MARK_CCWS, "Mark Cross Compartment Wrappers", PHASE_MARK_ROOTS },
        { PHASE_MARK_ROOTERS, "Mark Rooters", PHASE_MARK_ROOTS },
        { PHASE_MARK_RUNTIME_DATA, "Mark Runtime-wide Data", PHASE_MARK_ROOTS },
        { PHASE_MARK_EMBEDDING, "Mark Embedding", PHASE_MARK_ROOTS },
        { PHASE_MARK_COMPARTMENTS, "Mark Compartments", PHASE_MARK_ROOTS },

but since we're not going to expose this granularity, it doesn't matter.

Mainly, I think the user will want to know how much time is spent in GC, how much in the mutator, and how those GC pauses are distributed. Then they'll want to know what triggered those GCs. Perhaps it would be useful to see if some of the pause time was waiting on some background GC-related thread vs the mainthread GC, but that's already feeling secondary. Whether GCs are global vs zonal is probably interesting. An indication of whether the GC system thought we were animating and therefore targeting 10 ms vs some other duration *might* be of interest, though that's questionable (esp given that we may just move everything to a 10ms target.)
Flags: needinfo?(sphink)
Oh, and I'm wondering about how much to expose about minor GCs. Are they useful for the developers to know about? If they're always going to be <1ms, then they might just serve as a distraction. Then again, I'm pretty sure they can get way longer than that currently, but I was hoping that we could declare those to be bugs. But there *are* things a developer could do to avoid those, so...?
Ok, here's the format I think the data passed to the hook (Debugger.Memory.prototype.onGarbageCollection?) should have:

  [
    {
      start: Timestamp,          // same timestamp format as alloc log
      end: Timestamp,

      reason: GcReason,
    },

    .
    .
    .
  ]

One object entry in the array per GC slice.

GcReason would be something from https://dxr.mozilla.org/mozilla-central/source/js/public/GCAPI.h#45 perhaps? Or maybe we want to simplify it a bit more?

We can fire it the same as the GC stats event is fired now: after each (major?) GC cycle completion. (Is this correct?) Aside: this also gives API users a good time to know when to drain the allocations log.

Thoughts?
Maybe also a timestamp since the last GC event? That way you could compute time spent in GC vs in the mutator.
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Flags: needinfo?(jimb)
(In reply to Steve Fink [:sfink, :s:] from comment #6)
> Oh, and I'm wondering about how much to expose about minor GCs. Are they
> useful for the developers to know about? If they're always going to be <1ms,
> then they might just serve as a distraction. Then again, I'm pretty sure
> they can get way longer than that currently, but I was hoping that we could
> declare those to be bugs. But there *are* things a developer could do to
> avoid those, so...?

I think we should expose them, and if they aren't taking much time, then our visualization will de-emphasize them. If they are going longer than a ms, then the visualization will give them more emphasis, and at that point the developer can start to worry about them.
(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Ok, here's the format I think the data passed to the hook
> (Debugger.Memory.prototype.onGarbageCollection?) should have:
> 
>   [
>     {
>       start: Timestamp,          // same timestamp format as alloc log
>       end: Timestamp,
> 
>       reason: GcReason,
>     },
> 
>     .
>     .
>     .
>   ]
> 
> One object entry in the array per GC slice.

I think that looks okay. My only suggestion might be returning { collections: [ { start: ... }, ... ] } --- that is, just wrapping the whole thing in an object --- to make it easier to add additional values. The hook could be defined as:

  function hook({ collections })

and then hooks can just add properties as we provide them and they become interested in them.
Flags: needinfo?(jimb)
Seems reasonable. The only worry is that our GC trigger reasons are somewhere between complex and complicated, so making those actionable is going to be difficult. I guess you can expose them and work on one reason at a time.
Flags: needinfo?(terrence)
Depends on: 1139993
Summary: Expose GC stats as a Debugger.Memory hook → [jsdbg2] Add a Debugger.Memory.prototype.onGarbageCollected hook
(Still working on implementation for firing the hook, but I think these bits are self contained and can be reviewed now. Especially the docs for the hook, to make sure I am impl'ing what we want to expose.)
Summary: [jsdbg2] Add a Debugger.Memory.prototype.onGarbageCollected hook → [jsdbg2] Add a Debugger.Memory.prototype.onGarbageCollection hook
Attachment #8574955 - Attachment is obsolete: true
Attachment #8574955 - Flags: review?(jimb)
Attachment #8575599 - Flags: review?(jimb)
(In reply to Nick Fitzgerald [:fitzgen] from comment #18)
> Created attachment 8575601 [details] [diff] [review]
> Part 4: Test the Debugger.Memory.prototype.onGarbageCollection hook

This part is still a WIP -- not sure how to test GC slices in a nice way.
Attachment #8574954 - Attachment is obsolete: true
Attachment #8574954 - Flags: review?(jimb)
Attachment #8575648 - Flags: review?(sphink)
Attachment #8575599 - Attachment is obsolete: true
Attachment #8575599 - Flags: review?(jimb)
Attachment #8575649 - Flags: review?(sphink)
Attachment #8575600 - Attachment is obsolete: true
Attachment #8575600 - Flags: review?(jimb)
Attachment #8575650 - Flags: review?(sphink)
(Swapping review to :sfink because I already have 5 patches in jimb's review queue, and they've been in there for a couple weeks now. (We need more Debugger reviewers.))
Comment on attachment 8575648 [details] [diff] [review]
Part 1: Make {get,set}HookImpl not rely on a Debugger instance in the 'this' slot

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

REVIEW_COMMENTS(1137844, true, Comments::HARD_TO_FOLLOW | Comments::ACCEPT_ANYWAY, "THIS_DEBUGGER")
Attachment #8575648 - Flags: review?(sphink) → review+
Attachment #8575649 - Flags: review?(sphink) → review+
Comment on attachment 8575650 [details] [diff] [review]
Part 3: Fire the Debugger.Memory.prototype.onGarbageCollection hook after GCs

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

r=me unless the setGCState condition isn't straightforward.

::: js/src/gc/Zone.h
@@ +165,5 @@
>          MOZ_ASSERT(runtimeFromMainThread()->isHeapBusy());
>          MOZ_ASSERT_IF(state != NoGC, canCollect());
>          gcState_ = state;
> +        if (state != NoGC)
> +            notifyObservingDebuggers();

Hmm... that means you do this 4-5 times per GC? Or would this see a setGCState(Mark)...setGCState(Mark)...setGCState(Mark)... during an incremental GC, in which case you'd get a callback for every slice? What do you want?

I'd expect if (state == Finished).

::: js/src/vm/Debugger.cpp
@@ +1315,5 @@
> +    MOZ_ASSERT(cx);
> +
> +    RootedObject hook(cx, getHook(OnGarbageCollection));
> +    MOZ_ASSERT(hook);
> +    MOZ_ASSERT(hook->isCallable());

This is guaranteed? If some sick bastard did debugger.onGarbageCollection = 78, it would be caught earlier? I guess I should read the code.

::: js/src/vm/Debugger.h
@@ +236,5 @@
>      // Return true if the given compartment is a debuggee of this debugger,
>      // false otherwise.
>      bool isDebuggee(const JSCompartment *compartment) const;
>  
> +    // Notify this Debugger that one of its compartment's zones is being

compartments'

@@ +685,5 @@
> +     * Converts an implementor level of detail gcstats::Statistics object into a
> +     * JSObject that web developers should be able to make sense of. Returns
> +     * nullptr on failure.
> +     */
> +    JSObject *wrapGCStatistics(JSContext *cx, const gcstats::Statistics &stats);

please, not "wrap"! That far too strongly implies cross-compartment wrapping (or worse, x-rays or whatever.)

translate? parse? extract? explain?

or if you think of it from the other side, make?
Attachment #8575650 - Flags: review?(sphink) → review+
Comment on attachment 8575651 [details] [diff] [review]
Part 4: Test the Debugger.Memory.prototype.onGarbageCollection hook

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

Let's agree on the assert first.

::: js/src/jit-test/tests/debug/Memory-onGarbageCollection-01.js
@@ +24,5 @@
> +    assertEq(typeof slice.reason, "string");
> +  }
> +
> +  // NUM_SLICES incremental gc slices + 1 full gc
> +  assertEq(data.collections.length, NUM_SLICES + 1);

I don't think this is in any way guaranteed. Try running it with JS_GC_ZEAL=2,5 . I'm worried that one of our test builds will trigger an extra GC or two.

Maybe just data.collections.length >= NUM_SLICES + 1? Or use gcparam("gcNumber") somehow?

@@ +46,5 @@
> +
> +  // Full GC
> +  waitOneHundredMilliseconds();
> +  for (var i = 100; i > 0; i--)
> +    allocs.push({});

you have allocs here and this.allocs in the previous loop.
Attachment #8575651 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink, :s:] from comment #26)
> Comment on attachment 8575650 [details] [diff] [review]
> Part 3: Fire the Debugger.Memory.prototype.onGarbageCollection hook after GCs
> 
> Review of attachment 8575650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me unless the setGCState condition isn't straightforward.
> 
> ::: js/src/gc/Zone.h
> @@ +165,5 @@
> >          MOZ_ASSERT(runtimeFromMainThread()->isHeapBusy());
> >          MOZ_ASSERT_IF(state != NoGC, canCollect());
> >          gcState_ = state;
> > +        if (state != NoGC)
> > +            notifyObservingDebuggers();
> 
> Hmm... that means you do this 4-5 times per GC? Or would this see a
> setGCState(Mark)...setGCState(Mark)...setGCState(Mark)... during an
> incremental GC, in which case you'd get a callback for every slice? What do
> you want?
> 
> I'd expect if (state == Finished).

Good catch.

> 
> ::: js/src/vm/Debugger.cpp
> @@ +1315,5 @@
> > +    MOZ_ASSERT(cx);
> > +
> > +    RootedObject hook(cx, getHook(OnGarbageCollection));
> > +    MOZ_ASSERT(hook);
> > +    MOZ_ASSERT(hook->isCallable());
> 
> This is guaranteed? If some sick bastard did debugger.onGarbageCollection =
> 78, it would be caught earlier? I guess I should read the code.

Yep, setHookImpl will throw in the setter if anyone tries such foolishness.
Comment on attachment 8574953 [details] [diff] [review]
Part 0: Add docs for Debugger.Memory.prototype.onGarbageCollection

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

This looks good to me.

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +131,5 @@
> +          "reason": <i>reasonString</i>
> +        }
> +        </code></pre>
> +
> +        Here *timestamp* is the timestamp of the event in units of microseconds

This is cut-and-paste from drainAllocationsLog, right? You've got startTimestamp and endTimestamp properties here.

Let's actually add a section "Timestamps" to Conventions.md that specifies that all timestamps are in microseconds since the epoch, and says whatever can be said about whatever the epoch is. Then, drainAllocationsLog and onGarbageCollection can both just cite that.

@@ +133,5 @@
> +        </code></pre>
> +
> +        Here *timestamp* is the timestamp of the event in units of microseconds
> +        since the epoch, and *reasonString* is a very short string describing
> +        the reason why the collection slice was triggered.

Let's list the reasonStrings we know, even if they're SM-specific.

Based on IRC chat with Terrence, it seems like this will move to the overall 'statistics' object, as individual slices don't have reasons any more. (???)
Attachment #8574953 - Flags: review?(jimb) → feedback+
(In reply to Nick Fitzgerald [:fitzgen] from comment #30)
> Created attachment 8576108 [details] [diff] [review]
> Part 3: Fire the Debugger.Memory.prototype.onGarbageCollection hook after GCs

This hasn't really changed, except for the introduction of AutoOnGCHookReentrancyGuard. Rerequesting review just for that part.
Comment on attachment 8576108 [details] [diff] [review]
Part 3: Fire the Debugger.Memory.prototype.onGarbageCollection hook after GCs

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

::: js/src/vm/Debugger.h
@@ +263,5 @@
>  
> +    // During a GC cycle, this is true if one of this Debugger's debuggees was
> +    // collected. When the GC cycle completes, this flag is reset.
> +    bool debuggeeWasCollected;
> +    // True while we are executing the onGarbageCollection hook, and therefore

Add a blank line
Attachment #8576108 - Flags: review?(sphink) → review+
Comment on attachment 8576109 [details] [diff] [review]
Part 4: Test the Debugger.Memory.prototype.onGarbageCollection hook

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

Nice tests!

::: js/src/jit-test/tests/debug/Memory-onGarbageCollection-01.js
@@ +55,5 @@
> +
> +// NUM_SLICES + 1 full gc + however many were triggered naturally (due to
> +// whatever zealousness setting).
> +print("Found " + slicesFound + " slices");
> +assertEq(slicesFound > NUM_SLICES, true);

I would prefer this to be slicesFound >= NUM_SLICES + 1. I know they're the same mathematically, but it's a better semantic match.

::: js/src/jit-test/tests/debug/Memory-onGarbageCollection-02.js
@@ +1,1 @@
> +// Test multiple debuggers, GCs, and zones interacting with eachother.

each other
Attachment #8576109 - Flags: review?(sphink) → review+
Comment on attachment 8576166 [details] [diff] [review]
Part 0: Add docs for Debugger.Memory.prototype.onGarbageCollection

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

*\0/*

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +99,5 @@
> +call when given events occur in debuggee code.
> +
> +Unlike `Debugger`'s hooks, `Debugger.Memory`'s handlers' return values are not
> +significant, and are ignored. The handler functions receive the
> +`Debugger.Memory`'s owning `Debugger` instance as their `this` value.

uncaughtExceptionHandler still works for them, though, right?

::: js/src/doc/Debugger/config.sh
@@ +10,5 @@
>    label 'conventions'                           "Debugger API: General Conventions"
>    label 'dbg code'      '#debuggee-code'        "Debugger API: General Conventions: Debuggee Code"
>    label 'cv'            '#completion-values'    "Debugger API: General Conventions: Completion Values"
>    label 'rv'            '#resumption-values'    "Debugger API: General Conventions: Resumption Values"
> +  label 'timestamps'    '#timestamps'           "Debugger API: General COnventions: Timestamps"

"COnventions"
Attachment #8576166 - Flags: review?(jimb) → review+
Flags: needinfo?(sphink)
Looks like I actually need to fix a hazard...
Keywords: checkin-needed
I think all that was jsut infra, but here is a new try push rebased on top of the latest m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e412f187e0
Depends on: 1144108
Depends on: 1144356
Depends on: 1148925
You need to log in before you can comment on or make changes to this bug.