Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1573313002:
Add support for CountResidentBytes in windows (Closed)
|
Created:
4 years, 11 months ago by ssid Modified:
4 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for CountResidentBytes in windows
This CL adds support for CountResidentBytes on windows.
The QueryWorkingSetEx gives the information about pages in range of
given address that are in the working set of the process. We use that
to count the resident bytes similar to how it's done in other
platforms.
BUG=570655
Committed: https://crrev.com/b39972df6e73b63050a982db7723b664c2afb729
Cr-Commit-Position: refs/heads/master@{#370689}
Patch Set 1 #Patch Set 2 : Fixes. #Patch Set 3 : fix? #
Total comments: 4
Patch Set 4 : Remove +1 #
Total comments: 6
Patch Set 5 : fixed result and comment. #Patch Set 6 : Nit. #
Total comments: 6
Patch Set 7 : Fixes. #
Total comments: 2
Patch Set 8 : Fix. #Patch Set 9 : format. #Messages
Total messages: 26 (7 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
I checked with local test cases that it gives correct values. By allocating memory and writing few pages, writing all pages, writing pages in middle, and it seems to give correct values. The time taken for one discardable segment was 0.1ms extra which is the same as linux. PTAL Thanks.
On 2016/01/12 12:21:25, ssid wrote: > I checked with local test cases that it gives correct values. > By allocating memory and writing few pages, writing all pages, writing pages in > middle, and it seems to give correct values. > > The time taken for one discardable segment was 0.1ms extra which is the same as > linux. > > PTAL Thanks. ping
primiano@chromium.org changed reviewers: + brucedawson@chromium.org
I think I'd ask +brucedawson here. Not a windows expert here. Bruce: can I ask you to take a look to this CL (~20 lines). The goal is to count how many pages in a virtual range (real use case: discardable memory) are actually resident.
https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:70: std::vector<PSAPI_WORKING_SET_EX_INFORMATION> vec(page_count + 1); Why is there a "+ 1" here? https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:81: resident_page_count += vec[i].VirtualAttributes.Valid; I *think* this is correct, but the documentation is not entirely explicit about what "Valid" really means. That is, it doesn't directly say whether that means that the page is in the working set of the process, but I think it must.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:70: std::vector<PSAPI_WORKING_SET_EX_INFORMATION> vec(page_count + 1); On 2016/01/19 19:39:08, brucedawson wrote: > Why is there a "+ 1" here? I was anticipating to count for non-aligned memory, so i added 1 for extra. This shouldn't be here. https://codereview.chromium.org/1573313002/diff/40001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:81: resident_page_count += vec[i].VirtualAttributes.Valid; On 2016/01/19 19:39:08, brucedawson wrote: > I *think* this is correct, but the documentation is not entirely explicit about > what "Valid" really means. That is, it doesn't directly say whether that means > that the page is in the working set of the process, but I think it must. Yes, I had the same doubt. So, I tried experimenting using the patch and it gave me Valid only on pages I used. Also Since the api name is PSAPI_WORKING_SET_EX_INFORMATION, it gives me more confidence that the page is valid only if it's part of working set.
A few thoughts. Only the #else comment really needs fixing, but the others are worth considering. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:52: int result = 0; // 0 on success. Reusing "int result" as a success/error code across three platforms is probably stretching it a bit. Perhaps better to have "bool failure = false;" and then remap the mincore/QueryWorkingSetEx failure codes to that more obvious type. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:70: std::vector<PSAPI_WORKING_SET_EX_INFORMATION> vec(page_count); Possibly out of scope for this change, but the std::vector should really be allocated *outside* of the loop, to avoid reallocating it each time. It's also worth thinking about using a local variable array instead of std::vector to avoid allocating any memory. With kMaxChunkSize set to 32 MB that would give us a 8K element array (probably 128 KB on 64-bit) which is large but not totally nuts. kMaxChunkSize could be reduced on Windows to minimize the size of the array, but knowing whether that was worthwhile would require profiling QueryWorkingSetEx. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:82: #else // defined(OS_MACOSX) || defined(OS_IOS) This comment is no longer meaningful. Consider changing this to #elif defined(???) instead of just a fall-through.
Thanks, replied inline. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:52: int result = 0; // 0 on success. On 2016/01/19 20:16:43, brucedawson wrote: > Reusing "int result" as a success/error code across three platforms is probably > stretching it a bit. Perhaps better to have "bool failure = false;" and then > remap the mincore/QueryWorkingSetEx failure codes to that more obvious type. Done. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:70: std::vector<PSAPI_WORKING_SET_EX_INFORMATION> vec(page_count); On 2016/01/19 20:16:43, brucedawson wrote: > Possibly out of scope for this change, but the std::vector should really be > allocated *outside* of the loop, to avoid reallocating it each time. > > It's also worth thinking about using a local variable array instead of > std::vector to avoid allocating any memory. With kMaxChunkSize set to 32 MB that > would give us a 8K element array (probably 128 KB on 64-bit) which is large but > not totally nuts. kMaxChunkSize could be reduced on Windows to minimize the size > of the array, but knowing whether that was worthwhile would require profiling > QueryWorkingSetEx. I will add a follow up to this CL. https://codereview.chromium.org/1573313002/diff/80001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:82: #else // defined(OS_MACOSX) || defined(OS_IOS) On 2016/01/19 20:16:43, brucedawson wrote: > This comment is no longer meaningful. Consider changing this to #elif > defined(???) instead of just a fall-through. Done.
A few remaining nits (sorry I didn't notice one of them earlier) but lgtm. https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:63: mincore(reinterpret_cast<void*>(chunk_start), chunk_size, vec.data()); Maybe do the !! dance to make it explicit that you're converting from an integer to a bool? https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:87: failure = result; Possible !! here also? https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:103: LOG(ERROR) << "mincore() call failed. The resident size is invalid"; Just noticed this - note that the message is incorrect in the WIN32 case.
addressed comments. Thanks. primiano@ PTAL. https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:63: mincore(reinterpret_cast<void*>(chunk_start), chunk_size, vec.data()); On 2016/01/20 18:21:07, brucedawson wrote: > Maybe do the !! dance to make it explicit that you're converting from an integer > to a bool? Done. https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:87: failure = result; On 2016/01/20 18:21:07, brucedawson wrote: > Possible !! here also? Done. https://codereview.chromium.org/1573313002/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:103: LOG(ERROR) << "mincore() call failed. The resident size is invalid"; On 2016/01/20 18:21:07, brucedawson wrote: > Just noticed this - note that the message is incorrect in the WIN32 case. Done.
https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:84: result = !!mincore(reinterpret_cast<void*>(chunk_start), chunk_size, Oops! You can't put the !! here. It has to go on line 87 when you assign to failure.
https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:84: result = !!mincore(reinterpret_cast<void*>(chunk_start), chunk_size, On 2016/01/20 20:48:04, brucedawson wrote: > Oops! You can't put the !! here. It has to go on line 87 when you assign to > failure. Sorry, Fixed.
On 2016/01/20 20:50:51, ssid wrote: > https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... > File base/trace_event/process_memory_dump.cc (right): > > https://codereview.chromium.org/1573313002/diff/140001/base/trace_event/proce... > base/trace_event/process_memory_dump.cc:84: result = > !!mincore(reinterpret_cast<void*>(chunk_start), chunk_size, > On 2016/01/20 20:48:04, brucedawson wrote: > > Oops! You can't put the !! here. It has to go on line 87 when you assign to > > failure. > > Sorry, Fixed. I think that that sort of late-breaking bug is the one disadvantage of code reviews - sometimes the churn of code reviews can let something slip in after the developer is tired of testing. But, code reviews are still a big net positive. lgtm
LGTM Thanks a lot Bruce!
On 2016/01/21 11:58:10, Primiano Tucci wrote: > LGTM > Thanks a lot Bruce! Yup thanks. :)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1573313002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1573313002/180001
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1573313002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1573313002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add support for CountResidentBytes in windows This CL adds support for CountResidentBytes on windows. The QueryWorkingSetEx gives the information about pages in range of given address that are in the working set of the process. We use that to count the resident bytes similar to how it's done in other platforms. BUG=570655 ========== to ========== Add support for CountResidentBytes in windows This CL adds support for CountResidentBytes on windows. The QueryWorkingSetEx gives the information about pages in range of given address that are in the working set of the process. We use that to count the resident bytes similar to how it's done in other platforms. BUG=570655 Committed: https://crrev.com/b39972df6e73b63050a982db7723b664c2afb729 Cr-Commit-Position: refs/heads/master@{#370689} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b39972df6e73b63050a982db7723b664c2afb729 Cr-Commit-Position: refs/heads/master@{#370689} |
