Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(40)

Issue 2674973004: mac: Emit dyld info from the memory dump provider. (Closed)

Created:
3 years, 10 months ago by erikchen
Modified:
3 years, 10 months ago
CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

mac: Emit dyld info from the memory dump provider. Both task_info(TASK_DYLD_INFO) and mach_vm_region_recurse() provide information about the memory regions in the process. There is some duplicate information. This CL updates DumpProcessMemoryMaps() to use both methods, and merges the information from the two functions. This CL also avoids emitting byte stats for regions in the dyld shared cache, since it's not possible to assign byte stats to specific regions, and the regions are likely in use by non-Chrome processes anyways. BUG=677302 Review-Url: https://codereview.chromium.org/2674973004 Cr-Commit-Position: refs/heads/master@{#450113} Committed: https://chromium.googlesource.com/chromium/src/+/a232b05bbe4456d3e7ac38074a48a35488ea6f8b

Patch Set 1 #

Total comments: 1

Patch Set 2 : Merge info from dyld. #

Patch Set 3 : Minor fixes. #

Total comments: 6

Patch Set 4 : Comments from mark. #

Patch Set 5 : Use initprot instead of maxprot. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+208 lines, -39 lines) Patch
M components/tracing/common/process_metrics_memory_dump_provider.cc View 1 2 3 4 4 chunks +195 lines, -36 lines 0 comments Download
M components/tracing/common/process_metrics_memory_dump_provider_unittest.cc View 1 2 chunks +13 lines, -3 lines 0 comments Download

Messages

Total messages: 35 (21 generated)
erikchen
mark: Please review.
3 years, 10 months ago (2017-02-03 22:12:55 UTC) #3
Mark Mentovai
LGTM but consider strengthening the test https://codereview.chromium.org/2674973004/diff/1/components/tracing/common/process_metrics_memory_dump_provider.cc File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2674973004/diff/1/components/tracing/common/process_metrics_memory_dump_provider.cc#newcode261 components/tracing/common/process_metrics_memory_dump_provider.cc:261: // Ignore regions ...
3 years, 10 months ago (2017-02-03 23:17:13 UTC) #7
Primiano Tucci (use gerrit)
Speculative RS-LGTM on whatever you folks decide is the best here.
3 years, 10 months ago (2017-02-06 14:56:18 UTC) #9
erikchen
On 2017/02/03 23:17:13, Mark Mentovai wrote: > LGTM but consider strengthening the test > > ...
3 years, 10 months ago (2017-02-06 22:31:31 UTC) #10
erikchen
I think the right thing to do here is to get information from both TASK_DYLD_INFO ...
3 years, 10 months ago (2017-02-07 03:07:36 UTC) #11
Mark Mentovai
I can get on board with that.
3 years, 10 months ago (2017-02-09 15:56:16 UTC) #12
erikchen
mark: Please review.
3 years, 10 months ago (2017-02-11 01:25:53 UTC) #15
Mark Mentovai
LGTM https://codereview.chromium.org/2674973004/diff/40001/components/tracing/common/process_metrics_memory_dump_provider.cc File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2674973004/diff/40001/components/tracing/common/process_metrics_memory_dump_provider.cc#newcode278 components/tracing/common/process_metrics_memory_dump_provider.cc:278: if (strcmp(seg->segname, SEG_PAGEZERO) == 0) You should check ...
3 years, 10 months ago (2017-02-11 02:18:50 UTC) #19
erikchen
https://codereview.chromium.org/2674973004/diff/40001/components/tracing/common/process_metrics_memory_dump_provider.cc File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2674973004/diff/40001/components/tracing/common/process_metrics_memory_dump_provider.cc#newcode278 components/tracing/common/process_metrics_memory_dump_provider.cc:278: if (strcmp(seg->segname, SEG_PAGEZERO) == 0) On 2017/02/11 02:18:50, Mark ...
3 years, 10 months ago (2017-02-11 02:39:47 UTC) #20
Mark Mentovai
LGTM
3 years, 10 months ago (2017-02-11 02:44:33 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2674973004/60001
3 years, 10 months ago (2017-02-11 02:46:50 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/309294) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 10 months ago (2017-02-11 03:19:55 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2674973004/80001
3 years, 10 months ago (2017-02-13 20:50:21 UTC) #32
commit-bot: I haz the power
3 years, 10 months ago (2017-02-13 22:17:00 UTC) #35
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a232b05bbe4456d3e7ac38074a48...

Powered by Google App Engine
This is Rietveld 408576698