1137527 - Memory actor should expose GC events

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

Should send GC stats api info from memory actor

Comment 1

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

Attachment: Part 1: Make the memory actor emit events for cycle collection and garbage collection

Comment 2

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

Attachment: Part 2: Add a test for GC and CC MemoryActor events

Comment 3

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

Attachment: Part 3: Fix code example in protocol.js.md

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 5

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

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.

Comment 6

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

Attachment: Part 1: Make the memory actor emit events for garbage collection

Comment 7

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

Attachment: Part 2: Add a test for GC MemoryActor events

Comment 8

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

Attachment: Part 3: Fix code example in protocol.js.md

Comment 9

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

Attachment: Part 1: Make the memory actor emit events for garbage collection

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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!

Comment 11

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

ni? me to land this when fx-team opens up again.

Comment 12

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e6dbbba3f83

Comment 13

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

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.

Comment 14

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

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9de4e26cef9

Comment 15

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

Attachment: stack trace

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

Comment 16

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

Attachment: Part 1: Make the memory actor emit events for garbage collection

Comment 17

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

Attachment: Part 2: Add a test for GC MemoryActor events

Comment 18

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

Attachment: Part 3: Fix code example in protocol.js.md

Comment 19

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

Rebased now that bug 1150253 landed, and here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d0cc33ed4d5

Comment 20

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

Comment on attachment 8596903 [details] [diff] [review]
Part 1: Make the memory actor emit events for garbage collection

Carrying over r+

Comment 21

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

Comment on attachment 8596904 [details] [diff] [review]
Part 2: Add a test for GC MemoryActor events

Carrying over r+

Comment 22

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

Comment on attachment 8596905 [details] [diff] [review]
Part 3: Fix code example in protocol.js.md

Carrying over r+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ee6125ec034c
https://hg.mozilla.org/integration/fx-team/rev/1ead03b75744
https://hg.mozilla.org/integration/fx-team/rev/f9a7d3620d58

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee6125ec034c
https://hg.mozilla.org/mozilla-central/rev/1ead03b75744
https://hg.mozilla.org/mozilla-central/rev/f9a7d3620d58