Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2549803003:
Add function to compute proportional set size for OS_WIN (Closed)
DescriptionAdd function to compute proportional set size for OS_WIN
Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet.
This CL added functionality to calculate proportional set size for a process.
BUG=623499
Committed: https://crrev.com/b172892773fe15a10508f733ca0bc831240a7ec6
Cr-Commit-Position: refs/heads/master@{#437827}
Patch Set 1 #
Total comments: 6
Patch Set 2 : More encapsulation of code, better comment. #
Total comments: 17
Patch Set 3 : Fix small bugs and more code improvement #
Total comments: 1
Patch Set 4 : Fix functionality #
Total comments: 22
Patch Set 5 : Fix a few nits #Patch Set 6 : Better names for variables and functions #
Total comments: 5
Patch Set 7 : Make variable type consistent #
Total comments: 2
Patch Set 8 : Fix some comments #Patch Set 9 : Fix compile error for std::min #
Messages
Total messages: 76 (55 generated)
The CQ bit was checked by chengx@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 checked by chengx@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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add function to compute proportional set size for OS_WIN BUG=623499 ========== to ========== Add function to compute proportional set size for OS_WIN Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet. This CL added functionality to calculate proportional set size for a process. BUG=623499 ==========
chengx@chromium.org changed reviewers: + brucedawson@chromium.org
PTAL~
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks reasonable, modulo the comments below. Is this returning all of the information of interest? Or is dirty page counts and other information wanted? https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics.h:148: // Computes pss (proportional set size) of a process. The comment about cost from a few lines above should probably be copied over since GetProportionalSetSizeBytes also costs a few ms per process. https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics_win.cc:175: bool GetPageEntriesNum(WorkingSetInformationBuffer& buffer, The name of this function is misleading. It sounds like it will just get the number of page entries, but it actually gets the page entries (getting the number of them is part of this). If you remove 'Num' from the name then I think it is more accurate. Also, I believe the style guide prohibits non-const references. So, you need to use a pointer to buffer instead. https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics_win.cc:226: DWORD number_of_entries = 4096; // Just a guess. Probably no need to put this guess here. Instead have GetPageEntriesNum do the initial guess. Maybe put number_of_entries as par of WorkingSetInformationBuffer - better encapsulation.
On 2016/12/05 19:22:02, brucedawson wrote: > Looks reasonable, modulo the comments below. > > Is this returning all of the information of interest? Or is dirty page counts > and other information wanted? Short answer: no. Linux OS provides a lot of information as in ProcessMomeryMaps class, https://cs.chromium.org/chromium/src/base/trace_event/process_memory_maps.h?c... Currently, I only know how to get Pss for OS_Win. For other information like dirty page counts, I am not sure how to get it. We can discuss offline.
The CQ bit was checked by chengx@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...
chengx@chromium.org changed reviewers: + primiano@chromium.org
This CL is ready for review. PTAL~ https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics.h:148: // Computes pss (proportional set size) of a process. On 2016/12/05 19:22:02, brucedawson wrote: > The comment about cost from a few lines above should probably be copied over > since GetProportionalSetSizeBytes also costs a few ms per process. Done. https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics_win.cc:175: bool GetPageEntriesNum(WorkingSetInformationBuffer& buffer, On 2016/12/05 19:22:02, brucedawson wrote: > The name of this function is misleading. It sounds like it will just get the > number of page entries, but it actually gets the page entries (getting the > number of them is part of this). If you remove 'Num' from the name then I think > it is more accurate. > > Also, I believe the style guide prohibits non-const references. So, you need to > use a pointer to buffer instead. Done. https://codereview.chromium.org/2549803003/diff/20001/base/process/process_me... base/process/process_metrics_win.cc:226: DWORD number_of_entries = 4096; // Just a guess. On 2016/12/05 19:22:02, brucedawson wrote: > Probably no need to put this guess here. Instead have GetPageEntriesNum do the > initial guess. > > Maybe put number_of_entries as par of WorkingSetInformationBuffer - better > encapsulation. Done.
A few nits but otherwise lgtm. I defer to primiano for whether this is sufficient to hook it up for the memory bots. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:160: DWORD GetPageEntriesNumber() { return number_of_entries; } Should be const. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:217: DWORD number_of_entries; Could/should go "= 0;" here rather than listing the initialization in the constructor. New C++11 feature, and consistent with how buffer_ is initialized. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < buffer.GetPageEntriesNumber(); i++) { Should generally avoid calling functions like this in the loop. Not really important in this case, but it can make life harder for the optimizer. Ditto for loop below. Could also be very trendy and use begin()/end() functions and range-based for loops - ask me for details. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:262: ws_pss += 1.0f / buffer->WorkingSetInfo[i].ShareCount; Remove trailing 'f' from this line and line 264. The 'f' on this line may actively harm precision by triggering float-precision division instead of double-precision.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for championing this. Definitely having a PSS number is the key things for the bots. Just a question if we can avoid doing two QueryWorkingSet back to back in the same function (See comment inline below) More general comment (read: NOT for this CL): On linux we manage to grab the full mmaps table. That is not strictly required for the perf dashboard itself, but in the past helped a lot to diagnose a number of regressions and just bugs (you can download traces and inspect them manually in chrome://tracing from the chromeperf dashboard), for instance seeing a lot of more fonts being keept around. I wonder if something similar could be done on Windows. I guess that a combination of VirtualQueryEx + GetMappedFileName + QueryWorkingSetEx might do the job? https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:198: // the I think clang-format did bite you here (It's a KI it's not that good in re-wrapping pre-wrapped comments) https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:267: *pss_bytes = static_cast<uint64_t>(ws_pss); 1) isn't this missing a * 4096 ? As the name suggests these are bytes, but you are adding 1 (or a fraction) . 2) Are all pages 4096 bytes on Windows? https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:307: base::trace_event::ProcessMemoryMaps::VMRegion region; If I read the code correctly now within this function we are grabbing and iterating over WorkingSetInformationBuffer twice: once up in line 268 upon GetWorkingSetSize() and once here. Is there a way we could expose/wrap the WorkingSetInformationBuffer, or the result of postprocessing it? https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:313: #endif // defined(OS_LINUX) || defined(OS_ANDROID) plz can you remove the comment from this #endif as it doesn't apply anymore after the various #else branches?
The CQ bit was checked by chengx@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...
Thanks for the comments! Primiano@, I don't think two QueryWorkingSet() are called back to back. Please see my inline reply to yours in the CL. Please correct me if I am wrong. Yes, on linux we grab the full mmaps table. But it seems that perf dashboard just shows the sum value of all the mmaps, at least for Pss. That is why I just try to calculate Pss for an entire process in OS_WIN. Honestly, I am not sure how to achieve the same functionality as in Linux, i.e., the full mmaps, with the help of different Windows APIs. Bruce@ may know though. BESIDES, Primiano@ do you think this CL is sufficient to hook up the Windows Pss calculated for the memory bots. I have triggered/run the code added in this CL locally, and it is running fine. I re-used the data structure as used for OS_Linux mmaps, so I think the memory bots can hook it up automatically and show it in chromeperf.appspot.com? I really don't know how to test it end-to-end before landing this CL. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:160: DWORD GetPageEntriesNumber() { return number_of_entries; } On 2016/12/06 02:58:55, brucedawson wrote: > Should be const. Done. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:198: // the On 2016/12/06 17:47:57, Primiano Tucci wrote: > I think clang-format did bite you here (It's a KI it's not that good in > re-wrapping pre-wrapped comments) Acknowledged. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:217: DWORD number_of_entries; On 2016/12/06 02:58:55, brucedawson wrote: > Could/should go "= 0;" here rather than listing the initialization in the > constructor. New C++11 feature, and consistent with how buffer_ is initialized. Done. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < buffer.GetPageEntriesNumber(); i++) { On 2016/12/06 02:58:55, brucedawson wrote: > Should generally avoid calling functions like this in the loop. Not really > important in this case, but it can make life harder for the optimizer. Ditto for > loop below. > > Could also be very trendy and use begin()/end() functions and range-based for > loops - ask me for details. After a second thought, I think I will stick with index-based looping for now. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:262: ws_pss += 1.0f / buffer->WorkingSetInfo[i].ShareCount; On 2016/12/06 02:58:55, brucedawson wrote: > Remove trailing 'f' from this line and line 264. The 'f' on this line may > actively harm precision by triggering float-precision division instead of > double-precision. Done. https://codereview.chromium.org/2549803003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:267: *pss_bytes = static_cast<uint64_t>(ws_pss); On 2016/12/06 17:47:56, Primiano Tucci wrote: > 1) isn't this missing a * 4096 ? As the name suggests these are bytes, but you > are adding 1 (or a fraction) . > 2) Are all pages 4096 bytes on Windows? Yes, you are right. I am missing a *4096, which is the page size in Windows. Thanks! https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:307: base::trace_event::ProcessMemoryMaps::VMRegion region; On 2016/12/06 17:47:57, Primiano Tucci wrote: > If I read the code correctly now within this function we are grabbing and > iterating over WorkingSetInformationBuffer twice: > once up in line 268 upon GetWorkingSetSize() and once here. > Is there a way we could expose/wrap the WorkingSetInformationBuffer, or the > result of postprocessing it? My understanding is that line 268 calls GetWorkingSetSize() defined in process_metrics_win.cc. GetWorkingSetSize() itself calls a WinAPI GetProcessMemoryInfo. It seems WorkingSetInformationBuffer is not involved here. Please correct me if I am wrong. https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:313: #endif // defined(OS_LINUX) || defined(OS_ANDROID) On 2016/12/06 17:47:57, Primiano Tucci wrote: > plz can you remove the comment from this #endif as it doesn't apply anymore > after the various #else branches? Done.
> I guess that a > combination of VirtualQueryEx + GetMappedFileName + QueryWorkingSetEx might do > the job? This could be done, if the need is there. It would complicate the CL significantly, and would necessarily be racy since the virtual address map and working-set would be changing while we were scanning. I'm not sure if that matters. If it would be valuable then it would be helpful to know how the information will be used, in order to decide what sections to break the data into.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a small final comment > BESIDES, Primiano@ do you think this CL is sufficient to hook up the Windows Pss calculated for the memory bots. I have triggered/run the code added in this CL locally, and it is running fine. I re-used the data structure as used for OS_Linux mmaps, so I think the memory bots can hook it up automatically and show it in chromeperf.appspot.com? I really don't know how to test it end-to-end before landing this CL. You are correct. This should be enough to have the bots start reporting the PSS in the dashboard. Let me know if it doesn't happen. >This could be done, if the need is there. It would complicate the CL significantly, and would necessarily be racy since the virtual address map and working-set would be changing while we were scanning. I'm not sure if that matters. > If it would be valuable then it would be helpful to know how the information will be used, in order to decide what sections to break the data into. Let me clarify the situation. As far as the bots and chromeperf are concerned, this CL should be enough. The extra value of having the full mmaps (yes they are racy also on linux) is when developers pull the trace from the bots to investigate a regression and they happen to have more details. Again, I wouldn't change this CL anyways but it's a possible follow up for the future. https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2549803003/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:307: base::trace_event::ProcessMemoryMaps::VMRegion region; On 2016/12/06 19:40:09, chengx wrote: > On 2016/12/06 17:47:57, Primiano Tucci wrote: > > If I read the code correctly now within this function we are grabbing and > > iterating over WorkingSetInformationBuffer twice: > > once up in line 268 upon GetWorkingSetSize() and once here. > > Is there a way we could expose/wrap the WorkingSetInformationBuffer, or the > > result of postprocessing it? > > My understanding is that line 268 calls GetWorkingSetSize() defined in > process_metrics_win.cc. GetWorkingSetSize() itself calls a WinAPI > GetProcessMemoryInfo. It seems WorkingSetInformationBuffer is not involved here. > Please correct me if I am wrong. Ahh you are right. I confused GetWorkingSetSize() with GetWorkingSetKBytes() when reading the code. the latter does use the WSIBuffer. So you are right it's all good. https://codereview.chromium.org/2549803003/diff/60001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2549803003/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:309: pmd->process_mmaps()->AddVMRegion(region); ah you need also a call to pmd->set_has_process_mmaps(true)
The CQ bit was checked by chengx@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...
chengx@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, PTAL the /base/process code in this CL. Thank you!
https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:174: // Call the function once to get number of items Nit: I know this is just moving code, but this comment feels misleading. Maybe something like: On success, |buffer_| is populated with info about the working set of |process_|. On ERROR_BAD_LENGTH failure, increase the size of the buffer and try again. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:181: number_of_entries = static_cast<DWORD>(buffer_->NumberOfEntries); Is it possible to just make this a ULONG_PTR as well, so we don't static_cast all over the place? https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:196: // On windows 2000 the function returns 1 even when the buffer is too small. I don't think we support win2k? https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:213: DISALLOW_COPY_AND_ASSIGN(WorkingSetInformationBuffer); Nit: this is generally last https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:233: DWORD numPageEntries = buffer.GetPageEntriesNumber(); Nit: num_page_entries https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < numPageEntries; i++) { Match types (this should be DWORD or ULONG_PTR, whatever we end up deciding) https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:260: for (unsigned int i = 0; i < numPageEntries; i++) { Ditto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #6 (id:120001) has been deleted
chengx@chromium.org changed reviewers: + stanisc@chromium.org
@Daniel, I have fixed some of the nits. For the rest, I have included Stan, who wrote this code, for discussion. @Stan, Daniel has a few comments on your code in the CL, which I am not so sure about which way to go. Can you please take a look? Thanks! https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:174: // Call the function once to get number of items On 2016/12/08 20:25:30, dcheng wrote: > Nit: I know this is just moving code, but this comment feels misleading. > > Maybe something like: > On success, |buffer_| is populated with info about the working set of > |process_|. On ERROR_BAD_LENGTH failure, increase the size of the > buffer and try again. That is a good comment and I will take it. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:181: number_of_entries = static_cast<DWORD>(buffer_->NumberOfEntries); On 2016/12/08 20:25:30, dcheng wrote: > Is it possible to just make this a ULONG_PTR as well, so we don't static_cast > all over the place? I personally think ULONG_PTR should be fine. I will include stan, the owner of this code, in this discussion. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:213: DISALLOW_COPY_AND_ASSIGN(WorkingSetInformationBuffer); On 2016/12/08 20:25:30, dcheng wrote: > Nit: this is generally last Done. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:233: DWORD numPageEntries = buffer.GetPageEntriesNumber(); On 2016/12/08 20:25:30, dcheng wrote: > Nit: num_page_entries Done. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < numPageEntries; i++) { On 2016/12/08 20:25:30, dcheng wrote: > Match types (this should be DWORD or ULONG_PTR, whatever we end up deciding) I will do DWORD for now.
The CQ bit was checked by chengx@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/2549803003/diff/80001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:160: DWORD GetPageEntriesNumber() const { return number_of_entries; } Nit: Consider renaming this to either GetPageEntryCount or GetNumberOfPageEntries, or simply GetCount. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:163: bool GetPageEntries(const ProcessHandle& process_) { Nit: This doesn't get actual entries so perhaps the name should be different. How about QueryPageEntries? https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:181: number_of_entries = static_cast<DWORD>(buffer_->NumberOfEntries); On 2016/12/08 23:03:13, chengx wrote: > On 2016/12/08 20:25:30, dcheng wrote: > > Is it possible to just make this a ULONG_PTR as well, so we don't static_cast > > all over the place? > > I personally think ULONG_PTR should be fine. I will include stan, the owner of > this code, in this discussion. I am not the original author, but I did modify this code recently to fix an OOM bug. If this is changed to ULONG_PTR that would require using ULONG_PTR elsewhere where GetPageEntriesNumber is called. How about using a portable type such as size_t. I think it should be the same as ULONG_PTR? https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:196: // On windows 2000 the function returns 1 even when the buffer is too small. On 2016/12/08 20:25:30, dcheng wrote: > I don't think we support win2k? This might be an old comment. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < numPageEntries; i++) { On 2016/12/08 23:03:13, chengx wrote: > On 2016/12/08 20:25:30, dcheng wrote: > > Match types (this should be DWORD or ULONG_PTR, whatever we end up deciding) > > I will do DWORD for now. Consider using size_t as I mentioned above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:140001) has been deleted
PTAL~ Daniel@ Stan@ https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:160: DWORD GetPageEntriesNumber() const { return number_of_entries; } On 2016/12/09 01:19:55, stanisc wrote: > Nit: Consider renaming this to either GetPageEntryCount or > GetNumberOfPageEntries, or simply GetCount. renamed to GetPageEntryCount https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:163: bool GetPageEntries(const ProcessHandle& process_) { On 2016/12/09 01:19:55, stanisc wrote: > Nit: This doesn't get actual entries so perhaps the name should be different. > How about QueryPageEntries? Done. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:181: number_of_entries = static_cast<DWORD>(buffer_->NumberOfEntries); On 2016/12/09 01:19:55, stanisc wrote: > On 2016/12/08 23:03:13, chengx wrote: > > On 2016/12/08 20:25:30, dcheng wrote: > > > Is it possible to just make this a ULONG_PTR as well, so we don't > static_cast > > > all over the place? > > > > I personally think ULONG_PTR should be fine. I will include stan, the owner of > > this code, in this discussion. > > I am not the original author, but I did modify this code recently to fix an OOM > bug. If this is changed to ULONG_PTR that would require using ULONG_PTR > elsewhere where GetPageEntriesNumber is called. How about using a portable type > such as size_t. I think it should be the same as ULONG_PTR? I will just leave the old comment there for now. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:196: // On windows 2000 the function returns 1 even when the buffer is too small. On 2016/12/09 01:19:55, stanisc wrote: > On 2016/12/08 20:25:30, dcheng wrote: > > I don't think we support win2k? > > This might be an old comment. Acknowledged. https://codereview.chromium.org/2549803003/diff/80001/base/process/process_me... base/process/process_metrics_win.cc:234: for (unsigned int i = 0; i < numPageEntries; i++) { On 2016/12/09 01:19:55, stanisc wrote: > On 2016/12/08 23:03:13, chengx wrote: > > On 2016/12/08 20:25:30, dcheng wrote: > > > Match types (this should be DWORD or ULONG_PTR, whatever we end up deciding) > > > > I will do DWORD for now. > > Consider using size_t as I mentioned above. Done.
The CQ bit was checked by chengx@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 checked by chengx@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/2549803003/diff/160001/base/process/process_m... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... base/process/process_metrics_win.cc:160: DWORD GetPageEntryCount() const { return number_of_entries; } This should be size_t too; otherwise you'd get a build warning on x64. https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... base/process/process_metrics_win.cc:190: number_of_entries = number_of_entries * 1.1; Does this build without a warning?
Stan@, please see my reply to your comments below. Thanks! https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... base/process/process_metrics_win.cc:160: DWORD GetPageEntryCount() const { return number_of_entries; } On 2016/12/09 22:46:40, stanisc wrote: > This should be size_t too; otherwise you'd get a build warning on x64. I will do the change. But I didn't get warning. I think they are both unsigned long. https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... base/process/process_metrics_win.cc:190: number_of_entries = number_of_entries * 1.1; On 2016/12/09 22:46:40, stanisc wrote: > Does this build without a warning? It built without any warning here.
The CQ bit was checked by chengx@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...
lgtm
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_...)
lgtm with comments addressed https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/160001/base/process/process_m... base/process/process_metrics_win.cc:201: number_of_entries = std::min(number_of_entries, buffer_->NumberOfEntries); Can we add a TODO to remove this comment and this logic in a followup? Presuambly, it's no longer needed since we don't have Win2000 support. https://codereview.chromium.org/2549803003/diff/180001/base/process/process_m... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2549803003/diff/180001/base/process/process_m... base/process/process_metrics_win.cc:168: DWORD buffer_size = sizeof(PSAPI_WORKING_SET_INFORMATION) + Nit: this should be size_t https://codereview.chromium.org/2549803003/diff/180001/base/process/process_m... base/process/process_metrics_win.cc:175: // | process_ | .On ERROR_BAD_LENGTH failure, increase the size of the Nit: no spaces between the identifier and |: this should be |process_|. Also, space after the period rather than before.
The CQ bit was checked by chengx@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 chengx@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chengx@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 chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, primiano@chromium.org, stanisc@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2549803003/#ps220001 (title: "Fix compile error for std::min")
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": 1481525482253320,
"parent_rev": "37e42bb282e43be1f1dd7b4966b6b234dcce7c4e", "commit_rev":
"c11c9b4a2e2875ab77203841875060a6e3f571ed"}
Message was sent while issue was closed.
Description was changed from ========== Add function to compute proportional set size for OS_WIN Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet. This CL added functionality to calculate proportional set size for a process. BUG=623499 ========== to ========== Add function to compute proportional set size for OS_WIN Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet. This CL added functionality to calculate proportional set size for a process. BUG=623499 Review-Url: https://codereview.chromium.org/2549803003 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add function to compute proportional set size for OS_WIN Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet. This CL added functionality to calculate proportional set size for a process. BUG=623499 Review-Url: https://codereview.chromium.org/2549803003 ========== to ========== Add function to compute proportional set size for OS_WIN Memory-Infra on Windows doesn't use memory usage reported by the OS (proportional set size, private dirty, etc.) yet. This CL added functionality to calculate proportional set size for a process. BUG=623499 Committed: https://crrev.com/b172892773fe15a10508f733ca0bc831240a7ec6 Cr-Commit-Position: refs/heads/master@{#437827} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b172892773fe15a10508f733ca0bc831240a7ec6 Cr-Commit-Position: refs/heads/master@{#437827} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
