Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949353004:
Update documentation of memory-infra to describe updates to heap profiling. (Closed)
DescriptionUpdate documentation of memory-infra to describe updates to heap profiling.
Native heap profiling can now be run on official builds by setting a
chrome://flag.
BUG=
Review-Url: https://codereview.chromium.org/2949353004
Cr-Commit-Position: refs/heads/master@{#481747}
Committed: https://chromium.googlesource.com/chromium/src/+/ffe164964fbdf5b1a7245309b2887d45b96eabf0
Patch Set 1 #
Total comments: 2
Patch Set 2 : comments form ajwong. #
Total comments: 6
Patch Set 3 : Explciitly specify official builds for macOS/Windows. #Patch Set 4 : Update comments. #Patch Set 5 : comments from dskiba. #Messages
Total messages: 22 (6 generated)
erikchen@chromium.org changed reviewers: + ajwong@chromium.org
ajwong: Please review.
https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... File docs/memory-infra/heap_profiler.md (right): https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... docs/memory-infra/heap_profiler.md:62: On Linux / Android, you need to build Chromium with special flags to use native You don't need this on Linux... That just turns on the pseudo-backtrace stuff with frame pointers AFAIK.
https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... File docs/memory-infra/heap_profiler.md (right): https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... docs/memory-infra/heap_profiler.md:62: On Linux / Android, you need to build Chromium with special flags to use native On 2017/06/22 21:25:48, awong wrote: > You don't need this on Linux... That just turns on the pseudo-backtrace stuff > with frame pointers AFAIK. ah, but we don't have symbolization for Linux...Updated documentation.
erikchen@chromium.org changed reviewers: + dskiba@chromium.org
+ dskiba to confirm that symbol_level=1 is necessary and sufficient for symbolization on Linux and Android.
On 2017/06/22 21:35:40, erikchen wrote: > https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... > File docs/memory-infra/heap_profiler.md (right): > > https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... > docs/memory-infra/heap_profiler.md:62: On Linux / Android, you need to build > Chromium with special flags to use native > On 2017/06/22 21:25:48, awong wrote: > > You don't need this on Linux... That just turns on the pseudo-backtrace stuff > > with frame pointers AFAIK. > > ah, but we don't have symbolization for Linux...Updated documentation. Isn't that symbolization problem true for all platforms?
On 2017/06/22 21:36:30, awong wrote: > On 2017/06/22 21:35:40, erikchen wrote: > > > https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... > > File docs/memory-infra/heap_profiler.md (right): > > > > > https://codereview.chromium.org/2949353004/diff/1/docs/memory-infra/heap_prof... > > docs/memory-infra/heap_profiler.md:62: On Linux / Android, you need to build > > Chromium with special flags to use native > > On 2017/06/22 21:25:48, awong wrote: > > > You don't need this on Linux... That just turns on the pseudo-backtrace > stuff > > > with frame pointers AFAIK. > > > > ah, but we don't have symbolization for Linux...Updated documentation. > > Isn't that symbolization problem true for all platforms? No. The symbolization script *just works* on macOS and Windows.
https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... File docs/memory-infra/heap_profiler.md (right): https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:41: It's also possible to use heap profiling with native, symbolized stack traces. Add a note that it's for official builds. https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:69: symbol_level = 1 Move the `symbol_level` change into a note or section that says its needed for all custom builds.
Regarding symbol_level = 1 on Linux - let me double check (on Android it's fine). https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... File docs/memory-infra/heap_profiler.md (left): https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:48: enable_profiling = true Why are you removing this? Without enable_profiling binaries won't have frame pointers.
https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... File docs/memory-infra/heap_profiler.md (left): https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:48: enable_profiling = true On 2017/06/22 21:44:32, DmitrySkiba wrote: > Why are you removing this? Without enable_profiling binaries won't have frame > pointers. See very long discussion on https://bugs.chromium.org/p/chromium/issues/detail?id=706654. Unwinding with frame pointers is fundamentally flawed: https://bugs.chromium.org/p/chromium/issues/detail?id=706654#c6 And it's unnecessary now to obtain stack traces on Desktop, so there's no point in specifying it. https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... File docs/memory-infra/heap_profiler.md (right): https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:41: It's also possible to use heap profiling with native, symbolized stack traces. On 2017/06/22 21:43:51, awong wrote: > Add a note that it's for official builds. Renamed the sections below to distinguish between Chrome/Chromium builds. https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... docs/memory-infra/heap_profiler.md:69: symbol_level = 1 On 2017/06/22 21:43:51, awong wrote: > Move the `symbol_level` change into a note or section that says its needed for > all custom builds. Add comments for macOS / Windows w.r.t. symbol_level
LGTM from my side. On the enable_profiling bit, will let you and dskiba figure that out. One wonders if the trace sharding Erik did sped up android enough to not need the frame pointers either?
On 2017/06/22 21:55:56, erikchen wrote: > https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... > File docs/memory-infra/heap_profiler.md (left): > > https://codereview.chromium.org/2949353004/diff/20001/docs/memory-infra/heap_... > docs/memory-infra/heap_profiler.md:48: enable_profiling = true > On 2017/06/22 21:44:32, DmitrySkiba wrote: > > Why are you removing this? Without enable_profiling binaries won't have frame > > pointers. > > See very long discussion on > https://bugs.chromium.org/p/chromium/issues/detail?id=706654. > > Unwinding with frame pointers is fundamentally flawed: > https://bugs.chromium.org/p/chromium/issues/detail?id=706654#c6 > > And it's unnecessary now to obtain stack traces on Desktop, so there's no point > in specifying it. So, there are two data points: 1. Unwinding with frame pointers is significantly faster on Linux. Like 430 times faster, see https://codereview.chromium.org/1879073002#msg12 2. We scan stack on Linux if we fail to unwind, that repairs some of the traces. While I agree that unwinding with frame pointers on Linux, where system libraries don't have frame pointers is not ideal and sometimes gives incomplete results, from the practical perspective I feel that using frame pointers is the right choice. If someone who regularly uses native heap profiling on Linux steps up and LGTMs this change, then I'm fine with it. Until then I think we should leave it as is.
> > So, there are two data points: > > 1. Unwinding with frame pointers is significantly faster on Linux. Like 430 > times faster, see https://codereview.chromium.org/1879073002#msg12 > > 2. We scan stack on Linux if we fail to unwind, that repairs some of the traces. > > While I agree that unwinding with frame pointers on Linux, where system > libraries don't have frame pointers is not ideal and sometimes gives incomplete > results, from the practical perspective I feel that using frame pointers is the > right choice. > > If someone who regularly uses native heap profiling on Linux steps up and LGTMs > this change, then I'm fine with it. Until then I think we should leave it as is. I'm not interested in discussing this point further. We've already had this discussion multiple times. I've updated the section to read: """ macOS / Windows symbol_level = 1 Linux enable_profiling = true symbol_level = 1 """
On 2017/06/22 22:31:37, erikchen wrote: > > > > So, there are two data points: > > > > 1. Unwinding with frame pointers is significantly faster on Linux. Like 430 > > times faster, see https://codereview.chromium.org/1879073002#msg12 > > > > 2. We scan stack on Linux if we fail to unwind, that repairs some of the > traces. > > > > While I agree that unwinding with frame pointers on Linux, where system > > libraries don't have frame pointers is not ideal and sometimes gives > incomplete > > results, from the practical perspective I feel that using frame pointers is > the > > right choice. > > > > If someone who regularly uses native heap profiling on Linux steps up and > LGTMs > > this change, then I'm fine with it. Until then I think we should leave it as > is. > > I'm not interested in discussing this point further. We've already had this > discussion multiple times. I've updated the section to read: > > """ > macOS / Windows > > symbol_level = 1 > > Linux > > enable_profiling = true > symbol_level = 1 > """ I checked Linux and native heap profiler works with these GN flags.
On 2017/06/23 00:26:23, DmitrySkiba wrote: > On 2017/06/22 22:31:37, erikchen wrote: > > > > > > So, there are two data points: > > > > > > 1. Unwinding with frame pointers is significantly faster on Linux. Like 430 > > > times faster, see https://codereview.chromium.org/1879073002#msg12 > > > > > > 2. We scan stack on Linux if we fail to unwind, that repairs some of the > > traces. > > > > > > While I agree that unwinding with frame pointers on Linux, where system > > > libraries don't have frame pointers is not ideal and sometimes gives > > incomplete > > > results, from the practical perspective I feel that using frame pointers is > > the > > > right choice. > > > > > > If someone who regularly uses native heap profiling on Linux steps up and > > LGTMs > > > this change, then I'm fine with it. Until then I think we should leave it as > > is. > > > > I'm not interested in discussing this point further. We've already had this > > discussion multiple times. I've updated the section to read: > > > > """ > > macOS / Windows > > > > > > symbol_level = 1 > > > > > > Linux > > > > > > enable_profiling = true > > > symbol_level = 1 > > """ > > I checked Linux and native heap profiler works with these GN flags. thanks. landing.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org Link to the patchset: https://codereview.chromium.org/2949353004/#ps80001 (title: "comments from dskiba.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498177680446280,
"parent_rev": "460df2746e65fd606c137999c0493f3d9b26e2b9", "commit_rev":
"ffe164964fbdf5b1a7245309b2887d45b96eabf0"}
Message was sent while issue was closed.
Description was changed from ========== Update documentation of memory-infra to describe updates to heap profiling. Native heap profiling can now be run on official builds by setting a chrome://flag. BUG= ========== to ========== Update documentation of memory-infra to describe updates to heap profiling. Native heap profiling can now be run on official builds by setting a chrome://flag. BUG= Review-Url: https://codereview.chromium.org/2949353004 Cr-Commit-Position: refs/heads/master@{#481747} Committed: https://chromium.googlesource.com/chromium/src/+/ffe164964fbdf5b1a7245309b288... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ffe164964fbdf5b1a7245309b288... |
