Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1598393004:
[Docs] Move heap profiler docs 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@docs Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Docs] Move heap profiler docs into repository
This moves the page that was at [1] to in-tree Markdown documentation
and updates the information on there too.
This is the second 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/heap-profiling-with-memory-infra
TBR=oysteine@chromium.org
Committed: https://crrev.com/faa17b2a23b7def59ed19c29875379b10d6dc2da
Cr-Commit-Position: refs/heads/master@{#370105}
Patch Set 1 #
Total comments: 28
Patch Set 2 : Address petrcermak comments #Patch Set 3 : Address petrcermak comment #
Depends on Patchset: Messages
Total messages: 21 (10 generated)
ruuda@google.com changed reviewers: + primiano@chromium.org
Please take a look at this documentation move and update. Images are in Google Drive for now. They have been crushed. (It is unfortunate that Gitiles renders images at 100% width, it makes them draw far too much attention and the font size is horribly inconsistent, but there appears to be no syntax to set the dimensions.) cc Petr "UI" Cermak
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
Looks good overall. Just a couple of comments. Thanks, Petr https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... File components/tracing/docs/heap_profiler.md (right): https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:11: * Start Chrome with the `--enable-heap-profiling` switch. This will make Chrome Shouldn't this be an enumeration (1, 2, 3, ...)? https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:17: information in the heap profiler. maybe say "... in the heap profiler pseudo stack traces."? https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:29: malloc is currently in the prototype stage. s/is/are/ (or s/dumps/dumping/ on the previous line). https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:57: events, shown in the flame chart in timeline view. At every point in time these nit: This makes it sounds like Chrome *only* records the trace events shown in the flame chart, which is not the case. Not sure how to clarify that without complicating things though. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:64: only supported for PartitionAlloc. The second sentence seems irrelevant at the moment since we don't support malloc yet. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:68: size concerns. A development build has full type information. nit: I don't like the indefinite article in front of "development build". I'd say either "Every development build has", "All development builds have", or simply "Development builds have". https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:83: The pseudo stack of trace events translates in the tree of ƒ nodes below. Of the nit: s/translates in/is translated into/ https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:91: there. Of the 1.9 MiB, 371 KiB is spent on `ImmutableStylePropertySet`s, and nit: s/is/was/ (consistency with "were allocated" in the previous sentence) https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:92: 238 KiB is spent on `StringImpl`s. ditto https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:104: then with the control key select a heavy memory dump earlier in time. Below is a nit: Since you can also select a dump in the future, I'd say "select another heavy memory dump". https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:104: then with the control key select a heavy memory dump earlier in time. Below is a people tend to forget things, so it might be worth reminding the reader that heavy dumps are purple: "select another heavy memory dump (purple)" https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:106: MiB was allocated when parsing the documents in all those iframes, almost a probably s/was/were/ https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:107: megabyte of which was due to JavaScript. (Note that this is memory on the nit: s/memory on the/memory allocated by/
Thanks for your comments, PTAL. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... File components/tracing/docs/heap_profiler.md (right): https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:11: * Start Chrome with the `--enable-heap-profiling` switch. This will make Chrome On 2016/01/19 10:13:55, petrcermak wrote: > Shouldn't this be an enumeration (1, 2, 3, ...)? Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:17: information in the heap profiler. On 2016/01/19 10:13:55, petrcermak wrote: > maybe say "... in the heap profiler pseudo stack traces."? Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:29: malloc is currently in the prototype stage. On 2016/01/19 10:13:54, petrcermak wrote: > s/is/are/ (or s/dumps/dumping/ on the previous line). Fixed. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:57: events, shown in the flame chart in timeline view. At every point in time these On 2016/01/19 10:13:55, petrcermak wrote: > nit: This makes it sounds like Chrome *only* records the trace events shown in > the flame chart, which is not the case. Not sure how to clarify that without > complicating things though. Replaced “shown” with “most of which appear”. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:64: only supported for PartitionAlloc. On 2016/01/19 10:13:55, petrcermak wrote: > The second sentence seems irrelevant at the moment since we don't support malloc > yet. I know, but the screenshots show the icon for malloc too, so this is to clarify that people are not doing it wrong when they don’t get dumps for malloc if they go and try this out. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:68: size concerns. A development build has full type information. On 2016/01/19 10:13:54, petrcermak wrote: > nit: I don't like the indefinite article in front of "development build". I'd > say either "Every development build has", "All development builds have", or > simply "Development builds have". Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:83: The pseudo stack of trace events translates in the tree of ƒ nodes below. Of the On 2016/01/19 10:13:54, petrcermak wrote: > nit: s/translates in/is translated into/ Changed to “corresponds to”. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:91: there. Of the 1.9 MiB, 371 KiB is spent on `ImmutableStylePropertySet`s, and On 2016/01/19 10:13:55, petrcermak wrote: > nit: s/is/was/ (consistency with "were allocated" in the previous sentence) Done. You are sharp today :) https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:92: 238 KiB is spent on `StringImpl`s. On 2016/01/19 10:13:54, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:104: then with the control key select a heavy memory dump earlier in time. Below is a On 2016/01/19 10:13:55, petrcermak wrote: > nit: Since you can also select a dump in the future, I'd say "select another > heavy memory dump". Hah, actually I stressed that you have to select the latest-in-time dump first, because when I tried it looked like memory _decreased_ which didn’t make sense at all until I found out that it depends on the order in which you select them ... https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:104: then with the control key select a heavy memory dump earlier in time. Below is a On 2016/01/19 10:13:54, petrcermak wrote: > people tend to forget things, so it might be worth reminding the reader that > heavy dumps are purple: > > "select another heavy memory dump (purple)" Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:106: MiB was allocated when parsing the documents in all those iframes, almost a On 2016/01/19 10:13:54, petrcermak wrote: > probably s/was/were/ Done. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:107: megabyte of which was due to JavaScript. (Note that this is memory on the On 2016/01/19 10:13:54, petrcermak wrote: > nit: s/memory on the/memory allocated by/ Done.
LGTM with one more comment to resolve. Thanks, Petr https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... File components/tracing/docs/heap_profiler.md (right): https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:64: only supported for PartitionAlloc. On 2016/01/19 10:44:48, Ruud van Asseldonk wrote: > On 2016/01/19 10:13:55, petrcermak wrote: > > The second sentence seems irrelevant at the moment since we don't support > malloc > > yet. > > I know, but the screenshots show the icon for malloc too, so this is to clarify > that people are not doing it wrong when they don’t get dumps for malloc if they > go and try this out. Do you mean the heap dump icon? As far as I understand, that's not going to be shown for malloc. Anyway, that's not what this sentence refers to. In fact, I think that including it at the moment might be confusing for the readers because this suggests that they can somehow get the malloc breakdown without types. I'd re-add it as soon as malloc support lands. https://codereview.chromium.org/1598393004/diff/1/components/tracing/docs/hea... components/tracing/docs/heap_profiler.md:104: then with the control key select a heavy memory dump earlier in time. Below is a On 2016/01/19 10:44:48, Ruud van Asseldonk wrote: > On 2016/01/19 10:13:55, petrcermak wrote: > > nit: Since you can also select a dump in the future, I'd say "select another > > heavy memory dump". > > Hah, actually I stressed that you have to select the latest-in-time dump first, > because when I tried it looked like memory _decreased_ which didn’t make sense > at all until I found out that it depends on the order in which you select them > ... Oh, really? Then it's a bug in the memory UI. It shouldn't matter in which order you select the memory dumps :-\
On 2016/01/19 11:11:30, petrcermak wrote: > Do you mean the heap dump icon? As far as I understand, that's not going to be > shown for malloc. I removed the sentences. It is not going to show when you run Chrome master, but it does show in the screenshot included. > Oh, really? Then it's a bug in the memory UI. It shouldn't matter in which order > you select the memory dumps :-\ Filed https://github.com/catapult-project/catapult/issues/1899.
lgtm
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1598393004/#ps40001 (title: "Address petrcermak comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1598393004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1598393004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [Docs] Move heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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 heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1598393004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1598393004/40001
Message was sent while issue was closed.
Description was changed from ========== [Docs] Move heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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 heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Docs] Move heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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 heap profiler docs into repository This moves the page that was at [1] to in-tree Markdown documentation and updates the information on there too. This is the second 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/faa17b2a23b7def59ed19c29875379b10d6dc2da Cr-Commit-Position: refs/heads/master@{#370105} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/faa17b2a23b7def59ed19c29875379b10d6dc2da Cr-Commit-Position: refs/heads/master@{#370105} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
