Closed Bug 1252971 Opened 4 years ago Closed 4 years ago

Memory panel docs update

Categories

(DevTools :: Memory, defect, P1)

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

Details

Attachments

(1 file, 1 obsolete file)

After going through a deep dive into the memory tool I've updated
and clarified a few things in the docs that I found confusing.
Attached patch Memory panel docs update (obsolete) — Splinter Review
After going through a deep dive into the memory tool I've updated
and clarified a few things in the docs that I found confusing.
Attachment #8725806 - Attachment description: Memory panel docs update -r=fitzgen → Memory panel docs update
Attachment #8725806 - Flags: review?(nfitzgerald)
Comment on attachment 8725806 [details] [diff] [review]
Memory panel docs update

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

Thanks! r=me with comments below addressed

::: devtools/docs/memory-panel.md
@@ +1,5 @@
>  # Memory Tool Architecture
>  
>  The memory tool is built of three main elements:
>  
> +1. The live heap graph exists as highly optimized C++ code. In order to get a

This first sentence rubs me the wrong way. Abstractly, the heap graph is the result of considering objects/strings/etc in memory as vertices in a graph and the references from one to another as a directed edge in that graph. Concretely, this is implemented in C++ by the allocator and garbage collector.

@@ +2,5 @@
>  
>  The memory tool is built of three main elements:
>  
> +1. The live heap graph exists as highly optimized C++ code. In order to get a
> +   clear picture of what is going in the heap graph a specialized interface is

"... what is going in the heap graph <comma> a specialized interface ..."

@@ +7,5 @@
> +   created to represent its state. The `JS::ubi::Node` is the basis for this
> +   representation. This interface can be created from the live heap graph, or a
> +   serialized, offline snapshot from a previous moment in time. Our various heap
> +   analyses (census, dominator trees, shortest paths, etc) run on top of
> +   `JS::ubi::Node` graphs. The `ubi` in the name stands for ubiquitous and is a

Put "ubiquitous" in quotes.

@@ +8,5 @@
> +   representation. This interface can be created from the live heap graph, or a
> +   serialized, offline snapshot from a previous moment in time. Our various heap
> +   analyses (census, dominator trees, shortest paths, etc) run on top of
> +   `JS::ubi::Node` graphs. The `ubi` in the name stands for ubiquitous and is a
> +   namespace for all of the memory analysis C++ code.

"... and provides a namespace for memory analyses in C++ code."

@@ +39,5 @@
>  
>  A "heap snapshot" is a representation of the heap graph at some particular past
>  instance in time.
>  
> +A "heap analysis" is any number of algorithms that runs on a `JS::ubi::Node`

"Analysis" is singular, while "algorithms" is plural, and "runs" works with the singular form, not the plural form. The change to this sentence should be reverted, or further changed to:

  "Heap analyses" are algorithms that run on a `JS::ubi::Node` heap graph.

Personally, I prefer the existing sentence.

@@ +203,5 @@
> +state machine describing the snapshot states. Any of these states may go to the
> +ERROR state, from which they can never leave.
> +
> +```
> +SAVING -> SAVED -> READING -> READ     SAVED_CENSUS

Might as well take a moment to use "→" instead of "->" here, since we are already using unicode for the other arrows.
Attachment #8725806 - Flags: review?(nfitzgerald) → review+
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Has STR: --- → irrelevant
Priority: -- → P1
Attachment #8725855 - Flags: review+
Attachment #8725806 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7339ce7f9e5d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.