Closed Bug 1137527 Opened 5 years ago Closed 5 years ago

Memory actor should expose GC events

Categories

(DevTools :: Memory, defect)

x86
macOS
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(3 files, 8 obsolete files)

2.74 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
2.32 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
970 bytes, patch
fitzgen
: review+
Details | Diff | Splinter Review
Should send GC stats api info from memory actor
Attachment #8570268 - Flags: review?(jryans)
Attachment #8570269 - Flags: review?(jryans)
Comment on attachment 8570268 [details] [diff] [review]
Part 1: Make the memory actor emit events for cycle collection and garbage collection

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

Generally fine, but let's rethink the pref part.

I am out next week, may want someone else to do the next review.

::: toolkit/devtools/server/actors/memory.js
@@ +156,5 @@
> +                                false);
> +    Services.obs.removeObserver(this._onGarbageCollection,
> +                                "garbage-collection-statistics",
> +                                false);
> +    Services.prefs.setBoolPref("javascript.options.mem.notify",

Is this something we do from other actors?  It has the potential to get confused if you connect several toolboxes:

1. Connect page toolbox A, it captures false as original
2. Use memory feature in toolbox A
3. Connect some other toolbox B, to a new page, or browser toolbox, it capture true as original
4. Use memory feature in toolbox B
5. Close toolbox A, it restores false
6. Close toolbox B, it restores true

So, I think we need a smarter way to track the original value here.
Attachment #8570268 - Flags: review?(jryans) → feedback+
Ok the pref story is a mess with multiple debugger servers and multiple processes. We are gonna expose this API on Debugger.Memory in a much more sane way. Bug 1137844.
Depends on: 1137844
Attachment #8570269 - Attachment is obsolete: true
Attachment #8570270 - Attachment is obsolete: true
Attachment #8578964 - Flags: review?(jryans)
Attachment #8578964 - Attachment is obsolete: true
Attachment #8578964 - Flags: review?(jryans)
Attachment #8578972 - Flags: review?(jryans)
Comment on attachment 8578972 [details] [diff] [review]
Part 1: Make the memory actor emit events for garbage collection

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

Yay, much simpler!
Attachment #8578972 - Flags: review?(jryans) → review+
ni? me to land this when fx-team opens up again.
Flags: needinfo?(nfitzgerald)
I _think_ those failures are unrelated. Tried to do a new try push, rebased on m-c, but try is closed. Will do it later.
Flags: needinfo?(nfitzgerald)
Attached file stack trace (obsolete) —
I have a sneaking suspicion that the only way that new objects could enter the nursery between a nursery eviction and a markRuntime would be if there was some kind of "on garbage collection" hook that ran JS after the nursery was evicted and would therefore trigger the assertions we are seeing in this test...

Crap. I think we may have to rethink the way the onGarbageCollection hook is fired. Perhaps related to bug 1144356.

FWIW, here is the relevant part of the backtrace:

    frame #0: 0x000000010712f761 XUL`js::gc::ZoneCellIterUnderGC::ZoneCellIterUnderGC(this=0x00007fff5fbe3b60, zone=0x000000010057b000, kind=JITCODE) + 129 at jsgcinlines.h:260
    frame #1: 0x00000001070e3466 XUL`js::gc::ZoneCellIterUnderGC::ZoneCellIterUnderGC(this=0x00007fff5fbe3b60, zone=0x000000010057b000, kind=JITCODE) + 38 at jsgcinlines.h:263
    frame #2: 0x00000001072e1d3a XUL`js::jit::JitRuntime::Mark(trc=0x00007fff5fbe3f50) + 154 at Ion.cpp:494
    frame #3: 0x0000000107136aff XUL`js::gc::GCRuntime::markRuntime(this=0x00000001178d1348, trc=0x00007fff5fbe3f50, traceOrMark=TraceRuntime, rootsSource=TraceRoots) + 1791 at RootMarking.cpp:476
    frame #4: 0x0000000106e12ac9 XUL`js::TraceRuntime(trc=0x00007fff5fbe3f50) + 249 at Iteration.cpp:29
    frame #5: 0x000000010713d426 XUL`JS_TraceRuntime(trc=0x00007fff5fbe3f50) + 38 at Tracer.cpp:121
    frame #6: 0x00000001070a7bc7 XUL`JS::ubi::RootList::init(this=0x00007fff5fbe4568, debuggees=0x00007fff5fbe4180) + 151 at UbiNode.cpp:298
Depends on: 1150253
Summary: Memory actor should expose GC stats API → Memory actor should expose GC events
Attachment #8578972 - Attachment is obsolete: true
Attachment #8582112 - Attachment is obsolete: true
Comment on attachment 8596903 [details] [diff] [review]
Part 1: Make the memory actor emit events for garbage collection

Carrying over r+
Attachment #8596903 - Flags: review+
Comment on attachment 8596904 [details] [diff] [review]
Part 2: Add a test for GC MemoryActor events

Carrying over r+
Attachment #8596904 - Flags: review+
Comment on attachment 8596905 [details] [diff] [review]
Part 3: Fix code example in protocol.js.md

Carrying over r+
Attachment #8596905 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ee6125ec034c
https://hg.mozilla.org/mozilla-central/rev/1ead03b75744
https://hg.mozilla.org/mozilla-central/rev/f9a7d3620d58
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.