Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740423002:
mac: Fix calulation for number of resident pages in a process. (Closed)
Descriptionmac: Fix calulation for resident size of a process.
The previous implementation of GetWorkingSetSize used
task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the
pmap for the vm subsystem for the process. This fails to correctly account for
resident pages of copy-on-write and shared memory memory regions, as the pages
might be mapped by the pmap for another process.
In passing, this CL fixes a typo in process_metrics_memory_dump_provider.cc,
which used defined(MACOSX) instead of defined(OS_MACOSX).
BUG=700532, 693263
Review-Url: https://codereview.chromium.org/2740423002
Cr-Commit-Position: refs/heads/master@{#456514}
Committed: https://chromium.googlesource.com/chromium/src/+/56722415bddf6b1eb88f6e7fb9e9d4df7f21b9a1
Patch Set 1 #
Total comments: 11
Patch Set 2 : Comments from mark, primiano. #
Total comments: 2
Patch Set 3 : comments from mark. #
Messages
Total messages: 36 (19 generated)
Description was changed from ========== mac: Fix calulation for number of resident pages in a process. <todo> BUG=700532 ========== to ========== mac: Fix calulation for number of resident pages in a process. <todo> BUG=700532, 693263 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== mac: Fix calulation for number of resident pages in a process. <todo> BUG=700532, 693263 ========== to ========== mac: Fix calulation for resident size of a process. The previous implementation of GetWorkingSetSize used task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the pmap for the vm subsystem for the process. This fails to correctly account for resident pages of copy-on-write and shared memory memory regions, as the pages might be mapped by the pmap for another process. BUG=700532, 693263 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
erikchen@chromium.org changed reviewers: + mark@chromium.org, primiano@chromium.org
mark, primiano: Please review.
Not too many ideas on the various states on memory pages on OSX, but I trust that a combination of you and mark can come with the right answer there. components/tracing/ LGTM with just a small comment. https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:607: pmd->process_totals()->SetExtraFieldInBytes("private_bytes", private_bytes); does this shows up in the UI as is or we need an update in traceviewer? (in the latter case contact hjd@) https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:612: : process_metrics_->GetWorkingSetSize(); maybe you can factor out the ? rss_bytes_for_testing : rss_bytes_for_testing and just keep a var assignment in the #ifdef?
LGTM either way though https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = Leaving this alone was nice because it consolidates all of our “what is working set size?” logic in the Mac implementation file. https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:606: : private_bytes + shared_bytes; Same here.
ssid@chromium.org changed reviewers: + ssid@chromium.org
Thanks for fixing this https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = On 2017/03/13 18:13:33, Mark Mentovai wrote: > Leaving this alone was nice because it consolidates all of our “what is working > set size?” logic in the Mac implementation file. Yes, the main consumer of this number I'd expect would be the users and IIUC they'd see different number in Chrome task manager and system task manager.
https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = ssid wrote: > On 2017/03/13 18:13:33, Mark Mentovai wrote: > > Leaving this alone was nice because it consolidates all of our “what is > working > > set size?” logic in the Mac implementation file. > > Yes, the main consumer of this number I'd expect would be the users and IIUC > they'd see different number in Chrome task manager and system task manager. What I meant was “use the new logic, just consolidate it.” Are you suggesting “use the old logic” for one or more use sites?
Also is the bug 693263 related? https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = On 2017/03/13 18:48:03, Mark Mentovai wrote: > ssid wrote: > > On 2017/03/13 18:13:33, Mark Mentovai wrote: > > > Leaving this alone was nice because it consolidates all of our “what is > > working > > > set size?” logic in the Mac implementation file. > > > > Yes, the main consumer of this number I'd expect would be the users and IIUC > > they'd see different number in Chrome task manager and system task manager. > > What I meant was “use the new logic, just consolidate it.” Are you suggesting > “use the old logic” for one or more use sites? Um sorry, yes I just felt showing different number in Chrome task manager and system's might be get bad? It's just an opinion, I am fine with the change.
Also is the bug 693263 related? https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = On 2017/03/13 18:48:03, Mark Mentovai wrote: > ssid wrote: > > On 2017/03/13 18:13:33, Mark Mentovai wrote: > > > Leaving this alone was nice because it consolidates all of our “what is > > working > > > set size?” logic in the Mac implementation file. > > > > Yes, the main consumer of this number I'd expect would be the users and IIUC > > they'd see different number in Chrome task manager and system task manager. > > What I meant was “use the new logic, just consolidate it.” Are you suggesting > “use the old logic” for one or more use sites? Um sorry, yes I just felt showing different number in Chrome task manager and system's might be get bad? It's just an opinion, I am fine with the change.
On 2017/03/13 18:59:15, ssid wrote: > Also is the bug 693263 related? Issue 693263 is a boring ssl CQ bug.
Description was changed from ========== mac: Fix calulation for resident size of a process. The previous implementation of GetWorkingSetSize used task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the pmap for the vm subsystem for the process. This fails to correctly account for resident pages of copy-on-write and shared memory memory regions, as the pages might be mapped by the pmap for another process. BUG=700532, 693263 ========== to ========== mac: Fix calulation for resident size of a process. The previous implementation of GetWorkingSetSize used task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the pmap for the vm subsystem for the process. This fails to correctly account for resident pages of copy-on-write and shared memory memory regions, as the pages might be mapped by the pmap for another process. In passing, this CL fixes a typo in process_metrics_memory_dump_provider.cc, which used defined(MACOSX) instead of defined(OS_MACOSX). BUG=700532, 693263 ==========
https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group_sampler.cc (left): https://codereview.chromium.org/2740423002/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group_sampler.cc:129: memory_usage.physical_bytes = On 2017/03/13 18:59:14, ssid wrote: > On 2017/03/13 18:48:03, Mark Mentovai wrote: > > ssid wrote: > > > On 2017/03/13 18:13:33, Mark Mentovai wrote: > > > > Leaving this alone was nice because it consolidates all of our “what is > > > working > > > > set size?” logic in the Mac implementation file. > > > > > > Yes, the main consumer of this number I'd expect would be the users and IIUC > > > they'd see different number in Chrome task manager and system task manager. > > > > What I meant was “use the new logic, just consolidate it.” Are you > suggesting > > “use the old logic” for one or more use sites? > > Um sorry, yes I just felt showing different number in Chrome task manager and > system's might be get bad? It's just an opinion, I am fine with the change. Calculating private/shared bytes has a performance cost [order of magnitude: 1ms]. Keeping the existing API would require running that calculation twice. I'll add a new macOS-only function to base/process/process_metrics.h that returns private bytes, shared privates, and working set size. https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:606: : private_bytes + shared_bytes; On 2017/03/13 18:13:33, Mark Mentovai wrote: > Same here. Done. https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:607: pmd->process_totals()->SetExtraFieldInBytes("private_bytes", private_bytes); On 2017/03/13 13:59:34, Primiano Tucci (slow - perf) wrote: > does this shows up in the UI as is or we need an update in traceviewer? (in the > latter case contact hjd@) Yes it does. I forgot to include this in the original description, but the original code was never executed, since there was a typo in the preprocessor conditional. I've updated the CL description to indicate my fix. https://codereview.chromium.org/2740423002/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:612: : process_metrics_->GetWorkingSetSize(); On 2017/03/13 13:59:34, Primiano Tucci (slow - perf) wrote: > maybe you can factor out the ? rss_bytes_for_testing : rss_bytes_for_testing and > just keep a var assignment in the #ifdef? Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2740423002/#ps20001 (title: "Comments from mark, primiano.")
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2740423002/diff/20001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2740423002/diff/20001/base/process/process_me... base/process/process_metrics_mac.cc:117: return private_bytes + shared_bytes; Call the 3-arg GetMemoryBytes() and return the third arg.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2740423002/diff/20001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2740423002/diff/20001/base/process/process_me... base/process/process_metrics_mac.cc:117: return private_bytes + shared_bytes; On 2017/03/13 21:05:08, Mark Mentovai wrote: > Call the 3-arg GetMemoryBytes() and return the third arg. Done.
erikchen@chromium.org changed reviewers: + nick@chromium.org
nick: Please review chrome/browser/task_manager.
lgtm
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2740423002/#ps40001 (title: "comments from mark.")
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": 40001, "attempt_start_ts": 1489440758793570,
"parent_rev": "8e4878fdb12d295c90bbfe58adf3d87016561695", "commit_rev":
"56722415bddf6b1eb88f6e7fb9e9d4df7f21b9a1"}
Message was sent while issue was closed.
Description was changed from ========== mac: Fix calulation for resident size of a process. The previous implementation of GetWorkingSetSize used task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the pmap for the vm subsystem for the process. This fails to correctly account for resident pages of copy-on-write and shared memory memory regions, as the pages might be mapped by the pmap for another process. In passing, this CL fixes a typo in process_metrics_memory_dump_provider.cc, which used defined(MACOSX) instead of defined(OS_MACOSX). BUG=700532, 693263 ========== to ========== mac: Fix calulation for resident size of a process. The previous implementation of GetWorkingSetSize used task_info(TASK_BASIC_INFO_64), which returns the number of resident pages in the pmap for the vm subsystem for the process. This fails to correctly account for resident pages of copy-on-write and shared memory memory regions, as the pages might be mapped by the pmap for another process. In passing, this CL fixes a typo in process_metrics_memory_dump_provider.cc, which used defined(MACOSX) instead of defined(OS_MACOSX). BUG=700532, 693263 Review-Url: https://codereview.chromium.org/2740423002 Cr-Commit-Position: refs/heads/master@{#456514} Committed: https://chromium.googlesource.com/chromium/src/+/56722415bddf6b1eb88f6e7fb9e9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/56722415bddf6b1eb88f6e7fb9e9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
