Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417003003:
[tracing] Dump child processes' memory metrics in browser (Closed)
|
Created:
5 years, 1 month ago by ssid Modified:
4 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, darin-cc_chromium.org, jam, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@web_cache2_base Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Dump child processes' memory metrics in browser
The sandbox in linux prevents the process from reading the process
metrics file. To work around this issue, the browser process will now
dump statistics of the child processes too. This requires the
composable dumps support in trace viewer and telemetry.
This CL makes following changes.
1. Move the process totals and memory maps dump provider into a single
header as process_metrics_dump_provider in components/tracing. This is
because it is not necessary to have this in base and now the provider
knows/manages for different processes. Also the dump method are made to
handle error better, since process can vanish while dumping.
2. Make the dump provider non-singleton and have a register / unregister
that manages the lifetime of the dump providers.
3. The dump providers unregister using the new
UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002.
4. On linux the browser process dumps metrics for all processes and on
android the child processes can dump since seccomp sandbox is not
enabled in android yet.
5. The proc/status file is human readable stats and is not guaranteed to
have a field that is asked for. So, the NOTREACHED is removed in
ReadProcStatusAndGetFieldAsSizeT.
6. Since we introduce other process dumps from browser process there
could be races while unregistering and dumping. To test this, the
browser test is updated.
BUG=461788
Committed: https://crrev.com/4d77d76a42425282b1a3c5b7309db9b98e777f60
Cr-Commit-Position: refs/heads/master@{#369482}
Patch Set 1 #Patch Set 2 : Nits. #
Total comments: 2
Patch Set 3 : no gn. #Patch Set 4 : Fixes #Patch Set 5 : tests #Patch Set 6 : dNits. #
Total comments: 16
Patch Set 7 : Remove manager #
Total comments: 16
Patch Set 8 : rebase. #Patch Set 9 : Rebase. #Patch Set 10 : primiano's comments #
Total comments: 42
Patch Set 11 : addressing comments. #Patch Set 12 : Rebase and fix nacl #
Total comments: 12
Patch Set 13 : Fixes. #
Total comments: 5
Patch Set 14 : Fix header macro. #
Total comments: 3
Patch Set 15 : Adding comment instead of notreached. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 64 (21 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL, Thanks.
this is way more invasive that what it needs to be. The MDM should not need to know anything about the per-process providers. You already keep track of them in the TracingControllerImpl https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:104: // workaround the issue the coordinator process dumps statistics about the the manager should not need to know about all this. Why do you want to keep track of them here? https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/proces... File base/trace_event/process_memory_maps_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/proces... base/trace_event/process_memory_maps_dump_provider.h:21: static scoped_ptr<ProcessMemoryMapsDumpProvider> CreateForProcess( this cannot be a singleton and a non-singleton at the same time. de-singletonify?
On 2015/11/03 15:14:08, Primiano Tucci wrote: > this is way more invasive that what it needs to be. > The MDM should not need to know anything about the per-process providers. > You already keep track of them in the TracingControllerImpl > I added in MDM because there should be someone who owns the providers. The provider itself cannot be made to own multiple providers, else I should change its name. If not MDM then TraceMessageFilter should own it. If TraceMessageFilter owns the providers for child, then browser's providers should be owned by MDM or browser main or better suggestion?. I didn't like the idea of multiple locations owning the providers, so placed them all in MDM. Should I let the current process providers to be global objects and provider has method: static provider* GetForCurrentProcess()? Also, now register and unregister happens in IPC thread for child process providers, and dump happens in memory-infra thread. So, I should either use IPC task runner or add a lock here. > https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/memory... > File base/trace_event/memory_dump_manager.h (right): > > https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/memory... > base/trace_event/memory_dump_manager.h:104: // workaround the issue the > coordinator process dumps statistics about the > the manager should not need to know about all this. > Why do you want to keep track of them here? > > https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/proces... > File base/trace_event/process_memory_maps_dump_provider.h (right): > > https://codereview.chromium.org/1417003003/diff/20001/base/trace_event/proces... > base/trace_event/process_memory_maps_dump_provider.h:21: static > scoped_ptr<ProcessMemoryMapsDumpProvider> CreateForProcess( > this cannot be a singleton and a non-singleton at the same time. > de-singletonify?
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The child process cannot read /proc folder due to the sandbox in linux. To workaround this issue the memory metrics of the child processes are dumped from browser process and later these dumps will be merged into a single memory dump in trace viewer. See crrev.com/1404343005. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. 2. Make the dump provider non-singleton and have a singleton manager that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. BUG=461788 ==========
WDYT about this?
I am going to treview this soon, thanks for the CL. In the meantime, one thing comes to my mind: before landing this, we have to update telemetry. I am pretty sure that this change as it is will skew all values in telemetry that don't know how to recomose the split dumps. Maybe take a look to memory_infra.py in the meantime ensuring that when this lands we will have also the fix for telemetry ready.
On 2015/11/13 17:39:39, Primiano Tucci wrote: > I am going to treview this soon, thanks for the CL. > In the meantime, one thing comes to my mind: before landing this, we have to > update telemetry. I am pretty sure that this change as it is will skew all > values in telemetry that don't know how to recomose the split dumps. > Maybe take a look to memory_infra.py in the meantime ensuring that when this > lands we will have also the fix for telemetry ready. Yeah i forgot about telemetry. Lets keep this on hold till telemetry is fixed.
The approach makes sense. My main objection here is that I'd like to avoid yet another "manager". Can we move the register/unregisterForProcess in the dump provider (as static methods?). In the meantime I realized I need to finish crrev.com/1430073002 as you depend on this :) https://codereview.chromium.org/1417003003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event.gypi (left): https://codereview.chromium.org/1417003003/diff/100001/base/trace_event/trace... base/trace_event/trace_event.gypi:95: 'trace_event/process_memory_maps_dump_provider_unittest.cc', what about this unittest? https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... File components/tracing/child_memory_dump_manager_delegate_impl.cc (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... components/tracing/child_memory_dump_manager_delegate_impl.cc:7: #include "base/single_thread_task_runner.h" include what you use: add explicit build_config.h include https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... components/tracing/child_memory_dump_manager_delegate_impl.cc:55: // Do not register in linux due to sandbox. Make this more explicit and say: on Linux the browser process takes care of dumping process stats of child processes. The child themselves are not allowed to do so due to the BPF sandbox. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.cc (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.cc:25: if (process != base::kNullProcessHandle) not sure I understand this. This is saying: on Non-Linux, if you try to RegisterForProcess(some child pid) silently turn this into a noop? Why is this needed? Can we remove lines 23-27 completely? https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.h (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.h:20: class TRACING_EXPORT ProcessMetricsMemoryDumpManager { hmm we have way too many managers in memory-infra. I understand the need of RegisterFOrProcess, but can we just move that into the ProcessMetricsMemoryDumpProvider itself (as static methods)? https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:494: trace_message_filter->peer_pid()); Shouldn't this be >PeerHandle() ? https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:525: trace_message_filter->peer_pid()); ditto
Also, the UnregisterAsync was failing everytime some process ends. I will debug that once you finish your cl. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.cc (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.cc:25: if (process != base::kNullProcessHandle) On 2015/11/17 10:44:12, Primiano Tucci wrote: > not sure I understand this. > This is saying: on Non-Linux, if you try to RegisterForProcess(some child pid) > silently turn this into a noop? > Why is this needed? Can we remove lines 23-27 completely? If I do not add this here then I would have to change the tracing controller to have a if defined(OS_LINUX) then register. I thought it is better to have this ifdefs in one place. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.h (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.h:20: class TRACING_EXPORT ProcessMetricsMemoryDumpManager { On 2015/11/17 10:44:12, Primiano Tucci wrote: > hmm we have way too many managers in memory-infra. I understand the need of > RegisterFOrProcess, but can we just move that into the > ProcessMetricsMemoryDumpProvider itself (as static methods)? This would mean that there is a global std::map (not ScopedPtrMap) of the pointers to dump providers in anonymous namespace. Also, this map is stored as pointer or a lazy instance. Manager with scoped_ptrs sounded better than that. The reason for the manager is to store the providers, not registration. WDYT?
> This would mean that there is a global std::map (not ScopedPtrMap) of the > pointers to dump providers in anonymous namespace. Also, this map is stored as > pointer or a lazy instance. Right, you'll need a LazyInstance<ScopedPtrMap> in the anonymous namespace > Manager with scoped_ptrs sounded better than that. You want to provide some arguments/metric. to say better / wrong. > The reason for the manager is to store the providers, not registration. WDYT? Yeah, so you are saying that you are introducing an extra class, with all the singleton and buildfile boilerplate for the only sake of handling a map of pointers. IMHO the complexity you are introducing is not worth the unreadability cost and maintenance burden of an extra translation unit. Also this makes the code more confusing (another manager, which in reality is just a wrapper around a map of pointers)
thanks ptal. https://codereview.chromium.org/1417003003/diff/100001/base/trace_event/trace... File base/trace_event/trace_event.gypi (left): https://codereview.chromium.org/1417003003/diff/100001/base/trace_event/trace... base/trace_event/trace_event.gypi:95: 'trace_event/process_memory_maps_dump_provider_unittest.cc', On 2015/11/17 10:44:12, Primiano Tucci wrote: > what about this unittest? Done. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... File components/tracing/child_memory_dump_manager_delegate_impl.cc (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... components/tracing/child_memory_dump_manager_delegate_impl.cc:7: #include "base/single_thread_task_runner.h" On 2015/11/17 10:44:12, Primiano Tucci wrote: > include what you use: add explicit build_config.h include Done. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/chi... components/tracing/child_memory_dump_manager_delegate_impl.cc:55: // Do not register in linux due to sandbox. On 2015/11/17 10:44:12, Primiano Tucci wrote: > Make this more explicit and say: on Linux the browser process takes care of > dumping process stats of child processes. The child themselves are not allowed > to do so due to the BPF sandbox. Done. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.cc (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.cc:25: if (process != base::kNullProcessHandle) On 2015/11/17 11:13:07, ssid wrote: > On 2015/11/17 10:44:12, Primiano Tucci wrote: > > not sure I understand this. > > This is saying: on Non-Linux, if you try to RegisterForProcess(some child pid) > > silently turn this into a noop? > > Why is this needed? Can we remove lines 23-27 completely? > > If I do not add this here then I would have to change the tracing controller to > have a if defined(OS_LINUX) then register. I thought it is better to have this > ifdefs in one place. Done. https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_manager.h (right): https://codereview.chromium.org/1417003003/diff/100001/components/tracing/pro... components/tracing/process_metrics_memory_dump_manager.h:20: class TRACING_EXPORT ProcessMetricsMemoryDumpManager { On 2015/11/17 10:44:12, Primiano Tucci wrote: > hmm we have way too many managers in memory-infra. I understand the need of > RegisterFOrProcess, but can we just move that into the > ProcessMetricsMemoryDumpProvider itself (as static methods)? Done. https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:494: trace_message_filter->peer_pid()); On 2015/11/17 10:44:13, Primiano Tucci wrote: > Shouldn't this be >PeerHandle() ? Done. https://codereview.chromium.org/1417003003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:525: trace_message_filter->peer_pid()); On 2015/11/17 10:44:12, Primiano Tucci wrote: > ditto Done.
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. 2. Make the dump provider non-singleton and have a singleton manager that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. BUG=461788 ==========
LGTM with some final comment (please address them). I wonder how we are going to deal with Android once it will get bpf-sandbox, would be nice to have a plan there. Also, remember about telemetry, this cannot fly if telemetry is not ready https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:24: base::ProcessMetrics* CreateProcessMetrics(base::ProcessHandle process) { why not returning a scoped_ptr here? https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:43: scoped_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider( Can we have a DCHECK_CURRENTLY_ON to make sure that we call this only from the same thread (whatver it is)? https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:45: base::trace_event::MemoryDumpProvider::Options options(process); technically options takes a ProcessId, and now you are passing a ProcessHandle https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:54: scoped_ptr<ProcessMetricsMemoryDumpProvider> provider = same here about the DCHECK_CURRENTLY_ON https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:80: #if !defined(OS_LINUX) && !defined(OS_ANDROID) Can you add a comment explaining that on linux/android there is a specialized function for this and reference the file name. Otherwise this is really confusing. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:26: static void RegisterForProcess(base::ProcessHandle process); What is the rationale for passing ProcessHandle w.r.t. ProcessId? In the code looks like you just need the ProcessId (which, btw, on Linux/Android happens to be the same thing) https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider_posix.cc (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_posix.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. _posix is a bit misleading here, as it suggests that this is also for mac, which doesn't seem the case here. I think it's saner to call this _linux.cc (which it's ok if it applies also to android) https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_posix.cc:198: pmd->set_has_process_mmaps(); what happens if something fails? we don't get any output anymore? Not even a DCHECK? Why not keeping returning true / false if this can fail?
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ==========
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ==========
Patchset #10 (id:180001) has been deleted
Check diff with last patchset for a little clean diff of changes without rebase noise. Please take another look. Thanks. > I wonder how we are going to deal with Android once it will get bpf-sandbox, > would be nice to have a plan there. > I will discuss about the plan for android on the bug. > Also, remember about telemetry, this cannot fly if telemetry is not ready I have uploaded a cl for telemetry support: https://codereview.chromium.org/1553183002/ https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:24: base::ProcessMetrics* CreateProcessMetrics(base::ProcessHandle process) { On 2015/12/14 10:52:21, Primiano Tucci wrote: > why not returning a scoped_ptr here? Done. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:43: scoped_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider( On 2015/12/14 10:52:21, Primiano Tucci wrote: > Can we have a DCHECK_CURRENTLY_ON to make sure that we call this only from the > same thread (whatver it is)? This would require adding deps of content/public/browser to components tracing. I don't see a need to add this dcheck since it is thread safe and the call is non-blocking. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:45: base::trace_event::MemoryDumpProvider::Options options(process); On 2015/12/14 10:52:21, Primiano Tucci wrote: > technically options takes a ProcessId, and now you are passing a ProcessHandle Fixed this. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:54: scoped_ptr<ProcessMetricsMemoryDumpProvider> provider = On 2015/12/14 10:52:21, Primiano Tucci wrote: > same here about the DCHECK_CURRENTLY_ON same as above. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:80: #if !defined(OS_LINUX) && !defined(OS_ANDROID) On 2015/12/14 10:52:21, Primiano Tucci wrote: > Can you add a comment explaining that on linux/android there is a specialized > function for this and reference the file name. > Otherwise this is really confusing. Done. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:26: static void RegisterForProcess(base::ProcessHandle process); On 2015/12/14 10:52:21, Primiano Tucci wrote: > What is the rationale for passing ProcessHandle w.r.t. ProcessId? In the code > looks like you just need the ProcessId (which, btw, on Linux/Android happens to > be the same thing) Hm the CreateProcessMetrics takes ProcessHandle and ProcessId. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider_posix.cc (right): https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_posix.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/14 10:52:21, Primiano Tucci wrote: > _posix is a bit misleading here, as it suggests that this is also for mac, which > doesn't seem the case here. > I think it's saner to call this _linux.cc (which it's ok if it applies also to > android) Done. https://codereview.chromium.org/1417003003/diff/120001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_posix.cc:198: pmd->set_has_process_mmaps(); On 2015/12/14 10:52:21, Primiano Tucci wrote: > what happens if something fails? we don't get any output anymore? > Not even a DCHECK? > Why not keeping returning true / false if this can fail? Done.
Oh, I forgot. I also added points 5 and 6 of the description after your review.
Other review round. We're close :) Everything looks fine, but I've some smaller comments here and there. The summary is: - be careful, you have rebase conflicts here. see comments inline. - Use ProcessId instead of ProcessHandle and most of the awkward conversions will go away (also remember to replace kNullProcessHandle with kNullProcessId) - Up to you on this, but I wonder if instead of having the process_metric..._linux.cc file and have to deal with the crazy build file rules, you can put everything in the parent .cc file and #ifdef https://codereview.chromium.org/1417003003/diff/200001/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/1417003003/diff/200001/base/BUILD.gn#oldcode986 base/BUILD.gn:986: "debug/proc_maps_linux.cc", I think you are mistakenly removing this line after some rebase https://codereview.chromium.org/1417003003/diff/200001/base/process/process_m... File base/process/process_metrics_linux.cc (left): https://codereview.chromium.org/1417003003/diff/200001/base/process/process_m... base/process/process_metrics_linux.cc:89: NOTREACHED(); hmm if you do this I think you need to turn this into a function that returns a bool and puts the value into a out ptr. I mean: bool ReadProcStatusAndGetFieldAsSizeT(pid_t pid, const std::string& field, size_t* value) There are only two places that use ReadProcStatus...., you can move the DCHECK to those places https://codereview.chromium.org/1417003003/diff/200001/chrome/test/base/traci... File chrome/test/base/tracing_browsertest.cc (right): https://codereview.chromium.org/1417003003/diff/200001/chrome/test/base/traci... chrome/test/base/tracing_browsertest.cc:64: // Start new processes and stop a process while tracing is enabled. I think you have other leftover of rebase conflicts here https://codereview.chromium.org/1417003003/diff/200001/components/policy/core... File components/policy/core/common/schema_registry.cc (left): https://codereview.chromium.org/1417003003/diff/200001/components/policy/core... components/policy/core/common/schema_registry.cc:58: NOTREACHED(); same here (conflicts) https://codereview.chromium.org/1417003003/diff/200001/components/tracing.gyp File components/tracing.gyp (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing.gyp... components/tracing.gyp:51: ['include', '^tracing/process_metrics_memory_dump_provider_linux\\.'], I think the pattern in this case (to bypass the source assignment filter) is to terminate the regex with (cc|h)$ . i:e: ['include', '^tracing/process_metrics_memory_dump_provider_linux\\.(cc|h)$'] https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:32: // Creating process metrics for child processes in mac requires PortProvider. I'd add "and is a non needed use case" to the comment https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:48: base::GetProcId(process)); If you need a process_id at the end, doesn't it make more sense that you use ProcessId instead of ProcessHandle here (and in all the rest?). So you don't have to make this conversion. Also, the code for GetProcID says: "DEPRECATED. New code should be using Process::Pid() instead" https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:51: g_dump_providers_map.Get().insert( maybe make this a bool did_insert = g_dump_providers_map.Get().insert(...).second DCHECK(did_insert) << "ProcessMetricsMemoryDumpProvider already registered for process " << pid https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:59: DCHECK(iter != g_dump_providers_map.Get().end()); I'd make this a bit safer and do: if (iter == g_dump_providers_map.Get().end()) { NOTREACHED(); return; } https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:79: res &= DumpProcessMemoryMaps(args, pmd); shouldn't this check for heavy vs light dump here? https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:84: // Linux and Android have specialized function in I'd reword this as: // See process_metrics_memory_dump_provider_linux.cc for the Linux/Android-specific implementation. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:107: if (res) { nit: you can remove these braces here https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:117: // Returns true even if other metrics failed, since rss is reported. /reported/always reported/ https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:5: #ifndef BASE_TRACE_EVENT_PROCESS_MEMORY_MAPS_DUMP_PROVIDER_H_ This include guard needs to be adjusted to match the new path https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:61: #endif // BASE_TRACE_EVENT_PROCESS_MEMORY_MAPS_DUMP_PROVIDER_H_ ditto https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider_linux.cc (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:84: if (did_read != 1) out of curiosity. did you encounter cases where this fails? https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:148: bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals( to be honest, this function doesn't seem that much different (% the kernel rss support) than the base one. Maybe you should just make this a single one #ifdef-ing the rss part? Or am I missing something else? https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:162: // This file cannot be wriiten for other processes. So, do not try. I'd reword this as: Due to UID isolation, clear_refs can only be written by the process itself and cannot be used in the cases of a process dumping on behalf of another one. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:191: if (UNLIKELY(proc_smaps_for_testing)) { not your fault, since you are here can you remove this UNLIKELY? Considered how slow is the mmaps dumping, this is not going to make any difference, just makes the code more unreadable https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:194: std::string file_name = "/proc/" + (process_ == base::kNullProcessHandle and that's why you want ProcessId and not ProcessHandle here. This is now a bit of a hack. You are relying on the fact that on linux ProcessId and ProcessHandle are the same thing :) https://codereview.chromium.org/1417003003/diff/200001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/200001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:561: // The browser process dumps process metrics for child process in linux due to nit: reword as: On Linux the browser....
Patchset #14 (id:280001) has been deleted
Patchset #12 (id:240001) has been deleted
Patchset #13 (id:300001) has been deleted
Patchset #12 (id:260001) has been deleted
Patchset #12 (id:320001) has been deleted
I have addressed comments as much as possible. Please see replies inline. PTAL, thanks. https://codereview.chromium.org/1417003003/diff/200001/base/BUILD.gn File base/BUILD.gn (left): https://codereview.chromium.org/1417003003/diff/200001/base/BUILD.gn#oldcode986 base/BUILD.gn:986: "debug/proc_maps_linux.cc", On 2016/01/05 11:55:17, Primiano Tucci wrote: > I think you are mistakenly removing this line after some rebase Done. https://codereview.chromium.org/1417003003/diff/200001/base/process/process_m... File base/process/process_metrics_linux.cc (left): https://codereview.chromium.org/1417003003/diff/200001/base/process/process_m... base/process/process_metrics_linux.cc:89: NOTREACHED(); On 2016/01/05 11:55:17, Primiano Tucci wrote: > hmm if you do this I think you need to turn this into a function that returns a > bool and puts the value into a out ptr. > I mean: > > bool ReadProcStatusAndGetFieldAsSizeT(pid_t pid, const std::string& field, > size_t* value) > > There are only two places that use ReadProcStatus...., you can move the DCHECK > to those places I think this dcheck should not exist since it is okay to fail here. Sometimes status does not give the values needed. And the only place which uses this method is our code in dump provider. Now I made it bool and to fail in GetPeakPageFileUsage and not fail in GetPeakWorkingSetSize https://codereview.chromium.org/1417003003/diff/200001/chrome/test/base/traci... File chrome/test/base/tracing_browsertest.cc (right): https://codereview.chromium.org/1417003003/diff/200001/chrome/test/base/traci... chrome/test/base/tracing_browsertest.cc:64: // Start new processes and stop a process while tracing is enabled. On 2016/01/05 11:55:17, Primiano Tucci wrote: > I think you have other leftover of rebase conflicts here Adding close tab to make sure renderer is recycled. https://codereview.chromium.org/1417003003/diff/200001/components/policy/core... File components/policy/core/common/schema_registry.cc (left): https://codereview.chromium.org/1417003003/diff/200001/components/policy/core... components/policy/core/common/schema_registry.cc:58: NOTREACHED(); On 2016/01/05 11:55:17, Primiano Tucci wrote: > same here (conflicts) Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing.gyp File components/tracing.gyp (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing.gyp... components/tracing.gyp:51: ['include', '^tracing/process_metrics_memory_dump_provider_linux\\.'], On 2016/01/05 11:55:17, Primiano Tucci wrote: > I think the pattern in this case (to bypass the source assignment filter) is to > terminate the regex with (cc|h)$ . > i:e: > ['include', '^tracing/process_metrics_memory_dump_provider_linux\\.(cc|h)$'] removed. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:32: // Creating process metrics for child processes in mac requires PortProvider. On 2016/01/05 11:55:17, Primiano Tucci wrote: > I'd add "and is a non needed use case" to the comment Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:48: base::GetProcId(process)); On 2016/01/05 11:55:17, Primiano Tucci wrote: > If you need a process_id at the end, doesn't it make more sense that you use > ProcessId instead of ProcessHandle here (and in all the rest?). > So you don't have to make this conversion. > Also, the code for GetProcID says: > "DEPRECATED. New code should be using Process::Pid() instead" Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:51: g_dump_providers_map.Get().insert( On 2016/01/05 11:55:17, Primiano Tucci wrote: > maybe make this a > > bool did_insert = g_dump_providers_map.Get().insert(...).second > DCHECK(did_insert) << "ProcessMetricsMemoryDumpProvider already registered for > process " << pid hm, there are nacl processes in chromeos builds which do not give the correct pid to browser process. These processes are all with pid "1" and registers multiple times. It seems complicated to understand why the nacl helper processes are not giving the correct pids and I don't think its worth the time to fix that. So, making this a warning message. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:59: DCHECK(iter != g_dump_providers_map.Get().end()); On 2016/01/05 11:55:17, Primiano Tucci wrote: > I'd make this a bit safer and do: > > if (iter == g_dump_providers_map.Get().end()) { > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:79: res &= DumpProcessMemoryMaps(args, pmd); On 2016/01/05 11:55:17, Primiano Tucci wrote: > shouldn't this check for heavy vs light dump here? Moved it here. It was inside the function. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:84: // Linux and Android have specialized function in On 2016/01/05 11:55:17, Primiano Tucci wrote: > I'd reword this as: > > // See process_metrics_memory_dump_provider_linux.cc for the > Linux/Android-specific implementation. removed. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:107: if (res) { On 2016/01/05 11:55:17, Primiano Tucci wrote: > nit: you can remove these braces here Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:117: // Returns true even if other metrics failed, since rss is reported. On 2016/01/05 11:55:17, Primiano Tucci wrote: > /reported/always reported/ The rss is not reported, and function fails if child process ended. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:5: #ifndef BASE_TRACE_EVENT_PROCESS_MEMORY_MAPS_DUMP_PROVIDER_H_ On 2016/01/05 11:55:17, Primiano Tucci wrote: > This include guard needs to be adjusted to match the new path Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider_linux.cc (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:84: if (did_read != 1) On 2016/01/05 11:55:17, Primiano Tucci wrote: > out of curiosity. did you encounter cases where this fails? Not this case, I removed to be safe. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:148: bool ProcessMetricsMemoryDumpProvider::DumpProcessTotals( On 2016/01/05 11:55:17, Primiano Tucci wrote: > to be honest, this function doesn't seem that much different (% the kernel rss > support) than the base one. > Maybe you should just make this a single one #ifdef-ing the rss part? > Or am I missing something else? Yes, I had made 2 files because i didn't want one large file. Also if mac memory maps is added it should be split again. Feels like this file is getting dirty with if-defs. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:162: // This file cannot be wriiten for other processes. So, do not try. On 2016/01/05 11:55:17, Primiano Tucci wrote: > I'd reword this as: > Due to UID isolation, clear_refs can only be written by the process itself and > cannot be used in the cases of a process dumping on behalf of another one. Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:191: if (UNLIKELY(proc_smaps_for_testing)) { On 2016/01/05 11:55:17, Primiano Tucci wrote: > not your fault, since you are here can you remove this UNLIKELY? > Considered how slow is the mmaps dumping, this is not going to make any > difference, just makes the code more unreadable Done. https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider_linux.cc:194: std::string file_name = "/proc/" + (process_ == base::kNullProcessHandle On 2016/01/05 11:55:17, Primiano Tucci wrote: > and that's why you want ProcessId and not ProcessHandle here. > This is now a bit of a hack. > You are relying on the fact that on linux ProcessId and ProcessHandle are the > same thing :) Done. https://codereview.chromium.org/1417003003/diff/200001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/200001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:561: // The browser process dumps process metrics for child process in linux due to On 2016/01/05 11:55:17, Primiano Tucci wrote: > nit: reword as: On Linux the browser.... Done.
Excellent! final LGTM with few last comments. Sorry for the back and forth on base/, I think your previous proposal was better. Thanks for working on this, I start seeing the light! :) https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/200001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:51: g_dump_providers_map.Get().insert( On 2016/01/06 20:26:33, ssid wrote: > On 2016/01/05 11:55:17, Primiano Tucci wrote: > > maybe make this a > > > > bool did_insert = g_dump_providers_map.Get().insert(...).second > > DCHECK(did_insert) << "ProcessMetricsMemoryDumpProvider already registered for > > process " << pid > > hm, there are nacl processes in chromeos builds which do not give the correct > pid to browser process. These processes are all with pid "1" and registers > multiple times. It seems complicated to understand why the nacl helper processes > are not giving the correct pids and I don't think its worth the time to fix > that. So, making this a warning message. Ahh I see. Agree not worth wasting time for those. https://codereview.chromium.org/1417003003/diff/340001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1417003003/diff/340001/base/process/process_m... base/process/process_metrics_linux.cc:193: size_t value = 0; Oooh I see, you still cannot have the DCHECK here. Super apologies, didn't get this. At this point I think your previous proposal (just remove the dcheck) was better and more consistent. Sorry for having you go back and forth here. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:214: LOG(ERROR) << "ProcessMetricsMemoryDumpProvider already registered for " if you say that this can actually happen on CrOs, make it a DLOG, so won't spam on production builds. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:226: NOTREACHED(); at this point, given your comment on the pid=1, I guess you should remove this notreached, as you might have more >1 providers unregistering for pid=1 https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:250: // Snapshot of memory maps is taken only for detailed dump requests. I think this comment is a bit redundant, the code below is perfectly readable :) https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:282: base::WriteFileDescriptor(clear_refs_fd, kClearPeakRssCommand, you need to look at the return value of WriteFileDescriptor. What is going to happen on old kernels is that you will be able to open the file successfully, but the actual write() will fail. So if you see WriteFIleDescriptor failing, you need to set is_peak_resettable to false and avoid trying again. https://codereview.chromium.org/1417003003/diff/340001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/340001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:597: trace_message_filter->PeerHandle()); s/PeerHandle/peer_pid/
Thanks, i made this cl depend on https://codereview.chromium.org/1553183002/ and will finish that first. https://codereview.chromium.org/1417003003/diff/340001/base/process/process_m... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/1417003003/diff/340001/base/process/process_m... base/process/process_metrics_linux.cc:193: size_t value = 0; On 2016/01/07 11:34:35, Primiano Tucci wrote: > Oooh I see, you still cannot have the DCHECK here. > Super apologies, didn't get this. > At this point I think your previous proposal (just remove the dcheck) was better > and more consistent. > Sorry for having you go back and forth here. Done. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:214: LOG(ERROR) << "ProcessMetricsMemoryDumpProvider already registered for " On 2016/01/07 11:34:35, Primiano Tucci wrote: > if you say that this can actually happen on CrOs, make it a DLOG, so won't spam > on production builds. Done. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:226: NOTREACHED(); On 2016/01/07 11:34:35, Primiano Tucci wrote: > at this point, given your comment on the pid=1, I guess you should remove this > notreached, as you might have more >1 providers unregistering for pid=1 Acknowledged. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:250: // Snapshot of memory maps is taken only for detailed dump requests. On 2016/01/07 11:34:35, Primiano Tucci wrote: > I think this comment is a bit redundant, the code below is perfectly readable :) Done. https://codereview.chromium.org/1417003003/diff/340001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:282: base::WriteFileDescriptor(clear_refs_fd, kClearPeakRssCommand, On 2016/01/07 11:34:35, Primiano Tucci wrote: > you need to look at the return value of WriteFileDescriptor. > What is going to happen on old kernels is that you will be able to open the file > successfully, but the actual write() will fail. > So if you see WriteFIleDescriptor failing, you need to set is_peak_resettable to > false and avoid trying again. Acknowledged. https://codereview.chromium.org/1417003003/diff/340001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1417003003/diff/340001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:597: trace_message_filter->PeerHandle()); On 2016/01/07 11:34:35, Primiano Tucci wrote: > s/PeerHandle/peer_pid/ Done.
ssid@chromium.org changed reviewers: + dsinclair@chromium.org, thakis@chromium.org
+thakis for changes in process_metrics_linux.cc. I removed a NOTREACHED (see description point 5). And chrome/test/base/tracing_browsertest.cc for changing browser test +dsinclair for changes in components/tracing. It is mostly copying trace_event/ files and merging them.
dsinclair@chromium.org changed reviewers: + simonhatch@chromium.org - dsinclair@chromium.org
passing over to simonhatch@.
primiano@chromium.org changed reviewers: + sievers@chromium.org
Also +sievers for 3 lines change in content/browser/browser_main_loop.cc
components/tracing lgtm https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:24: #include "build/build_config.h" nit: Some stuff included here is also in the header, ie memory_dump_manager.h/build_config.h https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:5: #ifndef COMPONENTS_TRACING_PROCESS_MEMORY_MAPS_DUMP_PROVIDER_H_ nit: Shouldn't this be COMPONENTS_TRACING_PROCESS_METRICS_MEMORY_DUMP_PROVIDER_H_
On 2016/01/07 14:47:33, Primiano Tucci wrote: > Also +sievers for 3 lines change in content/browser/browser_main_loop.cc lgtm
@shatch, thanks see replies inline. https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:24: #include "build/build_config.h" On 2016/01/07 15:52:58, shatch wrote: > nit: Some stuff included here is also in the header, ie > memory_dump_manager.h/build_config.h Hm, manager is not included in header. (it is provider.h) I left the build_config.h here just in case if the header removes it, this file will still have it. note that we use #if defined(MACRO) from the build_config.h file. It won't throw error if this is removed from header. but miss stuff. If you still feel this should be removed, i will do it. https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.h:5: #ifndef COMPONENTS_TRACING_PROCESS_MEMORY_MAPS_DUMP_PROVIDER_H_ On 2016/01/07 15:52:58, shatch wrote: > nit: Shouldn't this be > COMPONENTS_TRACING_PROCESS_METRICS_MEMORY_DUMP_PROVIDER_H_ Done.
https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... File components/tracing/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/1417003003/diff/360001/components/tracing/pro... components/tracing/process_metrics_memory_dump_provider.cc:24: #include "build/build_config.h" On 2016/01/07 19:25:24, ssid wrote: > On 2016/01/07 15:52:58, shatch wrote: > > nit: Some stuff included here is also in the header, ie > > memory_dump_manager.h/build_config.h > > Hm, manager is not included in header. (it is provider.h) > I left the build_config.h here just in case if the header removes it, this file > will still have it. note that we use #if defined(MACRO) from the build_config.h > file. It won't throw error if this is removed from header. but miss stuff. > > If you still feel this should be removed, i will do it. Seems fair, just leave it in then.
https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... File base/process/process_metrics_linux.cc (left): https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... base/process/process_metrics_linux.cc:89: NOTREACHED(); is there test coverage for this change?
https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... File base/process/process_metrics_linux.cc (left): https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... base/process/process_metrics_linux.cc:89: NOTREACHED(); On 2016/01/08 00:45:52, Nico wrote: > is there test coverage for this change? There doesn't seem to be any unittest for this function or GetPeakWorkingSet api. So, the answer here is no.
https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... File base/process/process_metrics_linux.cc (left): https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... base/process/process_metrics_linux.cc:89: NOTREACHED(); On 2016/01/08 11:26:01, ssid wrote: > On 2016/01/08 00:45:52, Nico wrote: > > is there test coverage for this change? > > There doesn't seem to be any unittest for this function or GetPeakWorkingSet > api. So, the answer here is no. I mean, is this path now taken while it wasn't before? Or are you removing the NOTREACHED() just in case?
On 2016/01/08 16:07:34, Nico wrote: > https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... > File base/process/process_metrics_linux.cc (left): > > https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... > base/process/process_metrics_linux.cc:89: NOTREACHED(); > On 2016/01/08 11:26:01, ssid wrote: > > On 2016/01/08 00:45:52, Nico wrote: > > > is there test coverage for this change? > > > > There doesn't seem to be any unittest for this function or GetPeakWorkingSet > > api. So, the answer here is no. > > I mean, is this path now taken while it wasn't before? Or are you removing the > NOTREACHED() just in case? Oh yes. There are browsertests (that I also updated in this CL) which will hit this NOTREACHED. This might hit the check because the process shuts down while tracing and the file can go corrupt while it is being read and it would crash. That is why i removed this. It was not an issue before since it was always used in same process and the file was always available from within the process. There was no use case of this path for other processes before this CL.
On 2016/01/08 16:10:39, ssid wrote: > On 2016/01/08 16:07:34, Nico wrote: > > > https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... > > File base/process/process_metrics_linux.cc (left): > > > > > https://codereview.chromium.org/1417003003/diff/380001/base/process/process_m... > > base/process/process_metrics_linux.cc:89: NOTREACHED(); > > On 2016/01/08 11:26:01, ssid wrote: > > > On 2016/01/08 00:45:52, Nico wrote: > > > > is there test coverage for this change? > > > > > > There doesn't seem to be any unittest for this function or GetPeakWorkingSet > > > api. So, the answer here is no. > > > > I mean, is this path now taken while it wasn't before? Or are you removing the > > NOTREACHED() just in case? > > Oh yes. There are browsertests (that I also updated in this CL) which will hit > this NOTREACHED. > This might hit the check because the process shuts down while tracing and the > file can go corrupt while it is being read and it would crash. That is why i > removed this. Sorry for being unclear. The renderer process could shut down while tracing. The browser reading this file might see only half the file. > It was not an issue before since it was always used in same process and the file > was always available from within the process. There was no use case of this path > for other processes before this CL.
On 2016/01/08 16:07:34, Nico wrote: > I mean, is this path now taken while it wasn't before? Or are you removing the > NOTREACHED() just in case? Nico is right. Before we had a NOTREACHED() just in case, nobody really cared about what is the return value of GetWorkingSet(42) (assuming pid=42 does not exist) Now you are relying on this behavior, you don't want GetWorkingSet(42) to crash (Even if just in debug) and you want to know that you can rely on that. So you should add a test, to make sure that somebody doesn't reintroduce a NOTREACHED just-in-case ignoring the fact that you rely on that.
if the existing browser tests reliably hit this, that's good enough for me, lgtm, thanks.
No, the test does not hit reliably. Just had an offline discussion with primiano about how can i possibly test this. But, there is not really a way to reliably hit this case since it is a kind of a race. I could maybe add an unittest for this function with a fake status file string (but this function is not even an api). But doesn't seem worth it since most of this file is not covered with unittests and not even used anywhere in code base. There shoudl ideally be no one adding back the DCHECK since the description of proc/<pid>/status says it is human readable so some fields may not exist. Do you have any suggestions?
is there a way to discover that a file read from below /proc is truncated due to the process going away?
On 2016/01/08 18:24:59, Nico wrote: > is there a way to discover that a file read from below /proc is truncated due to > the process going away? No. It doesn't actually get truncated but it just missed few fields and gives invalid values for some fields. So there's no way to detect if it is truncated. There are cases where the file has first ten and last few lines with invalid and didn't show other fields.
Hm, maybe put in a comment "// This can be reached if a process dies while the /proc file is read -- in that case, the kernel can return missing fields" where the NOTREACHED() was then. Other than that, I think the CL is fine.
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This requires the composable dumps support in trace viewer and telemetry. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ==========
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417003003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417003003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, sievers@chromium.org, simonhatch@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1417003003/#ps400001 (title: "Adding comment instead of notreached.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417003003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417003003/400001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This requires the composable dumps support in trace viewer and telemetry. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This requires the composable dumps support in trace viewer and telemetry. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This requires the composable dumps support in trace viewer and telemetry. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 ========== to ========== [tracing] Dump child processes' memory metrics in browser The sandbox in linux prevents the process from reading the process metrics file. To work around this issue, the browser process will now dump statistics of the child processes too. This requires the composable dumps support in trace viewer and telemetry. This CL makes following changes. 1. Move the process totals and memory maps dump provider into a single header as process_metrics_dump_provider in components/tracing. This is because it is not necessary to have this in base and now the provider knows/manages for different processes. Also the dump method are made to handle error better, since process can vanish while dumping. 2. Make the dump provider non-singleton and have a register / unregister that manages the lifetime of the dump providers. 3. The dump providers unregister using the new UnregisterAndDeleteDumpProviderAsync api added by crrev.com/1430073002. 4. On linux the browser process dumps metrics for all processes and on android the child processes can dump since seccomp sandbox is not enabled in android yet. 5. The proc/status file is human readable stats and is not guaranteed to have a field that is asked for. So, the NOTREACHED is removed in ReadProcStatusAndGetFieldAsSizeT. 6. Since we introduce other process dumps from browser process there could be races while unregistering and dumping. To test this, the browser test is updated. BUG=461788 Committed: https://crrev.com/4d77d76a42425282b1a3c5b7309db9b98e777f60 Cr-Commit-Position: refs/heads/master@{#369482} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/4d77d76a42425282b1a3c5b7309db9b98e777f60 Cr-Commit-Position: refs/heads/master@{#369482}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:400001) has been created in https://codereview.chromium.org/1586893003/ by huangs@chromium.org. The reason for reverting is: This is suspected by Findit to cause failure in TracingBrowserTest.TestMemoryInfra under Mac ASan 64 Tests (1)..
Message was sent while issue was closed.
On 2016/01/14 20:54:52, huangs wrote: > A revert of this CL (patchset #15 id:400001) has been created in > https://codereview.chromium.org/1586893003/ by mailto:huangs@chromium.org. > > The reason for reverting is: This is suspected by Findit to cause failure in > TracingBrowserTest.TestMemoryInfra under Mac ASan 64 Tests (1).. Link to the failure? Usually you want to put in the revert message or in a bug attached to that when you revert. Please see https://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium
Message was sent while issue was closed.
On 2016/01/14 21:04:55, Primiano Tucci wrote: > On 2016/01/14 20:54:52, huangs wrote: > > A revert of this CL (patchset #15 id:400001) has been created in > > https://codereview.chromium.org/1586893003/ by mailto:huangs@chromium.org. > > > > The reason for reverting is: This is suspected by Findit to cause failure in > > TracingBrowserTest.TestMemoryInfra under Mac ASan 64 Tests (1).. > > Link to the failure? Usually you want to put in the revert message or in a bug > attached to that when you revert. > Please see > https://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium I can't see why this is related but i could find e463401bc88116d770ba6ed209f9066a84155a4c was related to gpu. But mine is more suspicious to cause errors. Link: https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests... |
