1214066 - UI for toggling the recording of allocation stacks on/off

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

The memory tool needs a way to toggle the recording of allocation stacks on and off. Heap snapshots are vastly more informative if allocation stack recording was enabled when the things that are retained in the heap were allocated, however JS which allocates many objects can run about 4x slower when recording allocation stacks is enabled. Because of that drawback, we can't leave it on all the time, and have to put the user in control of whether it is enabled or not at any given time. Typically, I expect users to enable it for most of the duration of a memory-debugging session, but would definitely want to disable it when switching to a performance-debugging session.

Not sure what the UI should look like here. Checkbox? Button with toggled on/off state? I think we should do the thing where we highlight the panel's tab/label when recording allocation stacks is enabled like how we do for the performance tool when it is recording a profile or with the debugger when it is paused. That isn't interactive, however, just informative for the user (especially when switching to a different panel).

Any suggestions, Helen?

Comment 1

[Helen V. Holmes (:helenvholmes) (:✨)]

I would recommend a checkbox, honestly, since it's a) clear and b) we're already using it elsewhere. Not super exciting, but it would definitely keep you from being blocked. Ultimately it might be worth revisiting and switching all of these sorts of edge cases to use the same language (I actually really like the sideways toggle you see that Apple uses for on/off states).

For the moment we can probably highlight the panel as green like performance does; were you imagining like the debugger that we might have an icon that changes?

Comment 2

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

Highlighting the panel green when going to another panel while recording allocations is pretty easy -- I don't think we'll need another icon for that state luckily.

+1 on checkbox for now (with some warning/tooltip maybe to mention overhead in any tool?), and more +1s for consistent checkbox styles/sideways toggles

Comment 3

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

Attachment: Add the ability to toggle allocation stack recording

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4374cb709ab7

Comment 5

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

Comment on attachment 8678294 [details] [diff] [review]
Add the ability to toggle allocation stack recording

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

Looks awesome. Some nits, test coverage

::: devtools/client/memory/actions/allocations.js
@@ +11,5 @@
> +
> +    if (getState().recordingAllocationStacks) {
> +      yield front.stopRecordingAllocations();
> +    } else {
> +      yield front.startRecordingAllocations({

Should probably store this config object in constants

::: devtools/client/memory/app.js
@@ +79,5 @@
>   */
>  function mapStateToProps (state) {
> +  return {
> +    allocations: state.allocations,
> +    snapshots: state.snapshots

oh hum, wonder if we should add the other state props here (errors, breakdown)

::: devtools/client/memory/reducers/allocations.js
@@ +7,5 @@
> +
> +let handlers = Object.create(null);
> +
> +handlers[actions.TOGGLE_RECORD_ALLOCATION_STACKS_START] = function (state, action) {
> +  assert(!state.recordingChanging,

Wonder if it makes more sense to pass in a `recording` bool in the action so we can turn on/off recording based off of the action, rather than the current state of "toggling" -- it shouldn't matter in practice, because with the UI button, it's only every going to toggle, but this might make things more robust in the future

@@ +23,5 @@
> +         + "recording state already.");
> +
> +  return {
> +    recording: state.recording,
> +    recordingChanging: false,

I don't like this prop name, but not sure what to call it. Like, isBusy or isProcessing or isAllocationStateChanging?

::: devtools/client/memory/test/unit/test_action-toggle-recording-allocations.js
@@ +35,5 @@
> +
> +  equal(getState().allocations.recording, false, "now we are not recording");
> +  equal(getState().allocations.recordingChanging, false,
> +        "done toggling, not in the middle of changing recording");
> +  ok(front.recordingAllocations, "front is not recording anymore");

We should add a test here that after the START action is fired, that `recordingChanging` is true. Can use `waitUntilState` helper for a quick predicate check on store change:

dispatch(toggleRecordingAllocationStacks(front))
yield waitUntilState(store, () => getState().allocations.recordingChanging);
ok(true, "`recordingChanging` set to true when toggling");
yield waitUntilState(store, () => !getState().allocations.recordingChanging);
// assert toggling occurred here

Comment 6

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5)
> ::: devtools/client/memory/reducers/allocations.js
> @@ +7,5 @@
> > +
> > +let handlers = Object.create(null);
> > +
> > +handlers[actions.TOGGLE_RECORD_ALLOCATION_STACKS_START] = function (state, action) {
> > +  assert(!state.recordingChanging,
> 
> Wonder if it makes more sense to pass in a `recording` bool in the action so
> we can turn on/off recording based off of the action, rather than the
> current state of "toggling" -- it shouldn't matter in practice, because with
> the UI button, it's only every going to toggle, but this might make things
> more robust in the future

If we need to change it in the future, we can do it then. I like how it doesn't mirror any of the existing state right now, and the assertions should catch any misuse from future changes.

> devtools/client/memory/test/unit/test_action-toggle-recording-allocations.js
> @@ +35,5 @@
> > +
> > +  equal(getState().allocations.recording, false, "now we are not recording");
> > +  equal(getState().allocations.recordingChanging, false,
> > +        "done toggling, not in the middle of changing recording");
> > +  ok(front.recordingAllocations, "front is not recording anymore");
> 
> We should add a test here that after the START action is fired, that
> `recordingChanging` is true. Can use `waitUntilState` helper for a quick
> predicate check on store change:
> 
> dispatch(toggleRecordingAllocationStacks(front))
> yield waitUntilState(store, () => getState().allocations.recordingChanging);
> ok(true, "`recordingChanging` set to true when toggling");
> yield waitUntilState(store, () => !getState().allocations.recordingChanging);
> // assert toggling occurred here

Cool, I wasn't sure how to test it so I punted. This is great, thanks.

Comment 7

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

Attachment: Add the ability to toggle allocation stack recording

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/06a0edd5da4d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06a0edd5da4d