Closed Bug 1214066 Opened 4 years ago Closed 4 years ago

UI for toggling the recording of allocation stacks on/off

Categories

(DevTools :: Memory, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(hholmes)
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?
Flags: needinfo?(hholmes)
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
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
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
Attachment #8678294 - Flags: review?(jsantell) → review+
(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.
Attachment #8678294 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/06a0edd5da4d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.