This is orthogonal (and less important) than the heap views, but this will be easier to get done and wire up the HeapAnalysisClient worker for bug 960776 without having to get caught up in focusing on tree rendering.

Is this the bug for total count/bytes to be displayed in the list as part of the snapshot's label?

No; this is for all the other wiring together except actually rendering the data in a tree (and maybe just as a JSON dump, for example)

This patch sets up all the state changes between the list and main view. There's not much styling, and the tree view will be handled in another patch, but this is the rough idea. If it looks good I'll send for full review then.

Also check the state changes/selection how they load and when censuses are being generated -- does this make sense/look good, or should we do another way of handling states? Gif of this in action: http://i.imgur.com/3I7n4kc.gif

Comment on attachment 8674072 [details] [diff] [review] 1214872-memory-state.patch Played with it, pretty groovy! When we hit the snapshot button, should we automatically switch to the new snapshot? Right now it leaves you at the snapshot you are currently viewing. Will glance at the code now...

Comment on attachment 8674072 [details] [diff] [review] 1214872-memory-state.patch Review of attachment 8674072 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/memory/app.js @@ +16,5 @@ > /** > + * {HeapAnalysesClient} > + * Used to communicate with the worker that performs analyses on heaps. > + */ > + heapWorker: PropTypes.any, uber nit: I think "analysesWorker" (or even "heapAnalysesWorker") might be a better name @@ +72,5 @@ > + console.log("Selected snapshot:", selectedSnapshot); > + if (selectedSnapshot && > + selectedSnapshot.state === "loaded" && > + selectedSnapshot.censusState === "empty") { > + dispatch(takeCensus(heapWorker, selectedSnapshot, BREAKDOWN)); Dispatching inside render() seems really weird to me. Shouldn't that be triggered when the loading promise is resolved (however that flows through redux)? I think we want to use some kind of middleware (do we have the thunk middleware? looks like yes) to dispatch multiple actions asyncly. See http://redux.js.org/docs/advanced/AsyncActions.html and the "fetchPosts" example. ::: devtools/client/memory/components/heap.js @@ +29,5 @@ > + dom.div({ className: "heap-view-panel", "data-state": "initial" }, > + dom.button({ className: "take-snapshot", onClick: onSnapshotClick }, TAKE_SNAPSHOT_TEXT) > + ), > + dom.div({ className: "heap-view-panel", "data-state": "loading" }, "Taking snapshot..."), > + dom.div({ className: "heap-view-panel", "data-state": "loading-census" }, "Taking census..."), nitty nit nit: "Saving snapshot" and "taking census"? That way we aren't just taking everything? (gimme mines) @@ +30,5 @@ > + dom.button({ className: "take-snapshot", onClick: onSnapshotClick }, TAKE_SNAPSHOT_TEXT) > + ), > + dom.div({ className: "heap-view-panel", "data-state": "loading" }, "Taking snapshot..."), > + dom.div({ className: "heap-view-panel", "data-state": "loading-census" }, "Taking census..."), > + dom.div({ className: "heap-view-panel", "data-state": "loaded" }, JSON.stringify(censusData || {})) Rather than doing the fancy CSS hide/show thing here, I think it would be better to only render the div that corresponds to the active state.

Comment on attachment 8674072 [details] [diff] [review] 1214872-memory-state.patch Review of attachment 8674072 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/memory/app.js @@ +16,5 @@ > /** > + * {HeapAnalysesClient} > + * Used to communicate with the worker that performs analyses on heaps. > + */ > + heapWorker: PropTypes.any, no, because to remember if analysis is plural or not is too hard (not even joking), and think it makes more sense to be `analysis` anyway, leading to more confusion, and I'm arguably a native english speaker @@ +69,5 @@ > + let { dispatch, snapshots, front, heapWorker } = this.props; > + let selectedSnapshot = snapshots.find(s => s.selected); > + > + console.log("Selected snapshot:", selectedSnapshot); > + if (selectedSnapshot && This is where my biggest uncertainty lay -- the displaying of a census is a two step process: 1) Take a snapshot 2) Read that snapshot as a census Now we could do 2) a few times if we change our census viewing rules (breakdown), but both of these steps should show state progress, and not sure how the best way to do that in redux/react, since if we made it one async action, we wouldn't have updates inbetween. So is this something the main app's render function should handle, or should this be a separate `store.subscribe()` to look for states that need to fetch a census? TLDR: What's the best way to do sequential async actions in redux? @@ +72,5 @@ > + console.log("Selected snapshot:", selectedSnapshot); > + if (selectedSnapshot && > + selectedSnapshot.state === "loaded" && > + selectedSnapshot.censusState === "empty") { > + dispatch(takeCensus(heapWorker, selectedSnapshot, BREAKDOWN)); Will clean this up in bug 1215302 ::: devtools/client/memory/components/heap.js @@ +29,5 @@ > + dom.div({ className: "heap-view-panel", "data-state": "initial" }, > + dom.button({ className: "take-snapshot", onClick: onSnapshotClick }, TAKE_SNAPSHOT_TEXT) > + ), > + dom.div({ className: "heap-view-panel", "data-state": "loading" }, "Taking snapshot..."), > + dom.div({ className: "heap-view-panel", "data-state": "loading-census" }, "Taking census..."), SGTM @@ +30,5 @@ > + dom.button({ className: "take-snapshot", onClick: onSnapshotClick }, TAKE_SNAPSHOT_TEXT) > + ), > + dom.div({ className: "heap-view-panel", "data-state": "loading" }, "Taking snapshot..."), > + dom.div({ className: "heap-view-panel", "data-state": "loading-census" }, "Taking census..."), > + dom.div({ className: "heap-view-panel", "data-state": "loaded" }, JSON.stringify(censusData || {})) SGTM

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7) > This is where my biggest uncertainty lay -- the displaying of a census is a > two step process: > > 1) Take a snapshot > 2) Read that snapshot as a census > > Now we could do 2) a few times if we change our census viewing rules > (breakdown), but both of these steps should show state progress, and not > sure how the best way to do that in redux/react, since if we made it one > async action, we wouldn't have updates inbetween. > > So is this something the main app's render function should handle, or should > this be a separate `store.subscribe()` to look for states that need to fetch > a census? > > TLDR: What's the best way to do sequential async actions in redux? I think the answer is composing each async action in a task that does them in order (bug 1215251 enables multiple dispatches, nullifying the problem with a "single" async action).

Need to add tests for the new actions, and I think it's good for some mochitest/integration tests now as well. I'll file some follow up bugs for robustness (like deleting a snapshot in the middle of saving/reading/censusing, changing breakdown in the middle as well, etc)

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7) > > @@ +69,5 @@ > > + let { dispatch, snapshots, front, heapWorker } = this.props; > > + let selectedSnapshot = snapshots.find(s => s.selected); > > + > > + console.log("Selected snapshot:", selectedSnapshot); > > + if (selectedSnapshot && > > This is where my biggest uncertainty lay -- the displaying of a census is a > two step process: > > 1) Take a snapshot > 2) Read that snapshot as a census > > Now we could do 2) a few times if we change our census viewing rules > (breakdown), but both of these steps should show state progress, and not > sure how the best way to do that in redux/react, since if we made it one > async action, we wouldn't have updates inbetween. > > So is this something the main app's render function should handle, or should > this be a separate `store.subscribe()` to look for states that need to fetch > a census? > > TLDR: What's the best way to do sequential async actions in redux? I'm assuming this is where the task middleware came from, and that works for me. I still think that you shouldn't need to call `dispatch` manually in the task very much. You should be calling other action creators that emit the promise states, and the UI will respond to those individually. Most likely, in your workflow, the UI will be interested in the last promise action: a READ_SNAPSHOT_CENSUS action with a `{ status: "done" }` came through and it just does what you need them to the UI. No need to dispatch extra things in the task. This way the UI can always respond to errors specific to certain actions, or it can fill in the UI as things come in (like if you used Promise.all to execute some things at the same time). Anyway, just over-communicating and you may already be thinking this.

It is nice the promise middleware tags for errors -- work arounds for this would be try/catch dispatching in the generator, or can change it to be like the promise middleware, where we return an object with a GEN field and action type. Or we can just use Task in the action itself, as it returns a promise, with the promise moddleware. Either way, this'll be easy to change to whatever we want, I'm not sure I quite get what you mean the task should not be dispatching, but we can try some real examples with this case in this bug (but in the future)

Comment on attachment 8674663 [details] [diff] [review] 1214872-memory-state.patch Review of attachment 8674663 [details] [diff] [review]: ----------------------------------------------------------------- I think the whole task + not having render dispatch the next action in the chain is looking really great. Much better than before. r=me with a few comments below, mostly nits. ::: devtools/client/memory/actions/snapshot.js @@ +13,2 @@ > */ > +const takeSnapshot = exports.takeSnapshot = function takeSnapshot (front, heapWorker) { Nit: Can we name this takeSnapshotAndCensus? Since eventually we will be sometimes doing them both together, and sometimes only taking censuses. Even better: have a takeSnapshot function, and then this function just calls that and then takeCensus. @@ +18,5 @@ > + > + let snapshotId = yield front.saveHeapSnapshot() > + dispatch({ type: actions.TAKE_SNAPSHOT_END, snapshot, snapshotId }); > + > + yield dispatch(takeCensus(heapWorker, snapshot)); Is yield needed here? Doesn't the middleware automatically resolve and dispatch promises? @@ +49,5 @@ > + // If snapshot has not been read into memory, do that now; > + // only need to do once per snapshot > + if (!snapshot.snapshotRead) { > + dispatch({ type: actions.READ_HEAP_SNAPSHOT, snapshot }); > + yield heapWorker.readHeapSnapshot(snapshot.snapshotId); Why not move the reading in with `takeSnapshot`? That way we can read it in after saving with no conditional checking, rather than checking a conditional every time we take a census. @@ +53,5 @@ > + yield heapWorker.readHeapSnapshot(snapshot.snapshotId); > + } > + > + // Make sure we're using the up-to-date breakdown > + let breakdown = getStore().breakdown; Nice. ::: devtools/client/memory/reducers/snapshots.js @@ +2,5 @@ > +const { getSnapshot, selectSnapshot } = require("../utils"); > +const DevToolsUtils = require("devtools/shared/DevToolsUtils"); > + > +function handleTakeSnapshotStart (state, { snapshot }) { > + // Auto select new snapshots immediately Nit: punctuate comments. @@ +49,5 @@ > + case actions.TAKE_CENSUS_END: > + return handleTakeCensusEnd(state, action); > + // Nothing to do for READ_HEAP_SNAPSHOT for now > + // since we can infer this state > + case actions.READ_HEAP_SNAPSHOT: This isn't clear to me. Why can we infer this state? If we can infer this state, why do we even have an action? ::: devtools/client/memory/utils.js @@ +18,5 @@ > + > +exports.getSnapshot = function getSnapshot (snapshots, snapshot) { > + let found = snapshots.find(s => s.id === snapshot.id); > + if (!found) { > + DevToolsUtils.reportException(`No matching snapshot found for ${snapshot.id}`); This returns undefined if not found? Pls add doc comment. Also doc comments to other functions here, please. ::: devtools/client/themes/memory.css @@ +126,5 @@ > + * Main panel > + */ > + > +#heap-view { > + flex: 1 1 auto; Nit: trailing white space.

Changed a few things -- consolidated properties of a snapshot with a single state property with enums, broke out the actions to start/stop events, and yeah this is what the promise middleware would give us, but this lets us add some assertions in there, do middle steps, and less magical (for myself, and I'd imagine new comers as well)

Comment on attachment 8675131 [details] [diff] [review] 1214872-memory-state.patch Review of attachment 8675131 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/memory/constants.js @@ +27,5 @@ > + * Various states a snapshot can be in. > + * An FSM describing snapshot states: > + * > + * SAVING -> SAVED -> READING -> READ <- <- <- SAVED_CENSUS > + * ↘ ↗ your arrows are fire ::: devtools/client/memory/reducers/snapshots.js @@ +62,5 @@ > + return handleReadSnapshotStart(snapshots, action); > + case actions.READ_SNAPSHOT_END: > + return handleReadSnapshotEnd(snapshots, action); > + case actions.SELECT_SNAPSHOT: > + return handleSelectSnapshot(snapshots, action); I just had this thought, sorry for not bringing it up earlier. I think we should do: const handlers = Object.create(null); handlers[actions.TAKE_SNAPSHOT_START] = function (snapshots, action) { ... }; // Etc... module.exports = function (snapshots=[], action { const handler = handlers[action]; if (handler) { return handler(snapshots, action); } else { // Should we log a warning here? return snapshots; } }; Which reads better and has a little bit less boilerplate involved.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #13) > Created attachment 8675131 [details] [diff] [review] > 1214872-memory-state.patch > > Changed a few things -- consolidated properties of a snapshot with a single > state property with enums, broke out the actions to start/stop events, and > yeah this is what the promise middleware would give us, but this lets us add > some assertions in there, do middle steps, and less magical (for myself, and > I'd imagine new comers as well) T_T I don't really think it's magical at all. It just does all this automatically, and is easy to understand (fire actions on start/stop/err), and additionally you never call `dispatch` inside a promise (so UI errors are never caught in promise exceptions). We should be using the promise middleware for everything eventually. It's fine to do this for now but I don't want to write start/start/error code every single time.

The other thing the promise middelware does which task doesn't (I think it could, not sure) is it assigns a `seqId` property for every async action. All start/stop/err actions for the same promise has the same seqId. I'm using this in the debugger, for example, when shutting down; I keep track of open async actions (by listening for actions with a seqId and status of "open") and if there are any open I wait for all of them to close. Every async action should have a seqId so we can generically do stuff with async work.

Tasks are just an abstraction over promises with generators. There's no reason we can't make it have the same behavior.

Right, Task.spawn returns a promise, so you can fully use it with the promise middleware. If you're going to make it have the same behavior, you're going to end up with the promise middelware. I use tasks all the time with it: https://github.com/jlongster/gecko-dev/blob/debugger-refactor-sources-4/devtools/client/debugger/content/actions/sources.js#L125 Lots of other ones in there too. My point is that we should construct async action creators that emit start/stop/error events with a specific action type, and that's exactly what the promise middleware does, and you just use Task.spawn.

(In reply to James Long (:jlongster) from comment #19) > Right, Task.spawn returns a promise, so you can fully use it with the > promise middleware. If you're going to make it have the same behavior, > you're going to end up with the promise middelware. I use tasks all the time > with it: > https://github.com/jlongster/gecko-dev/blob/debugger-refactor-sources-4/ > devtools/client/debugger/content/actions/sources.js#L125 Lots of other ones > in there too. > > My point is that we should construct async action creators that emit > start/stop/error events with a specific action type, and that's exactly what > the promise middleware does, and you just use Task.spawn. The difference here is we want every single promise yielded to dispatch, not just the start and end of the tasks overall promise. Still do-able, but not the same as just passing Task.spawn's promise to the promise middleware.

In that case, you should be calling other action creators that return a promise, and they will automatically dispatch actions, like: function saveSnapshot() { return { type: SAVE_SNAPSHOT, [PROMISE]: /* take the snapshot... */ } } function takeCensus() { return { type: TAKE_CENSUS, [PROMISE]: /* take the snapshot... */ } } function saveAndTakeCensus() { return function*(dispatch) { yield dispatch(saveSnapshot()); yield dispatch(takeCensus()); }) } The actions would look like this: { type: SAVE_SNAPSHOT, seqId: 1, status: "start" } { type: SAVE_SNAPSHOT, seqId: 1, status: "done" } { type: TAKE_CENSUS, seqId: 2, status: "start" } { type: TAKE_CENSUS, seqId: 2, status: "done" } The UI doesn't care about the whole process, but if it did you could return a promise action in `saveAndTakeCensus` as well and you'd get start/done actions for the whole process. I can see it being helpful sometimes to have the task middleware for something like `saveAndTakeCensus`, and I guess you're saying it would automatically dispatch every yielded value? (So you `yield` actions) In that case, that's totally fine, but if you need to track the process it should be turned in to a promise-based action. Here, we'd get updates (errors, etc) for each part of the process, which is usually fine.

Note that I don't really care what you do right now (more important to Ship It) but it's worth having these conversations.