Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568313004:
[memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider. (Closed)
|
Created:
4 years ago by ssid Modified:
4 years ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider.
For fast polling the dump provider keeps the /proc/<pid>/statm files
open during the tracing session.
BUG=607533
Committed: https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6
Cr-Commit-Position: refs/heads/master@{#439405}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Fixes. #Patch Set 3 : Remove memset. #
Total comments: 8
Patch Set 4 : Fixes. #
Total comments: 8
Patch Set 5 : ScopedFD #Patch Set 6 : nits. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 52 (39 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, this is the 2nd part of the split.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Thanks a lot for splitting, way easier to review. Just minor comments. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:35: const int kInvalidFD = -1; honestly I think that directly using -1 makes the code more readable (especially in the >= 0 condition down below in SetEnabled) https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:166: bool GetResidentSizesFromStatmFile(int file_des, s/file_des/fd/ https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:175: base::CStringTokenizer t(line, line + strlen(line), " "); hmm CStringTokenizer will implicitly convert line to a std::string (so make a copy on the heap). Unfortunately there doesn't seem to be any Tokenizer that can be used on a StringPiece. How about a good old int res = sscanf(line, "%*zu %zu %zu", resident_bytes, shared_bytes) ? if (res != 2) return false; which will reduce both the code and the runtime of this? https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:203: base::LazyInstance< is there a functional reason why you moved this? IIRC the code style variable declarations should be above function blocks. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:366: *memory_total = (process_ == base::kNullProcessId ? rss : rss - shared) * hmm not sure I am convinced here. The math should change depending on which process you dump from. This is just an implementation detail about the sandbox workaround. I think always counting priv+shared is easier to reason about. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:376: if (proc_statm_file_ >= 0) Ok I see how doing the auto-opening makes things easier. But at this point just call this function SetFastMemoryPollingDisabled. no reason to pass a boolean that you don't use. Also ironically now this is called set...enabled but you call it only for disabling :) https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:36: void PollFastMemoryTotal(uint64_t* memory_total) override; maybe give this argument a more descriptive name, dunno, approximate_resident_bytes ? https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:56: void ClearProcStatmFile(); remove this, not defined anywhere, I guess it's a leftover https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:60: int proc_statm_file_; fast_polling_statm_fd_ https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:242: for (int i = 0; i < 4; i++) { I think a slighly better approach would be: PollFastMemoryTotal(&total1) kBufSize = 16 * 1024 * 1024; auto x = MakeUnique<char[]>(kBufSize); memset(x.get, 0x42, kFourMB); PollFastMemoryTotal(&total2); EXPECT_GT(total2 - total1, kBufSize / 2) The rationale of the /2 in the test is: gtest in the meantime could do some frees that make the difference not exactly 16 MB. If you see an increment of at least half you call it a day. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:247: EXPECT_GT(mdp.proc_statm_file_, 0); s/GT/GE/. In an extremely unlikely scenario, one might close all file descriptors and if yours is the first open() will get fd 0. will almost never happen, but for pure sake of consistency this should be >= 0
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #3 (id:40001) has been deleted
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/v2/patch-status/codereview.chromium.or...
fixed cl, ptal. Thanks https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:35: const int kInvalidFD = -1; On 2016/12/13 20:02:39, Primiano Tucci wrote: > honestly I think that directly using -1 makes the code more readable (especially > in the >= 0 condition down below in SetEnabled) Done. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:166: bool GetResidentSizesFromStatmFile(int file_des, On 2016/12/13 20:02:39, Primiano Tucci wrote: > s/file_des/fd/ Done. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:175: base::CStringTokenizer t(line, line + strlen(line), " "); On 2016/12/13 20:02:39, Primiano Tucci wrote: > hmm CStringTokenizer will implicitly convert line to a std::string (so make a > copy on the heap). Unfortunately there doesn't seem to be any Tokenizer that can > be used on a StringPiece. > How about a good old > int res = sscanf(line, "%*zu %zu %zu", resident_bytes, shared_bytes) ? > if (res != 2) > return false; > > which will reduce both the code and the runtime of this? Makes sense. fixed. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:203: base::LazyInstance< On 2016/12/13 20:02:39, Primiano Tucci wrote: > is there a functional reason why you moved this? IIRC the code style variable > declarations should be above function blocks. Sorry this was a mistake. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:366: *memory_total = (process_ == base::kNullProcessId ? rss : rss - shared) * On 2016/12/13 20:02:39, Primiano Tucci wrote: > hmm not sure I am convinced here. > The math should change depending on which process you dump from. This is just an > implementation detail about the sandbox workaround. > I think always counting priv+shared is easier to reason about. Hm but we will miss the shared memory increases if we leave shared from all processes. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:376: if (proc_statm_file_ >= 0) On 2016/12/13 20:02:39, Primiano Tucci wrote: > Ok I see how doing the auto-opening makes things easier. But at this point just > call this function SetFastMemoryPollingDisabled. > no reason to pass a boolean that you don't use. > Also ironically now this is called set...enabled but you call it only for > disabling :) It is weird to have SetFastMemoryPollingDisabled method in the MDP interface. I think it is better to have it as SetFastMemoryPollingEnabled. I was using an |is_final_poll| argument in PollFastMemoryTotal instead, if we watn to keep the number of methods in MDP lesser. Either ways discussion in https://codereview.chromium.org/2537363003/diff/260001/base/trace_event/memor... Fixed this to clear only on disable. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:36: void PollFastMemoryTotal(uint64_t* memory_total) override; On 2016/12/13 20:02:39, Primiano Tucci wrote: > maybe give this argument a more descriptive name, dunno, > approximate_resident_bytes ? Please note this patch depends on that. And, this has to be addressed by https://codereview.chromium.org/2537363003/diff/260001/base/trace_event/memor... https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:56: void ClearProcStatmFile(); On 2016/12/13 20:02:39, Primiano Tucci wrote: > remove this, not defined anywhere, I guess it's a leftover Sorry fixed. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.h:60: int proc_statm_file_; On 2016/12/13 20:02:39, Primiano Tucci wrote: > fast_polling_statm_fd_ Done. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:242: for (int i = 0; i < 4; i++) { On 2016/12/13 20:02:39, Primiano Tucci wrote: > I think a slighly better approach would be: > PollFastMemoryTotal(&total1) > kBufSize = 16 * 1024 * 1024; > auto x = MakeUnique<char[]>(kBufSize); > memset(x.get, 0x42, kFourMB); > PollFastMemoryTotal(&total2); > EXPECT_GT(total2 - total1, kBufSize / 2) > > The rationale of the /2 in the test is: gtest in the meantime could do some > frees that make the difference not exactly 16 MB. If you see an increment of at > least half you call it a day. Done. https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:247: EXPECT_GT(mdp.proc_statm_file_, 0); On 2016/12/13 20:02:39, Primiano Tucci wrote: > s/GT/GE/. > In an extremely unlikely scenario, one might close all file descriptors and if > yours is the first open() will get fd 0. > will almost never happen, but for pure sake of consistency this should be >= 0 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #2 (id:20001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:242: for (int i = 0; i < 4; i++) { On 2016/12/14 02:59:32, ssid wrote: > On 2016/12/13 20:02:39, Primiano Tucci wrote: > > I think a slighly better approach would be: > > PollFastMemoryTotal(&total1) > > kBufSize = 16 * 1024 * 1024; > > auto x = MakeUnique<char[]>(kBufSize); > > memset(x.get, 0x42, kFourMB); > > PollFastMemoryTotal(&total2); > > EXPECT_GT(total2 - total1, kBufSize / 2) > > > > The rationale of the /2 in the test is: gtest in the meantime could do some > > frees that make the difference not exactly 16 MB. If you see an increment of > at > > least half you call it a day. > Done. Release builds do not make the memory resident even after memset. I have to add this line to make it resident: for (int i = 0; i < kBufSize; i += (buf[i] == 0x42); Removed this bit and testing the parsing code with temp file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
> Release builds do not make the memory resident even after memset. I have to add this line to make it resident: for (int i = 0; i < kBufSize; i += (buf[i] == 0x42); Removed this bit and testing the parsing code with temp file. Oh I think I know what the problem is. It is not memset. It's that the compiler is too smart in release mode. If you malloc (indirectly via MakeUnique) write but never read, the compiler will say "well, you don't really need this memory then" and will optimize away all the code. I ran into something like this in the past in a similar test. The way to force the compiler is: - create the buffer - memset it - read the first and last element via a (volatile char*) cast. Or more simply, do a loop where you read and write using a volatile ptr, i.e.: for (size_t i=0; i < ksize; i++) buf[i] = *((volatile char*)&buf[i]) + 1; https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:366: *memory_total = (process_ == base::kNullProcessId ? rss : rss - shared) * On 2016/12/14 02:59:32, ssid wrote: > On 2016/12/13 20:02:39, Primiano Tucci wrote: > > hmm not sure I am convinced here. > > The math should change depending on which process you dump from. This is just > an > > implementation detail about the sandbox workaround. > > I think always counting priv+shared is easier to reason about. > > Hm but we will miss the shared memory increases if we leave shared from all > processes. I was suggesting the other way round, *always*count shared, i.e. don't have this special path that subtracts it. The reason is the following: the "shared" field from /proc/.../statm is different from the "shared" that you get from /proc/.../smaps the one in statm is really "share-able". It doesn't count the fact that pages are really shared or not (that's one of the reasons why reading statm is so fast). For statm the definition of "shared" is simply: "if the memory map is backed by a file, i.e. not anonymous" This means that if you have a scenario where a child process is the only one using a /ashmem segment, you will not count it here with this code (but you will if you remove the "- shared" part). IMHO better double count (if you have the same in browser and renderer) than not count at all. The real downside (that we knew since the beginning of times) is that this "shared" counter will also count clean memory from mmaped filed. So if somebody mmaps() a file read-only you will see an increase. On the other side, our shared memory is based on files. So IMHO better be pessimistic, and count it, than ignore shared memory at all (Which is what you are doing here, if you don't to this discount for child processes) https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:159: int OpenStatmFile(base::ProcessId process) { Since you kept the full SetFastPollingEnabled(bool) signature, at this point I think you can remove this funciton and move this code into the "bool = true" branch of SetFastPollingEnabled, so your code in ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal will look like: ... if (fast_polling_statm_fd_ == -1) SetFastPollingEnabled(true); ... https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:172: int res = read(fd, line, kMaxLineSize); if you do this, then here you have to pass kMaxLineSize - 1. THere is a subtle bug (though very unrealistic) here: if read manages to fill the buffer, and hence return kMaxLineSize, line 175 will be an out of bound access ;-) https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.h:58: void set_fast_polling_statm_fd_for_testing(int fd) { maybe use the same pattern as above: static int proc_statm_for_testing; and when you get to open the file, check if this field is non-zero. THe current design is a bit odd because you first have to open the real /proc/xxx/statm file and *then* override with this function. https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:242: mdp.SetFastMemoryPollingEnabled(true); i'd remove this line to make sure that the file is lazily opened anyways
On 2016/12/15 15:00:02, Primiano Tucci wrote: > > Release builds do not make the memory resident even after memset. > I have to add this line to make it resident: > for (int i = 0; i < kBufSize; i += (buf[i] == 0x42); > Removed this bit and testing the parsing code with temp file. > > Oh I think I know what the problem is. It is not memset. > It's that the compiler is too smart in release mode. If you malloc (indirectly > via MakeUnique) write but never read, the compiler will say "well, you don't > really need this memory then" and will optimize away all the code. I ran into > something like this in the past in a similar test. > The way to force the compiler is: > - create the buffer > - memset it > - read the first and last element via a (volatile char*) cast. > > Or more simply, do a loop where you read and write using a volatile ptr, i.e.: > for (size_t i=0; i < ksize; i++) > buf[i] = *((volatile char*)&buf[i]) + 1; But, do we really need to test if GetWorkingSetSize is working? This is already tested in ProcessMetrics. The only change that is happening here is on Linux and Android where we read the statm file. This can be tested by the test fd. Do you think that is not sufficient? https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/1/components/tracing/common/p... components/tracing/common/process_metrics_memory_dump_provider.cc:366: *memory_total = (process_ == base::kNullProcessId ? rss : rss - shared) * On 2016/12/15 15:00:02, Primiano Tucci wrote: > On 2016/12/14 02:59:32, ssid wrote: > > On 2016/12/13 20:02:39, Primiano Tucci wrote: > > > hmm not sure I am convinced here. > > > The math should change depending on which process you dump from. This is > just > > an > > > implementation detail about the sandbox workaround. > > > I think always counting priv+shared is easier to reason about. > > > > Hm but we will miss the shared memory increases if we leave shared from all > > processes. > > I was suggesting the other way round, *always*count shared, i.e. don't have this > special path that subtracts it. > The reason is the following: the "shared" field from /proc/.../statm is > different from the "shared" that you get from /proc/.../smaps > the one in statm is really "share-able". It doesn't count the fact that pages > are really shared or not (that's one of the reasons why reading statm is so > fast). > For statm the definition of "shared" is simply: "if the memory map is backed by > a file, i.e. not anonymous" > This means that if you have a scenario where a child process is the only one > using a /ashmem segment, you will not count it here with this code (but you will > if you remove the "- shared" part). > IMHO better double count (if you have the same in browser and renderer) than not > count at all. > > The real downside (that we knew since the beginning of times) is that this > "shared" counter will also count clean memory from mmaped filed. So if somebody > mmaps() a file read-only you will see an increase. > On the other side, our shared memory is based on files. So IMHO better be > pessimistic, and count it, than ignore shared memory at all (Which is what you > are doing here, if you don't to this discount for child processes) > makes sense. Changed to just resident size. https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:159: int OpenStatmFile(base::ProcessId process) { On 2016/12/15 15:00:02, Primiano Tucci wrote: > Since you kept the full SetFastPollingEnabled(bool) signature, at this point I > think you can remove this funciton and move this code into the "bool = true" > branch of SetFastPollingEnabled, so your code in > ProcessMetricsMemoryDumpProvider::PollFastMemoryTotal will look like: > > ... > if (fast_polling_statm_fd_ == -1) > SetFastPollingEnabled(true); > ... Done. https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:172: int res = read(fd, line, kMaxLineSize); On 2016/12/15 15:00:02, Primiano Tucci wrote: > if you do this, then here you have to pass kMaxLineSize - 1. > THere is a subtle bug (though very unrealistic) here: if read manages to fill > the buffer, and hence return kMaxLineSize, line 175 will be an out of bound > access ;-) Done. https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.h:58: void set_fast_polling_statm_fd_for_testing(int fd) { On 2016/12/15 15:00:02, Primiano Tucci wrote: > maybe use the same pattern as above: > > static int proc_statm_for_testing; > > and when you get to open the file, check if this field is non-zero. > THe current design is a bit odd because you first have to open the real > /proc/xxx/statm file and *then* override with this function. Done. https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2568313004/diff/120001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:242: mdp.SetFastMemoryPollingEnabled(true); On 2016/12/15 15:00:02, Primiano Tucci wrote: > i'd remove this line to make sure that the file is lazily opened anyways now its needed.
LGTM with some final comments (sorry I realized about ScopedFD only now). On 2016/12/16 03:16:41, ssid wrote: > But, do we really need to test if GetWorkingSetSize is working? This is already > tested in ProcessMetrics. > The only change that is happening here is on Linux and Android where we read the > statm file. This can be tested by the test fd. Do you think that is not > sufficient? No I think it's allright. You testing strategy makes sense. https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:363: *memory_total = rss_pages * base::GetPageSize(); base::GetPageSize() is a bit silly because makes a call to getpagesize() all the times. I think you can optimize it a little bit by doing: static size_t page_size = base::GetPageSize(); (it should be fine w.r.t. static initializers as local static do not create initializers, because they are set the first time they are hit) https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:373: DCHECK(fast_polling_statm_fd_); I think you really mean here: DCHECK_EQ(-1, fast_polling_statm_fd_); otherwise this dcheck will pass all the times regardless: -1 is still true, and 0 here is extremely unlikely https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:378: fast_polling_statm_fd_ = open(name.c_str(), O_RDONLY); I'd add a DCHECK_GE(fast_polling_statm_fd_, 0); at this point, so if one day we get sandbox problems again we'll realize. https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.h:59: int fast_polling_statm_fd_; I'm sorry I realized only now. This should be a base::ScopedFD, it's safer w.r.t. risking of leaking fds.
Patchset #5 (id:160001) has been deleted
Fixed. thanks https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:363: *memory_total = rss_pages * base::GetPageSize(); On 2016/12/16 11:50:40, Primiano Tucci wrote: > base::GetPageSize() is a bit silly because makes a call to getpagesize() all the > times. > I think you can optimize it a little bit by doing: > static size_t page_size = base::GetPageSize(); > (it should be fine w.r.t. static initializers as local static do not create > initializers, because they are set the first time they are hit) > Done. https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:373: DCHECK(fast_polling_statm_fd_); On 2016/12/16 11:50:40, Primiano Tucci wrote: > I think you really mean here: > DCHECK_EQ(-1, fast_polling_statm_fd_); > otherwise this dcheck will pass all the times regardless: -1 is still true, and > 0 here is extremely unlikely Yes i meant eq -1. But removed this now. https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:378: fast_polling_statm_fd_ = open(name.c_str(), O_RDONLY); On 2016/12/16 11:50:40, Primiano Tucci wrote: > I'd add a DCHECK_GE(fast_polling_statm_fd_, 0); > at this point, so if one day we get sandbox problems again we'll realize. Done. https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.h (right): https://codereview.chromium.org/2568313004/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.h:59: int fast_polling_statm_fd_; On 2016/12/16 11:50:40, Primiano Tucci wrote: > I'm sorry I realized only now. This should be a base::ScopedFD, it's safer > w.r.t. risking of leaking fds. Done.
LGTM There is just some small inconsistency with the fact that sometime you use <0 some other times ==-1
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/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 ssid@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/2568313004/#ps200001 (title: "nits.")
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 ssid@chromium.org
Patchset #6 (id:200001) has been deleted
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 Link to the patchset: https://codereview.chromium.org/2568313004/#ps220001 (title: "nits.")
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": 1482117342021280,
"parent_rev": "324a4ed62e5e6f4b1cd47bfe248d798c35ccbd2f", "commit_rev":
"86a44419a3f37a27d5f22242125d8926b427d258"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider. For fast polling the dump provider keeps the /proc/<pid>/statm files open during the tracing session. BUG=607533 ========== to ========== [memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider. For fast polling the dump provider keeps the /proc/<pid>/statm files open during the tracing session. BUG=607533 Review-Url: https://codereview.chromium.org/2568313004 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider. For fast polling the dump provider keeps the /proc/<pid>/statm files open during the tracing session. BUG=607533 Review-Url: https://codereview.chromium.org/2568313004 ========== to ========== [memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider. For fast polling the dump provider keeps the /proc/<pid>/statm files open during the tracing session. BUG=607533 Committed: https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6 Cr-Commit-Position: refs/heads/master@{#439405} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6 Cr-Commit-Position: refs/heads/master@{#439405} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
