Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1601773002:
[Docs] Move MemoryInfra overview into repository (Closed)
|
Created:
4 years, 11 months ago by Ruud van Asseldonk Modified:
4 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, petrcermak Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Docs] Move MemoryInfra overview into repository
This moves the page that was at [1] to in-tree Markdown documentation.
At the same time this port makes the document more concise and outdated
parts are updated. Screenshots have been updated as well to match the
latest version of Catapult.
This is the first part of an incremental effort to move all MemoryInfra
documentation (and eventually all tracing documentation) off of Google
Sites and into the repository.
[1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-profiling-tool/memory
TBR=oysteine@chromium.org
Committed: https://crrev.com/cefbd3ccc78cbd6a848f0b342477a5eae6560ec5
Cr-Commit-Position: refs/heads/master@{#370093}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address primiano comments #Patch Set 3 : Fix images and iframes #Patch Set 4 : Fix multiparagraph list (due to images) #
Total comments: 1
Patch Set 5 : <ul> to <ol> #Messages
Total messages: 18 (8 generated)
ruuda@google.com changed reviewers: + primiano@chromium.org
Please take a look at this documentation move. I didn’t copy the document verbatim, instead I updated it a bit, I left out the irrelevant parts, and I put the part that other developers are most likely interested in first. I crushed the pngs and they are all below the 256KiB limit, but I’ll remove them before landing if we have a hosting solution. (See https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/irLAQ8f8uGk.) cc Petr "UI" Cermak
Many thanks for doing this. Just few comments. Also please let's figure out an agreement on chromium-dev as regards images before landing this. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... File components/tracing/docs/memory_infra.md (right): https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:54: * **Total Resident**: (undocumented). I'd replace all these (undocumented) with TODO: document https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:60: **Columns in black** reflect the amount of _virtual memory_ requested by various _virtual memory_ : not really: these provide the best estimation they can provide for resident memory requested by the various subsystems. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:82: above information. This memory would not be used if tracing were not enabled. Add: and is properly discounted from both malloc and the resident (Blue) columns https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:90: To illustrate the difference between physical and virtual memory, consider the Remove this section about physical and virtual. I'd like people to not thing about that. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:152: * [Architectural](https://drive.google.com/a/google.com/embeddedfolderview?id=0B3KuDeqD-lVJfmp0cW1VcE5XVWNxZndxelV5T19kT2NFSndYZlNFbkFpc3pSa2VDN0hlMm8) Hmm I'd like you kept the <iframes> here, as reduce one level of indirection.
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
Thanks for taking care of this. Looks good overall, especially the high-resolution screenshots. One question: Is it intentional that none of the FAQs is included? Petr
On 2016/01/18 15:58:34, petrcermak wrote: > Thanks for taking care of this. Looks good overall, especially the > high-resolution screenshots. > > One question: Is it intentional that none of the FAQs is included? > > Petr Yes, see below. > Which platforms are currently supported? It already says at the top: all of them. > How frequently do the memory snapshots happen? Since I’ve been here I have not seen anybody ask this question, it is quite obvious from the timeline view, and “in the near future” means “indefinitely three months from now” so it is not helpful. > I want my subsystem / allocator to be reported in this memory profiling infrastructure. How do I do? Converted this into the “Development” section. > What is the difference between the blue used memory columns (PSS, Resident size, swap) and the allocator columns (V8, PartAlloc, BlinkGC, ...) This is the new “Columns” section. Code sample removed at Primiano’s request. > Uh, this is confusing. Can you just tell the proportion of used memory for each subsystem? Not a helpful answer, might as well leave it out. > What are all those other memory-related tracks I see in the trace tracks (DiscardableMemoryUsage, unused_bytes, GPU TransferBuffer)? No longer there? > All this aggregated data is cool, but I want more details. > But I really want detailed call sites for each allocation. The heap profiler is a thing now :D Docs to come in a follow-up CL. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... File components/tracing/docs/memory_infra.md (right): https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:54: * **Total Resident**: (undocumented). On 2016/01/18 15:45:28, Primiano Tucci wrote: > I'd replace all these (undocumented) with TODO: document Done. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:60: **Columns in black** reflect the amount of _virtual memory_ requested by various On 2016/01/18 15:45:28, Primiano Tucci wrote: > _virtual memory_ : not really: these provide the best estimation they can > provide for resident memory requested by the various subsystems. Fixed. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:82: above information. This memory would not be used if tracing were not enabled. On 2016/01/18 15:45:28, Primiano Tucci wrote: > Add: and is properly discounted from both malloc and the resident (Blue) columns Done. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:90: To illustrate the difference between physical and virtual memory, consider the On 2016/01/18 15:45:28, Primiano Tucci wrote: > Remove this section about physical and virtual. I'd like people to not thing > about that. Done. https://codereview.chromium.org/1601773002/diff/1/components/tracing/docs/mem... components/tracing/docs/memory_infra.md:152: * [Architectural](https://drive.google.com/a/google.com/embeddedfolderview?id=0B3KuDeqD-lVJfmp0cW1VcE5XVWNxZndxelV5T19kT2NFSndYZlNFbkFpc3pSa2VDN0hlMm8) On 2016/01/18 15:45:28, Primiano Tucci wrote: > Hmm I'd like you kept the <iframes> here, as reduce one level of indirection. Done, so the one person who actually looks at these can reach them one click faster ;)
I uploaded the images to drive for now. Any further comments?
LGTM with one more comment from my side. Thanks, Petr https://codereview.chromium.org/1601773002/diff/60001/components/tracing/docs... File components/tracing/docs/memory_infra.md (right): https://codereview.chromium.org/1601773002/diff/60001/components/tracing/docs... components/tracing/docs/memory_infra.md:12: * Get a bleeding-edge or tip-of-tree build of Chrome. Shouldn't this be an enumeration (1, 2, 3, ...)?
LGTM thanks
Description was changed from ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... ========== to ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... TBR=oysteine@chromium.org ==========
ruuda@google.com changed reviewers: + oysteine@chromium.org
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1601773002/#ps80001 (title: "<ul> to <ol>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601773002/80001
Message was sent while issue was closed.
Description was changed from ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... TBR=oysteine@chromium.org ========== to ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... TBR=oysteine@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... TBR=oysteine@chromium.org ========== to ========== [Docs] Move MemoryInfra overview into repository This moves the page that was at [1] to in-tree Markdown documentation. At the same time this port makes the document more concise and outdated parts are updated. Screenshots have been updated as well to match the latest version of Catapult. This is the first part of an incremental effort to move all MemoryInfra documentation (and eventually all tracing documentation) off of Google Sites and into the repository. [1]: https://sites.google.com/a/chromium.org/dev/developers/how-tos/trace-event-pr... TBR=oysteine@chromium.org Committed: https://crrev.com/cefbd3ccc78cbd6a848f0b342477a5eae6560ec5 Cr-Commit-Position: refs/heads/master@{#370093} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cefbd3ccc78cbd6a848f0b342477a5eae6560ec5 Cr-Commit-Position: refs/heads/master@{#370093} |
