1224077 - MemoryActor should use "runtime: true" rather than "debuggees: this.dbg" when saving snapshots from browser toolbox

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

Even though we ahve all globals as debuggees, it affects the root set of the heap snapshot as well as the edges that get traversed.

Comment 1

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

Attachment: Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

Comment 2

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

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

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

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

::: devtools/server/performance/memory.js
@@ +142,5 @@
>     *
>     * @returns {String} The snapshot id.
>     */
>    saveHeapSnapshot: expectState("attached", function () {
> +    const opts = this.parent instanceof ChromeActor || this.parent instanceof ChildProcessActor

Comments here on what this is doing, and when this would be called (browser toolbox for ChromeActor, and FxOS for the ChildProcessActors?) What is parent an instance of when using this.dbg?

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/08d771037660

Comment 5

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

ni myself to request uplift once this sits in m-c for a couple days

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08d771037660

Comment 7

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

Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

Approval Request Comment
[Feature/regressing bug #]: bug 1200446
[User impact if declined]: Saving a heap snapshot for the whole browser rather than a specific tab will be subtly wrong
[Describe test coverage new/current, TreeHerder]: Pretty extensive test suite, been green in m-c for a few days
[Risks and why]: Minimal
[String/UUID change made/needed]: None

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

If this is not a recent regression (i.e. since 42 released) and given the size of the patch (possible risk associated(, I would like this to ride the trains. 

Are our end-users noticing the subtle difference in the memory snapshots and complaining about it? I am just trying to minimize code churn and only uplift patches that are low risk+high end-user impact. Please let me know if you have any concerns.

Comment 9

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

(In reply to Ritu Kothari (:ritu) from comment #8)
> Comment on attachment 8687277 [details] [diff] [review]
> Scope the boundaries of full runtime heap snapshots taken from the
> MemoryActor properly
> 
> If this is not a recent regression (i.e. since 42 released) and given the
> size of the patch (possible risk associated(, I would like this to ride the
> trains. 
> 
> Are our end-users noticing the subtle difference in the memory snapshots and
> complaining about it? I am just trying to minimize code churn and only
> uplift patches that are low risk+high end-user impact. Please let me know if
> you have any concerns.

We didn't expose this functionality to users until 44. No one has noticed, but I'd like to reconsider as fxos branches off of 44, and people debugging fxos are among those who will be most affected. The risk of taking this patch is *very* low.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #9)
> (In reply to Ritu Kothari (:ritu) from comment #8)
> > Comment on attachment 8687277 [details] [diff] [review]
> > Scope the boundaries of full runtime heap snapshots taken from the
> > MemoryActor properly
> > 
> > If this is not a recent regression (i.e. since 42 released) and given the
> > size of the patch (possible risk associated(, I would like this to ride the
> > trains. 
> > 
> > Are our end-users noticing the subtle difference in the memory snapshots and
> > complaining about it? I am just trying to minimize code churn and only
> > uplift patches that are low risk+high end-user impact. Please let me know if
> > you have any concerns.
> 
> We didn't expose this functionality to users until 44. No one has noticed,
> but I'd like to reconsider as fxos branches off of 44, and people debugging
> fxos are among those who will be most affected. The risk of taking this
> patch is *very* low.

Nick, thanks for the additional context. It was unclear to me from the original aurora uplift request that this may affect fxos end-users. Also, upon looking at the patch again, I noticed that most of the change is in the test automation. I guess I will take this patch in Aurora.

Comment 11

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

Thanks! I will try and write better requests in the future :)

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687277 [details] [diff] [review]
Scope the boundaries of full runtime heap snapshots taken from the MemoryActor properly

See comment 10 for justification. Aurora44+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e6d20d6dba3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2e6d20d6dba3