Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Description • 9 years ago

Write some architectural overview docs and throw them in /devtools/docs/ where we have similar things for other tools.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 1 • 9 years ago

Important because its best to get this kind of thing in early so that the architecture doesn't devolve over time as new folks start contributing (and so they can effectively contribute).

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 2 • 9 years ago

Flagging both some people who are familiar with the tool's implementation and
some people who are less familiar so we can get a variety of viewpoints and
feedback.

If you don't want to review this documentation, feel free to cancel the review!

	Jim Blandy :jimb
	Comment 3 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

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

::: devtools/docs/memory-panel.md
@@ +1,4 @@
> +# Memory Tool Architecture
> +
> +This document provides an overview of the architecture of the "Memory" panel
> +within the Firefox Developer Tools.

This is kind of what the title says. Just say, in the next line, "The Firefox Developer Tools' memory tool is built..."

@@ +13,5 @@
> +2. The `HeapAnalysesWorker` orchestrates running specific analyses on specific
> +   snapshots and translating the results into something the frontend can simply
> +   and quickly render without performing any extra work. This is done in a
> +   worker thread, as the analyses can be rather heavy weight and slow. The
> +   `HeapAnalysesWorker` is consulted via the `HeapAnalysesClient`.

I'm inferring from this that HeapAnalysesWorker is a constructor that runs in the worker, and HeapAnalysesClient is the main-thread thing that talks to it. But I think it's possible to spell these things out, without really using any more text:

The `HeapAnalysesWorker' runs in a worker thread, performing analyses on snapshots and translating the results into something the frontend can render simply and quickly. The analyses can take some time to run, so doing them in a worker thread allows the interface to stay responsive. The HeapAnalysisClient provides the main thread's interface to the worker.

I think there are many points in the document where you could replace broad language with something more explicit --- and possibly even slightly shorter! But I haven't actually gone and tried it, so who knows.

@@ +18,5 @@
> +
> +3. Finally, the last element is the frontend that renders data received from the
> +   `HeapAnalysesWorker` to the DOM and translates user input into requests for
> +   new data from the `HeapAnalysesWorker`. The frontend is implemented with
> +   React and Redux.

So it talks directly to the HeapAnalysesWorker? Was I wrong about HeapAnalysesClient handling communication with the worker?

@@ +37,5 @@
> +`JS::ubi::Node` itself is very well documented in the `js/public/UbiNode.h`
> +header. I suggest you at least skim that documentation before continuing.
> +
> +A "heap snapshot" is the heap graph at some particular past instance in time. We
> +tend to call the serialized binary blob a "core dump". So, one would save a

I would choose a single term, and stick with it. There's no reason to create confusion by using the term "core dump" here or in the code. "Heap snapshot", or simply "snapshot" when the context is clear, are perfectly adequate terms.

@@ +40,5 @@
> +A "heap snapshot" is the heap graph at some particular past instance in time. We
> +tend to call the serialized binary blob a "core dump". So, one would save a
> +snapshot of the heap graph into a core dump on the Remote Debugger Server,
> +transfer the core dump to the client over the protocol, and then deserialize the
> +core dump into memory as a `HeapSnapshot` instance upon which we run analyses.

"into memory" doesn't add anything, as the serialized form is in memory too. It almost makes one think that one has brought the JavaScript heap back to life as JavaScript objects.

@@ +64,5 @@
> +   possible. If we are taking a snapshot to debug frequent out-of-memory errors,
> +   we don't want to trigger an OOM ourselves!
> +
> +To solve (1), we use the protobuf message format, and that's about all that
> +needs to be said about that. The message definitions themselves are in

"and that's about all that needs to be said about that" doesn't carry much meaning, which is pretty ironic for something talking about how little needs to be said!

@@ +122,5 @@
> +method on the [`HeapSnapshot`](dom/webidl/HeapSnapshot.webidl) webidl
> +interface. There is a small amount of glue code in Gecko for each analysis to
> +pass the `DeserializedNode`s that implement `JS::ubi::Node` from the C++
> +implementation of the webidl interface (`mozilla::devtools::HeapSnapshot`) to
> +the analysis.

I had to read this sentence three or four times before I kind of got what it was after. Possible suggestions:
- Use a separate paragraph to explain the problem this glue code addresses.
- Explicitly name the functions or methods that constitute this 'glue code'.

@@ +147,5 @@
> +The `HeapAnalysesWorker` orchestrates running specific analyses on snapshots and
> +transforming the results into something that can simply and quickly be rendered
> +by the frontend. We perform all analyses off the main thread because they are
> +rather heavy weight (can be multiples of seconds) and we don't want to degrade
> +UI responsiveness.

This whole paragraph is a repetition of what you said up in the architecture summary. Perhaps you could make the up-front text more focused and leave the details down here?

@@ +166,5 @@
> +### Testing the `HeapAnalysesWorker` and `HeapAnalysesClient`
> +
> +Tests for the `HeapAnalysesWorker` and `HeapAnalysesClient` reside in
> +`devtools/shared/heapsnapshot/test/**` and can be run with the usual `mach`
> +commands.

If you specify what kind of tests they are (xpcshell? mochi?) then you don't have to mention 'mach'.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 4 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

Additional review from Julian, since he is the first person to actually write a patch for the memory tool frontend other than me and Jordan :)

	Tom Tromey :tromey
	Comment 5 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

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

I read through this.  It seems clear to me.  I don't have anything to add beyond Jim's comments.

	Jordan Santell [:jsantell] [@jsantell]
	Comment 6 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

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

::: devtools/docs/memory-panel.md
@@ +13,5 @@
> +2. The `HeapAnalysesWorker` orchestrates running specific analyses on specific
> +   snapshots and translating the results into something the frontend can simply
> +   and quickly render without performing any extra work. This is done in a
> +   worker thread, as the analyses can be rather heavy weight and slow. The
> +   `HeapAnalysesWorker` is consulted via the `HeapAnalysesClient`.

+1 on Jim's replacement sentences

@@ +18,5 @@
> +
> +3. Finally, the last element is the frontend that renders data received from the
> +   `HeapAnalysesWorker` to the DOM and translates user input into requests for
> +   new data from the `HeapAnalysesWorker`. The frontend is implemented with
> +   React and Redux.

I'd stick with just using either the HAC or HAW -- only mention the other one once in terms of implementation, but for consistency, they should refer to the same thing.

@@ +37,5 @@
> +`JS::ubi::Node` itself is very well documented in the `js/public/UbiNode.h`
> +header. I suggest you at least skim that documentation before continuing.
> +
> +A "heap snapshot" is the heap graph at some particular past instance in time. We
> +tend to call the serialized binary blob a "core dump". So, one would save a

Agreed, I never heard of the word "core dump" before, I think, in this context

@@ +147,5 @@
> +The `HeapAnalysesWorker` orchestrates running specific analyses on snapshots and
> +transforming the results into something that can simply and quickly be rendered
> +by the frontend. We perform all analyses off the main thread because they are
> +rather heavy weight (can be multiples of seconds) and we don't want to degrade
> +UI responsiveness.

I would maybe emphasize how important/great that all analyses are occuring off the main thread, and that continuing forward, this should continue being the case, with any kind of processing done OTMT

@@ +199,5 @@
> +
> +Impurity within the frontend is confined to the tasks that are creating and
> +dispatching actions. All communication with the outside world (such as the
> +`HeapAnalysesWorker`, the Remote Debugger Server, or the file system) is
> +restricted to within these tasks.

I would mention that there are simple actions, generator actions (and how they work), and then composite actions which are comprised of many actions; so that the actions used in app.js are mostly these composite, long-named action creators.

@@ +209,5 @@
> +Unit tests for actions, reducers, and state changes are in
> +`devtools/client/memory/test/unit/*`.
> +
> +Holistic integration tests for the frontend and the whole memory tool are in
> +`devtools/client/memory/test/browser/*`.

Maybe worth mentioning the scope of each test -- xpcshell tests test actions, or state changes, etc., and mochitests are for smoke screening UI clicks and rendering (you touched on this a bit)

@@ +211,5 @@
> +
> +Holistic integration tests for the frontend and the whole memory tool are in
> +`devtools/client/memory/test/browser/*`.
> +
> +All tests can be run with the usual `mach` commands.

Link to mach docs

	Victor Porof [:vporof][:vp]
	Comment 7 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

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

::: devtools/docs/memory-panel.md
@@ +13,5 @@
> +2. The `HeapAnalysesWorker` orchestrates running specific analyses on specific
> +   snapshots and translating the results into something the frontend can simply
> +   and quickly render without performing any extra work. This is done in a
> +   worker thread, as the analyses can be rather heavy weight and slow. The
> +   `HeapAnalysesWorker` is consulted via the `HeapAnalysesClient`.

+1

@@ +18,5 @@
> +
> +3. Finally, the last element is the frontend that renders data received from the
> +   `HeapAnalysesWorker` to the DOM and translates user input into requests for
> +   new data from the `HeapAnalysesWorker`. The frontend is implemented with
> +   React and Redux.

Do we really care what the specific implementation is? I'm pretty sure that if somebody opens the code it'll be immediately obvious that it uses redux and react. However, these two technologies are not crucial to the entire thing.

@@ +37,5 @@
> +`JS::ubi::Node` itself is very well documented in the `js/public/UbiNode.h`
> +header. I suggest you at least skim that documentation before continuing.
> +
> +A "heap snapshot" is the heap graph at some particular past instance in time. We
> +tend to call the serialized binary blob a "core dump". So, one would save a

I like "core dump", however it raises certain questions. Dump of what exactly, and which core? but yeah we should stick to a single term.

@@ +81,5 @@
> +graph in snapshots, but that work is deemed Very Important and Very High
> +Priority.
> +
> +Finally, (3) imposes upon us that we do not build the core dump blob in memory,
> +but instead stream it out to disk as generate it.

"as it's being generated" is probably more grammatically correct.

However, streaming something to disk isn't a guarantee that it's actually being streamed to disk only, when it comes to memory-mapped files for example. I understand what the intention is, but I wonder if we can be clearer about what this actually does. If I'm reading too much into this, please ignore :P

@@ +187,5 @@
> +React components should be pure functions from their props to the rendered
> +(virtual) DOM. The should be observably pure in that they do not mutate their
> +inputs (the props) and given the same props should always return the same DOM
> +structure. The internal implementation need not be pure (and most often isn't!),
> +so long as it doesn't break the observable purity, then all is gravy. For

mm gravy

@@ +194,5 @@
> +will always be the same given the same inputs.
> +
> +Redux reducers should also be observably pure. Given the same `(currentState,
> +action)` pair, they should always return the same new state. The current state
> +should not be mutated at all.

Do we really have to mention all of this? This isn't an intro to redux, but a readme about the memory tool.

	Julian Descottes [:jdescottes]
	Comment 8 • 9 years ago

Comment on attachment 8705855 [details] [diff] [review]
Add an architectural overview of the memory tool

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

That a very nice read for a newcomer to the platform! Very helpful and interesting.

::: devtools/docs/memory-panel.md
@@ +31,5 @@
> +release Firefox for Android server) is a no-op. As we add new analyses, we can
> +run them on snapshots taken on old servers no problem. The only requirement is
> +that changes to the snapshot format itself remain backwards compatible.
> +
> +## `JS::ubi::Node`, Heap Snapshots, and Heap Analyses

Title was a bit frightening to me. How about only "`JS::ubi::Node`", to mirror the introduction.

@@ +37,5 @@
> +`JS::ubi::Node` itself is very well documented in the `js/public/UbiNode.h`
> +header. I suggest you at least skim that documentation before continuing.
> +
> +A "heap snapshot" is the heap graph at some particular past instance in time. We
> +tend to call the serialized binary blob a "core dump". So, one would save a

I agree "core" is confusing here. Doesn't "serialized heap snapshot" usually translate to "heap dump". However if "core dump" is heavily referenced in other documents, maybe simply refer to "serialized snapshot" to avoid confusion.

@@ +73,5 @@
> +
> +For (2), we rely on SpiderMonkey's GC rooting hazard static analysis and the
> +`AutoCheckCannotGC` dynamic analysis to ensure that neither JS nor GC runs and
> +modifies objects or moves them from one address in memory to another. There is
> +no equivalent suppression and static analysis technique for the cycle collector,

Maybe add a link to cycle collector documentation (https://developer.mozilla.org/en/docs/Interfacing_with_the_XPCOM_cycle_collector ?)

@@ +129,5 @@
> +[`HeapSnapshot`](dom/webidl/HeapSnapshot.webidl) webidl interface.
> +
> +### Testing `JS::ubi::Node`, Snapshots, and Analyses
> +
> +The majority of the tests reside within `devtools/shared/heapsnapshot/test/**`.

`devtools/shared/heapsnapshot/test/**` -> `devtools/shared/heapsnapshot/tests/**`

@@ +141,5 @@
> +and `js/src/jsapi-tests/testUbiNode.cpp`. See
> +https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Automated_JavaScript_Tests#Running_jit-tests
> +for running the JIT tests.
> +
> +## `HeapAnalysesWorker` and `HeapAnalysesClient`

Same comment as for the previous title, "`HeapAnalysesWorker`" as title would better mirror the sections from the introduction.

@@ +155,5 @@
> +
> +The `HeapAnalysesWorker` doesn't actually do much itself; mostly just shuffling
> +data and transforming it from one representation to another or calling utility
> +functions that do those things. Most of these are implemented as traversals of
> +the resulting census or dominator trees.

Here you could mention that the worker uses ThreadSafeChromeUtils instead of "utility functions that do those things". It is mentioned in the previous section, but I think it's important to highlight how each layer talks to the other.

@@ +165,5 @@
> +
> +### Testing the `HeapAnalysesWorker` and `HeapAnalysesClient`
> +
> +Tests for the `HeapAnalysesWorker` and `HeapAnalysesClient` reside in
> +`devtools/shared/heapsnapshot/test/**` and can be run with the usual `mach`

`devtools/shared/heapsnapshot/test/**` -> `devtools/shared/heapsnapshot/tests/**`

@@ +194,5 @@
> +will always be the same given the same inputs.
> +
> +Redux reducers should also be observably pure. Given the same `(currentState,
> +action)` pair, they should always return the same new state. The current state
> +should not be mutated at all.

+1 IMO, all that is mandatory about React/Redux are the documentation links and folder descriptions you provided in the beginning. Then focus on how the React/Redux layer communicates with rest of the world (which is done right after).

@@ +203,5 @@
> +restricted to within these tasks.
> +
> +### Testing the Frontend
> +
> +Unit tests for React components are in `devtools/client/memory/test/chrome/*`.

Will there be any in the future? I don't have this folder.

	Julian Descottes [:jdescottes]
	Comment 9 • 9 years ago

Replying to myself :

> +1 IMO, all that is mandatory about React/Redux are the documentation links and folder 
> descriptions you provided in the beginning. Then focus on how the React/Redux layer 
> communicates with rest of the world (which is done right after).

To be clear I meant +1 to Victor's comment "Do we really have to mention all of this". But my mouse clicks betrayed me.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 10 • 9 years ago

Thanks for the feedback, everyone!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 12 • 9 years ago

Just documentation, so no try run. Commit marked as DONTBUILD.

	Pulsebot
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/2e6226f35e26

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2e6226f35e26

	Mayur Patil
	Comment 15 • 8 years ago

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected