Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839733004:
Fill in PlatformPrivateFootprint on Linux (Closed)
DescriptionFill in PlatformPrivateFootprint on Linux 1/2
This CL populates the rss_anon_bytes field of the PlatformPrivateFootprint struct on Linux.
The struct contains platform specific numbers that will be used by the
memory-infra service to compute the private memory footprint of the
process.
BUG=707019
Review-Url: https://codereview.chromium.org/2839733004
Cr-Commit-Position: refs/heads/master@{#469968}
Committed: https://chromium.googlesource.com/chromium/src/+/90e0440fbcc073544c8645595268e641ad8e51a5
Patch Set 1 #Patch Set 2 : update for parent patch changes #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : update #Patch Set 5 : re add removed line #Patch Set 6 : address comments #
Total comments: 12
Patch Set 7 : address comments #Patch Set 8 : remove extra } #
Total comments: 4
Patch Set 9 : rebase #Patch Set 10 : address comments #Patch Set 11 : rebase #Patch Set 12 : move code behind defines #
Messages
Total messages: 29 (14 generated)
hjd@chromium.org changed reviewers: + erikchen@chromium.org, primiano@chromium.org
So to a first approximation we want something like this?
ptal, thanks! https://codereview.chromium.org/2839733004/diff/20001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2839733004/diff/20001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:636: #if defined(OS_LINUX) I need to try this out on the other two platforms to check it works.
ptal, thanks!
Description was changed from ========== Fill in ProcessMemoryTotals.private_footprint_precursor_ on Linux This CL populates the field on Linux. The field represents a platform specific number that will be used by the Memory-Infra service to compute the private memory footprint. BUG=707019 ========== to ========== Fill in PlatformPrivateFootprint on Linux This CL populates the PlatformPrivateFootprint struct on Linux. The field contains platform specific numbers that will be used by the memory-infra service to compute the private memory footprint of the process. BUG=707019 ==========
Thanks for being so proactive! can you plz do everything just in ProcessMetricsMemoryDumpProvider? Bunch of reasons for this: We shouldn't expose extra APIs in base just because we need it (see "Three Strikes and You Refactor" [1]). Is not just about refactoring cost. Is mostly because those memory API in base expose very small building blocks to the codebase with an ill defined semantic which a lot of people in the rest of the codebase end up misunderstanding and misusing. I don't want to keep contributing to this entropy (and one day I even want to clean up all that mess, but that's another story). Furthermore, from a robustness viewpoint, that code gets often refactored and given the aforementioned weak semantic, I wouldn't be surprised if some years from now somebody will change the implementation of the base API (concrete example https://codereview.chromium.org/2558043007 which I intercepted by accident from the watchlist). Considering that these memory API are nearly impossible to test (as they cause flaky tests), I'd really like to reduce the number of layers involved. Least but not last, there is code already there (see ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal()) which reads /proc/ files and I'd like to keep that model. There are subtleties there that will make the difference on android, like /proc/self vs /proc/$own_pid (yeah, they can have different selinux policies) [1] http://wiki.c2.com/?ThreeStrikesAndYouRefactor
On 2017/04/26 15:09:38, Primiano Tucci wrote: > Thanks for being so proactive! > can you plz do everything just in ProcessMetricsMemoryDumpProvider? > > Bunch of reasons for this: > We shouldn't expose extra APIs in base just because we need it (see "Three > Strikes and You Refactor" [1]). > Is not just about refactoring cost. Is mostly because those memory API in base > expose very small building blocks to the codebase with an ill defined semantic > which a lot of people in the rest of the codebase end up misunderstanding and > misusing. I don't want to keep contributing to this entropy (and one day I even > want to clean up all that mess, but that's another story). > Furthermore, from a robustness viewpoint, that code gets often refactored and > given the aforementioned weak semantic, I wouldn't be surprised if some years > from now somebody will change the implementation of the base API (concrete > example https://codereview.chromium.org/2558043007 which I intercepted by > accident from the watchlist). Considering that these memory API are nearly > impossible to test (as they cause flaky tests), I'd really like to reduce the > number of layers involved. > > Least but not last, there is code already there (see > ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal()) which reads /proc/ > files and I'd like to keep that model. There are subtleties there that will make > the difference on android, like /proc/self vs /proc/$own_pid (yeah, they can > have different selinux policies) > > [1] http://wiki.c2.com/?ThreeStrikesAndYouRefactor Okay updated the bit that reads statm! We still need to decide how we should read status.
Had a chat offline, statm is a bit tricky as we use it for fast polling. There
are conflicting requirements here, which are:
- in general we don't want to keep a file (statm) open just because we might
need it once in a while. Doing that causes one less FD in the system, and FDs
are a scarce resource because the limit is quite low.
- However when we do fast polling we actually have a good reason for keeping the
statm fd open, because we really need to be aggressive and doing open/close
every X ms introduces too much overhead.
I think the right way to integrate your use case here is:
1. Introduce a ScopedFD OpenStatm() which only purpose is opening
/proc/[self|pid]/statm (+ the testing stuff)
2. use the new OpenStatm in the PollFastMemoryTotal
3. In OnMemoryDump:
ScopedFD autoclose;
int statm_fd = polling_cached_fd_.get();
if (statm_fd < 0) {
autoclose = OpenStatm();
statm_fd = autoclose.get();
}
... do the stuff using statm_fd
Essentially, the logic of OnMemoryDump should sound like:
- if we already have the statm cached because of polling reuse it (but don't
close it, as doing that would make polling unhappy)
- if we don't have a cached statm, open a new one, dump the stuff, but then
close it.
updated, thanks!
Description was changed from ========== Fill in PlatformPrivateFootprint on Linux This CL populates the PlatformPrivateFootprint struct on Linux. The field contains platform specific numbers that will be used by the memory-infra service to compute the private memory footprint of the process. BUG=707019 ========== to ========== Fill in PlatformPrivateFootprint on Linux 1/2 This CL populates the rss_anon_bytes field of the PlatformPrivateFootprint struct on Linux. The struct contains platform specific numbers that will be used by the memory-infra service to compute the private memory footprint of the process. BUG=707019 ==========
https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:640: base::trace_event::ProcessMemoryTotals::PlatformPrivateFootprint& footprint = const auto& footprint = ...? https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:649: if (statm_fd != -1) { I'd just return false if statm_Fd == -1 here. Given the importance I am not sure we wanto silently ignore this failure. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:650: static size_t page_size = base::GetPageSize(); static const size_t kPageSize https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:655: if (success) { ditto, if(!success) return false; https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:659: #endif // defined(OS_LINUX) add a // TODO(hjd): implement swap in the next CL. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:723: if (statm_fd == -1) I think this is changing the semantic for the for_testing . the previous code was first checking for invalidity of the statm_fd (which is the for_testing) and then if -1 resorting to open
Updated, thanks! https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:640: base::trace_event::ProcessMemoryTotals::PlatformPrivateFootprint& footprint = On 2017/05/03 19:21:21, Primiano Tucci wrote: > const auto& footprint = ...? After this the line is *exactly* 80 chars => Best review comment ever! Although actually I think that it can't be const? We write to field below. :( https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:649: if (statm_fd != -1) { On 2017/05/03 19:21:21, Primiano Tucci wrote: > I'd just return false if statm_Fd == -1 here. Given the importance I am not > sure we wanto silently ignore this failure. Done. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:650: static size_t page_size = base::GetPageSize(); On 2017/05/03 19:21:22, Primiano Tucci wrote: > static const size_t kPageSize Done. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:655: if (success) { On 2017/05/03 19:21:21, Primiano Tucci wrote: > ditto, if(!success) return false; Done. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:659: #endif // defined(OS_LINUX) On 2017/05/03 19:21:21, Primiano Tucci wrote: > add a // TODO(hjd): implement swap in the next CL. Done. https://codereview.chromium.org/2839733004/diff/100001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:723: if (statm_fd == -1) On 2017/05/03 19:21:21, Primiano Tucci wrote: > I think this is changing the semantic for the for_testing . > the previous code was first checking for invalidity of the statm_fd (which is > the for_testing) and then if -1 resorting to open Done, thanks!
thanks LGTM https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:730: uint64_t rss_pages = 0; at this point call this "private_pages" for consistency https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:731: uint64_t shared_pages = 0; and I'd call this "ignored" (or ignored_shared_pages, up to you) to document that we actually mean to ignore it
Updated https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:730: uint64_t rss_pages = 0; On 2017/05/04 11:11:16, Primiano Tucci wrote: > at this point call this "private_pages" for consistency thanks, Used resident_pages to match arg name https://codereview.chromium.org/2839733004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:731: uint64_t shared_pages = 0; On 2017/05/04 11:11:16, Primiano Tucci wrote: > and I'd call this "ignored" (or ignored_shared_pages, up to you) to document > that we actually mean to ignore it Done.
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2839733004/#ps180001 (title: "address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hjd@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...
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 hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2839733004/#ps220001 (title: "move code behind defines")
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": 220001, "attempt_start_ts": 1494248183827070,
"parent_rev": "78e1fd24e94610e1821b0173b5a53302219664fe", "commit_rev":
"90e0440fbcc073544c8645595268e641ad8e51a5"}
Message was sent while issue was closed.
Description was changed from ========== Fill in PlatformPrivateFootprint on Linux 1/2 This CL populates the rss_anon_bytes field of the PlatformPrivateFootprint struct on Linux. The struct contains platform specific numbers that will be used by the memory-infra service to compute the private memory footprint of the process. BUG=707019 ========== to ========== Fill in PlatformPrivateFootprint on Linux 1/2 This CL populates the rss_anon_bytes field of the PlatformPrivateFootprint struct on Linux. The struct contains platform specific numbers that will be used by the memory-infra service to compute the private memory footprint of the process. BUG=707019 Review-Url: https://codereview.chromium.org/2839733004 Cr-Commit-Position: refs/heads/master@{#469968} Committed: https://chromium.googlesource.com/chromium/src/+/90e0440fbcc073544c8645595268... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/90e0440fbcc073544c8645595268... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
