Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181263005:
Make task manager memory data more efficient and meaningful. (Closed)
|
Created:
5 years, 6 months ago by brucedawson Modified:
5 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, afakhry Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake task manager memory data more efficient and meaningful.
Chrome's default memory column is quite expensive to calculate. The
Windows version of ProcessMetrics::GetWorkingSetKBytes uses QueryWorkingSet
which returns information for every page in the process. Generating and
copying this data is expensive enough that it can lead to 100+ ms hitches
in scrolling, which also means that it is consuming 10% of a core (100 ms
for every per-second update). That is with about 20 tabs open and the cost
increases proportionally with more tabs. See bug 494210
Chrome's default memory column also does not display the best possible
number. Because it includes shareable-but-not-shared memory it means that
renderer code goes from being counted when one tab is open to being not
count when two tabs are open. This causes confusion. Proportional Set Size
could avoid this confusion but is non-standard for Windows. Private working
set is a better choice because it has a clear meaning and is a standard
memory column in task manager and process explorer. See bug 484433
This change adds a RefreshPhysicalMemoryFromWorkingSetSnapshot step to
TaskManagerModel::Refresh. After the old data has been thrown away the
RefreshPhysicalMemoryFromWorkingSetSnapshot function uses Pdh to
retrieve private working set data for all chrome processes. Then when
TaskManagerModel::GetPhysicalMemory is called it will almost always find
that values.is_physical_memory_valid is already true. Because Pdh retrieves
all private working sets in one call, from the private-to-Windows memory
bookkeeping data, this is much faster than calculating working sets for
each process individually and the cost is mostly independent of the number
of processes.
Therefore the speedup from using Pdh increases with more tabs. If the user
has one small tab open then Pdh is slightly faster. With three tabs open
Pdh is significantly faster. With six tabs (eight processes) Pdh is nine
times faster than using QueryWorkingSet (0.18 ms per process versus 1.6 ms
per process).
GetPhysicalMemory has been modified to subtract shareable from the
physical_memory instead of shared so that it also calculates private
working set, for consistency. This change is a NOP on OSX because shared
and shareable are both always zero (see GetCommittedAndWorkingSetKBytes in
process_metrics_mac.cc).
In the cases where Pdh fails or a new chrome process is created between
RefreshPhysicalMemoryFromWorkingSetSnapshot and GetPhysicalMemory the
GetPhysicalMemory calculations will be used. The results typically differ by
less than 0.5%. Alternating between the two methods on each refresh shows no
significant change.
Pdh supports retrieving private working set data on Vista and above. On
Windows XP the code will fall-back to using the old method.
Pdh will return private WS data for all chrome* processes, so canary and
stable will see each other's processes, but
RefreshPhysicalMemoryFromWorkingSetSnapshot will ignore unrelated processes.
Processes with names like 'chromeWidget.exe' will also be listed and will
also be ignored. This does not measurably affect performance.
R=nick@chromium.org,jschuh@chromium.org,rsesek@chromium.org,thakis@chromium.org
BUG=484433, 494210
Committed: https://crrev.com/f66d1ecf0cd7426393088c91f2af78ff1889bab4
Cr-Commit-Position: refs/heads/master@{#337096}
Committed: https://crrev.com/a762bc76dccf669d5a1cdfb588d8dafe1d0bd18f
Cr-Commit-Position: refs/heads/master@{#337886}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Major refactoring of task manager memory optimization code #Patch Set 3 : Final cleanup before submitting for review. #Patch Set 4 : Add spacing for better readability. #Patch Set 5 : Comment tweak #Patch Set 6 : Add missing include #
Total comments: 54
Patch Set 7 : Use GenericScopedHandle to clean up PDH_HQUERY #Patch Set 8 : Add pdh.lib and pdh.dll to follow setupapi.* pattern #
Total comments: 2
Patch Set 9 : Lots of fixes to issues raised in code review #Patch Set 10 : Reduced PdhGetFormattedCounterArray calls and now presubmit clean #
Total comments: 12
Patch Set 11 : Include order fixes, outline destructor, zero check #Patch Set 12 : Resurrce having two query calls to PdhGetFormattedCounterArray #
Total comments: 2
Patch Set 13 : Moved from base to chrome/browser #Patch Set 14 : Adding missing files and 'git cl format' #Patch Set 15 : Fix typo #
Total comments: 6
Patch Set 16 : Comments, using, and one bug fix. #Patch Set 17 : Don't try using Pdh to get working set on Vista. #Patch Set 18 : Adjust high threshold so it works with Dr Memory. #Messages
Total messages: 55 (11 generated)
I want to improve the task manager's memory column on Windows (it currently burns a lot of cycles to calculate what I think is the wrong number). I have a possible fix mostly wired up but I wanted to run it by you two before I finish it off. The solution is not perfect (still more expensive than it really should be) but it is the best I've been able to do, and it is a modest to huge improvement (a huge improvement when many tabs are open). Any high-level feedback would be appreciated. I'll be in Mountain View from the 15th to 19th.
nick@chromium.org changed reviewers: + jschuh@chromium.org, rsesek@chromium.org
+jschuh -- to weigh in on whether these new-to-chrome win32 API calls are a good idea, or if anyone we know has experience with them enough to review this patch. +rsesek -- for general ideas about how this might fit into the existing process_metrics code. At a high level: - I think we should probably meet up over VC or in person to get on the same page here about the benefits and limitations of this approach. I tried looking at the MSDN docs for the Pdh* calls and got lost pretty quickly. I'm in Seattle and have core work hours of 10-4. Please put something on my calendar. - Is the performance gain we see due to batching the stats-gathering into fewer calls, or is it due to the fact the stats are, like, gathered incrementally rather than computed from scratch each call? - Ideally this would take the form of enhancing base/process_metrics or another common utility, rather than adding win32-specific code to the task manager. https://codereview.chromium.org/1181263005/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/task_manager.cc (right): https://codereview.chromium.org/1181263005/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/task_manager.cc:1127: #include <pdhmsg.h> As I imagine you are aware, this won't compile on non-Windows, and #include's go at the top of files. Some ideas for the right way to structuring this code: ideally, we could work this into base/process_metrics. It probably doesn't belong in the task_manager. Wherever it winds up, it ought to live in some _win.cc file, ideally as an implementation of a platform-neutral interface (a non-win .h file). Files ending in _win.cc are by convention not compiled on other platforms. process_info_snapshot.h (which is a similar, mac-only memory api) might be a good inspiration for how to structure this properly. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro... One strong immediate reason to structure this code for reuse, is that we currently have two task manager implementations (task_manager/ and task_management/) due to a refactor-rewrite that's in progress, and we want them to be querying the same APIs. A second consideration is that there are other memory UIs, like about:memory and about:memory-internals, that may benefit from a batched process stats gathering API. https://codereview.chromium.org/1181263005/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/task_manager.cc:1128: #pragma comment(lib, "pdh.lib") It's been ages since I added a new dll dependency to chromium, so I'm not sure whether this is the appropriate way or not. Most libs I see are added to .gyp and .gn files, and some are specified as delayload. IIRC we kept an eye on these to avoid bloating the app startup time. I'm looping in jschuh for an opinion on this CL. https://codereview.chromium.org/1181263005/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/task_manager.cc:1130: CWorkingSetCollector::CWorkingSetCollector() { This class is not written to https://www.chromium.org/developers/coding-style , which has the effect of making it pretty hard for me and others follow: all we'll see are style nits, and it's hard to look past them and see the actual meat of your proposal. Variables should be named like_this, not likeThis. Data members should be named like_this_ We don't use prefixes like C to mean class, nor dw to mean dword. You can type "git cl format" to fix the whitespace/indentation. https://codereview.chromium.org/1181263005/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/task_manager.cc:1192: auto pPrivateWSResults = (PDH_FMT_COUNTERVALUE_ITEM*)&buffer[0]; No C-style casts.
Thanks for the detailed feedback. The reason this code is faster is that Windows keeps track of the private working set for processes and this lets us retrieve it 'directly', instead of having to examine the attributes of every page in each process' working set. In fact, I'm disappointed that there isn't more of a speedup, but the Pdh* APIs are painfully indirect. Ideally this code would take a fraction of a ms instead of still taking a few ms. I'd still like to find a better way to do this but I have had no luck. "As I imagine you are aware, this won't compile on non-Windows, and #include's go at the top of files." Yes, that was to get it working enough so that we could have a discussion. The #pragma comment will also go away - it's non-standard for Chromium. "I tried looking at the MSDN docs for the Pdh* calls and got lost pretty quickly" Definitely not my favorite API - it took me a while to figure out. I'll talk to Justin today and arrange a VC with you when I'm back in Kirkland, after I've cleaned up the working set collector class.
We chatted a bit in the office about the implementation and some API refinement. I concur that performance counters are generally The Right Way™ to get this sort of information on Windows. I also agree with the suggestions about this living in base and in general how to better integrate the code in the Chrome way. And I'd add that some extra effort in commenting is warranted given that the performance counters are a bit of a dark art. tl;dr - +1 one on performance counters and make it Chromey
brucedawson@chromium.org changed reviewers: + rvargas@chromium.org
Add spacing for better readability.
Comment tweak
PTAL After getting initial feedback from nick@ and jschuh@ I've reworked this code that it has a better API and is (I think) compatible with the Chrome coding style guide. The CPU time savings when the task manager is open can be considerable. The memory value being displayed is also more meaningful now. See the bugs for details on the problems that this is trying to solve.
Add missing include
Use GenericScopedHandle to clean up PDH_HQUERY
I just updated the patch to use GenericScopedHandle instead of manually calling PdhCloseQuery. - Pros - this lets me delete a destructor - Cons - the boilerplate to use GenericScopedHandle is ~40 lines
I just finished reviewing the older patch set. My feedback is mostly stylistic -- from a high level this looks great. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. 2015, and drop the (c) [yeah, seriously :( ] https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:5: #ifndef PRIVATE_WORKING_SET_SNAPSHOT_H_ This should be: BASE_PROCESS_PRIVATE_WORKING_SET_SNAPSHOT_ https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:32: void Sample(); I'd add a blank line between each method here. I think that's normal to do when you're declaring a set of public methods that form the caller's interface to the class, and -- most importantly -- it seems to be the done thing in the nearby process.h file. [As far as I know it's not against the style guide to omit the vertical whitespace -- and if anything, the style guide says to minimize use of blank lines. But in Chromium I'm accustomed to seeing the blank lines omitted in cases where a class is implementing more than one abstract base class, and there's a desire to tightly group together all the overridden methods of each interface.] https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:40: PDH_HQUERY query_handle_ = nullptr; We talked over VC about switching this to win::ScopedHandle (from base/win/scoped_handle.h). That would call CloseHandle in the dtor. To make it call PdhCloseQuery on destruction, you'd need to define a traits type that would call PdhCloseQuery, following this example: https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut.... The main benefit of ScopedHandle is as a defense against leaks and double-closes, from the fact that it's a move-only type. It is nearly overkill here, but I'd probably vote for it it anyway, on the grounds that a seeing a raw handle type in a .h file tempts a lot of us (myself included) to spend a few minutes scanning the code to prove that it's closed properly. However I'm open to arguments to the contrary. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:46: struct PdhCounterPair { A strict reading of the style guide requires these struct definitions to be before any data member declarations (With the typedefs & enums). http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:50: PDH_HCOUNTER private_ws_handle = nullptr; These handles don't need to be closed because their lifetimes are bound to query_handle_, right? If so, I'd add a comment here explaining that. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot.h:54: struct PidAndPrivateWS { WS -> WorkingSet http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... File base/process/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. 2015, no (c) https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:9: #if defined(OS_WIN) This isn't needed. Since the file ends in _win, it will be excluded from the build on other platforms. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:21: { "{" always goes on the previous line (here and elsewhere), and ":" gets a four space indent. Or do what I do, and let "git cl format" take care of this stuff. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:59: NULL, &new_counters.process_id_handle) != ERROR_SUCCESS) { Cases like this should get an additional 4 lines of indent. The common interpretation of "wrapped function call arguments get a 4 space indent" rule for it to be relative to the callable function -- so in this case, relative to the P in PdhAddCounterA. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:77: // as the process count exceeds two or three. Don't duplicate comments between the .h and the .cc file. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... I think you'd be fine to have no comment here -- the comments in the body of the function seem to suffice to describe how things work. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:102: if (PdhGetFormattedCounterArray(counter_pair.private_ws_handle, Optimization idea: since we bail if the sizes/counts aren't equal, it seems like we could skip this second call, assume that the buffer sizes are equal, and use the result of the "Retrieve the private working set data" operation to validate that they were? https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:119: (&buffer[0]); Apparently (I just learned this) C++11 has vector::data() as an alternative to &vector[0], with the advantage that it's safe to call on an empty vector. You could also use scoped_ptr<char[]> buffer or scoped_ptr<PDH_FMT_COUNTERVALUE_ITEM[]> instead of a vector. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:143: records_[start_offset + i].private_ws = std::min(max_size_t, Could this be a saturated_cast<size_t>(private_ws_data[i].FmtValue.largeValue)? https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:156: // is in bytes. This comment also is a duplicate of the one in the .h. You only need it in the .h. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... File base/process/private_working_set_snapshot_win_unittest.cc (right): https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2015, no (c) https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:10: typedef testing::Test PrivateWorkingSetSnapshotWinTest; Apparently we're supposed to prefer C++11 aliases now instead of typedefs. using PrivateWorkingSetSnapshotWinTest = testing::Test; https://chromium-cpp.appspot.com/#whitelist_lang_list https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:34: // change that unrelated changes could ever make this fail. This mostly just change->chance https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:35: // checks against some uncaught error that might return 0xFFFFFFFF. There are two things to worry about here. One is potential failures on the drmemory and similar memory-instrumented bots where the heap can be bloated by some large constant factor. Another is the fact that base_unittests don't spawn a new process for every test, so we may be affected by previous tests. My hunch is that if this fails in practice it will be a result of the previous two issues occurring. Given that it's base_unittests, and not the whole browser, we might just get lucky. It's fine to just deal with them as they occur. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:36: EXPECT_LT(private_ws, 500000000u); In a test similar to this (of code that reported memory usage), I've seen an approach where you create a memory allocation of a large size N, touch every page, after which point you could then do the following test expectations: size_t private_ws2 = private_ws_snapshot.GetPrivateWorkingSet(pid); EXPECT_EQ(private_ws, private_ws2) << "GetPrivateWorkingSet should be consistent until the next call to Sample()"; private_ws_snapshot.Sample(); size_t private_ws3 = private_ws_snapshot.GetPrivateWorkingSet(pid); EXPECT_GT(private_ws3, private_ws2 + N/2) << "GetPrivateWorkingSet should increase as we allocate more memory"; https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager.cc (right): https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager.cc:1134: void TaskManagerModel::PopulateCache() { Add a comment here (or at the bottom of the function) to describe why doing nothing is okay on non-windows. You could put it, if you want, in a comment in an (otherwise empty) #else clause. https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager.cc:1140: for (size_t rIndex = 0; rIndex < resources_.size(); ++rIndex) { "i", "index", "resource", or "resource_index", but definitely not "rIndex" https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager.cc:1141: size_t privateWS = working_set_snapshot_->GetPrivateWorkingSet( private_working_set https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager.cc:1162: PopulateCache(); Maybe: RefreshPhysicalMemoryFromWorkingSetSnapshot() https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager.h (right): https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager.h:29: class PrivateWorkingSetSnapshot; Nit: sort
https://codereview.chromium.org/1181263005/diff/140001/base/process/private_w... File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/140001/base/process/private_w... base/process/private_working_set_snapshot.h:48: class PDHDummyVerifierTraits { Does using the preexisting DummyVerifier not compile? I thought that PDH_HQUERY was just ultimately a typedef for HANDLE.
The CQ bit was checked by brucedawson@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/1181263005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Lots of updates based on feedback - thanks. The code is now "git cl format"
clean, so that should avoid some issues, and the (improved) test passes.
Known issues remaining:
I'm not sure if the gn build issues are fixed ("git cl try" is misbehaving and I
can't reproduce locally - investigating). The temporary mac build errors should
be fixed.
I have not tried removing the extra call to PdhGetFormattedCounterArray.
Two questions:
There are some member variables and at least one local still called
'private_ws'. You didn't say anything about them, and it would make things more
verbose, but I can certainly change those.
The include order rules are causing issues in both
private_working_set_snapshot_win.cc and private_working_set_snapshot_win.h. In
the .cc file I can't find a way to avoid warnings without either violating the
guidance or using #if defined(OS_WIN). In the .h file I can't find any way at
all.
The problem in the .h file is that "base/win/scoped_handle.h" has to be inside
#if defined(OS_WIN). Arguably all of the includes should be. but I can't find
any ordering of includes that avoids the presubmit warnings. I'll dig into the
presubmit script to figure out more details, but if you know the magic...
I'm also tempted to relabel the memory column as "Memory (private WS)" but I'm
not sure that would really help.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
File base/process/private_working_set_snapshot.h (right):
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Okay that's funny. Lesson learned about the perils of copying from existing
files. Fixed.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:5: #ifndef
PRIVATE_WORKING_SET_SNAPSHOT_H_
On 2015/06/26 21:46:15, ncarter wrote:
> This should be:
>
> BASE_PROCESS_PRIVATE_WORKING_SET_SNAPSHOT_
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:32: void Sample();
On 2015/06/26 21:46:15, ncarter wrote:
> I'd add a blank line between each method here. I think that's normal to do
when
> you're declaring a set of public methods that form the caller's interface to
the
> class, and -- most importantly -- it seems to be the done thing in the nearby
> process.h file.
>
> [As far as I know it's not against the style guide to omit the vertical
> whitespace -- and if anything, the style guide says to minimize use of blank
> lines. But in Chromium I'm accustomed to seeing the blank lines omitted in
cases
> where a class is implementing more than one abstract base class, and there's a
> desire to tightly group together all the overridden methods of each
interface.]
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:40: PDH_HQUERY query_handle_ =
nullptr;
On 2015/06/26 21:46:15, ncarter wrote:
> We talked over VC about switching this to win::ScopedHandle (from
> base/win/scoped_handle.h). That would call CloseHandle in the dtor. To make it
> call PdhCloseQuery on destruction, you'd need to define a traits type that
would
> call PdhCloseQuery, following this example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/shortcut....
>
> The main benefit of ScopedHandle is as a defense against leaks and
> double-closes, from the fact that it's a move-only type. It is nearly overkill
> here, but I'd probably vote for it it anyway, on the grounds that a seeing a
raw
> handle type in a .h file tempts a lot of us (myself included) to spend a few
> minutes scanning the code to prove that it's closed properly. However I'm open
> to arguments to the contrary.
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:46: struct PdhCounterPair {
On 2015/06/26 21:46:15, ncarter wrote:
> A strict reading of the style guide requires these struct definitions to be
> before any data member declarations (With the typedefs & enums).
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
>
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:50: PDH_HCOUNTER private_ws_handle =
nullptr;
On 2015/06/26 21:46:15, ncarter wrote:
> These handles don't need to be closed because their lifetimes are bound to
> query_handle_, right? If so, I'd add a comment here explaining that.
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot.h:54: struct PidAndPrivateWS {
On 2015/06/26 21:46:15, ncarter wrote:
> WS -> WorkingSet
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...
Done.
Although, I also have a few private_ws member variables and locals. I could
change them as well but it's *so* verbose...
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
File base/process/private_working_set_snapshot_win.cc (right):
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2015/06/26 21:46:16, ncarter wrote:
> 2015, no (c)
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:9: #if defined(OS_WIN)
The reason for the #if defined was to make the presubmit happy. Without it I get
this:
Your #include order seems to be broken. Remember to use the right collation
(LC_COLLATE=C) and check
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Or...
base\process\private_working_set_snapshot_win.cc:9 \
base\process\private_working_set_snapshot_win.cc:10
My assumption was that without the #if defined(OS_WIN) the presubmit check
couldn't tell that those were Windows header files and therefore wanted them
sorted in with <algorithm> and windows_version.h. Without the #if defined I can
only get past that warning by having all five #includes adjacent and sorted,
which is (I believe) incorrect.
Thoughts?
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:21: {
Huh. Clearly I *know* that rule, and yet...
Okay, time to try git cl format. Huh. It works nicely. I thought it failed last
time but it certainly worked this time. Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:59: NULL,
&new_counters.process_id_handle) != ERROR_SUCCESS) {
"git cl format" did it somewhat differently from that and it looks tidy so I'm
going to defer to it. That is, the new version of this is indented according to
git cl format.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:77: // as the process count
exceeds two or three.
On 2015/06/26 21:46:16, ncarter wrote:
> Don't duplicate comments between the .h and the .cc file.
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
>
> I think you'd be fine to have no comment here -- the comments in the body of
the
> function seem to suffice to describe how things work.
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:102: if
(PdhGetFormattedCounterArray(counter_pair.private_ws_handle,
The calls to PdhGetFormattedCounterArray are very cheap (they never show up on
the profile, only PdhCollectQueryData does) so there is not much gain. However
it should allow slightly less code, which is a good thing. I'll try it.
Aside: the check for matching buffer sizes/counts was broken. It was intended to
continue if *either* value differed, not if both. Probably irrelevant, but
*oops*.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:119: (&buffer[0]);
I started changing to .data() but then decided to leave it as [0] for symmetry
with [buffer_size1].
I also like the portability between C arrays and vector that &[] gives, although
that's not a great rationale.
I prefer vector over scoped_ptr<char[]> - more standard. And
scoped_ptr<PDH_FMT_COUNTERVALUE_ITEM[]> would not actually work (I believe)
because the size is actually greater than
item_count*sizeof(PDH_FMT_COUNTERVALUE_ITEM).
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:143: records_[start_offset +
i].private_ws = std::min(max_size_t,
On 2015/06/26 21:46:16, ncarter wrote:
> Could this be a
saturated_cast<size_t>(private_ws_data[i].FmtValue.largeValue)?
Ooh -- much better.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win.cc:156: // is in bytes.
On 2015/06/26 21:46:16, ncarter wrote:
> This comment also is a duplicate of the one in the .h. You only need it in the
> .h.
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
File base/process/private_working_set_snapshot_win_unittest.cc (right):
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2015/06/26 21:46:16, ncarter wrote:
> 2015, no (c)
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win_unittest.cc:10: typedef
testing::Test PrivateWorkingSetSnapshotWinTest;
On 2015/06/26 21:46:16, ncarter wrote:
> Apparently we're supposed to prefer C++11 aliases now instead of typedefs.
>
> using PrivateWorkingSetSnapshotWinTest = testing::Test;
>
> https://chromium-cpp.appspot.com/#whitelist_lang_list
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win_unittest.cc:34: // change that
unrelated changes could ever make this fail. This mostly just
On 2015/06/26 21:46:16, ncarter wrote:
> change->chance
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win_unittest.cc:35: // checks against
some uncaught error that might return 0xFFFFFFFF.
On 2015/06/26 21:46:16, ncarter wrote:
> There are two things to worry about here. One is potential failures on the
> drmemory and similar memory-instrumented bots where the heap can be bloated by
> some large constant factor. Another is the fact that base_unittests don't
spawn
> a new process for every test, so we may be affected by previous tests.
>
> My hunch is that if this fails in practice it will be a result of the previous
> two issues occurring. Given that it's base_unittests, and not the whole
browser,
> we might just get lucky. It's fine to just deal with them as they occur.
Done.
https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w...
base/process/private_working_set_snapshot_win_unittest.cc:36:
EXPECT_LT(private_ws, 500000000u);
On 2015/06/26 21:46:16, ncarter wrote:
> In a test similar to this (of code that reported memory usage), I've seen an
> approach where you create a memory allocation of a large size N, touch every
> page, after which point you could then do the following test expectations:
>
> size_t private_ws2 = private_ws_snapshot.GetPrivateWorkingSet(pid);
> EXPECT_EQ(private_ws, private_ws2) << "GetPrivateWorkingSet should be
consistent
> until the next call to Sample()";
> private_ws_snapshot.Sample();
> size_t private_ws3 = private_ws_snapshot.GetPrivateWorkingSet(pid);
> EXPECT_GT(private_ws3, private_ws2 + N/2) << "GetPrivateWorkingSet should
> increase as we allocate more memory";
Sounds good. Done.
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager.cc (right):
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager.cc:1134: void
TaskManagerModel::PopulateCache() {
On 2015/06/26 21:46:16, ncarter wrote:
> Add a comment here (or at the bottom of the function) to describe why doing
> nothing is okay on non-windows. You could put it, if you want, in a comment
in
> an (otherwise empty) #else clause.
Done.
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager.cc:1140: for (size_t rIndex = 0; rIndex
< resources_.size(); ++rIndex) {
On 2015/06/26 21:46:16, ncarter wrote:
> "i", "index", "resource", or "resource_index", but definitely not "rIndex"
Done.
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager.cc:1141: size_t privateWS =
working_set_snapshot_->GetPrivateWorkingSet(
On 2015/06/26 21:46:17, ncarter wrote:
> private_working_set
Done.
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager.cc:1162: PopulateCache();
On 2015/06/26 21:46:16, ncarter wrote:
> Maybe: RefreshPhysicalMemoryFromWorkingSetSnapshot()
Done.
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_manager/task_manager.h (right):
https://codereview.chromium.org/1181263005/diff/100001/chrome/browser/task_ma...
chrome/browser/task_manager/task_manager.h:29: class PrivateWorkingSetSnapshot;
On 2015/06/26 21:46:17, ncarter wrote:
> Nit: sort
Done.
https://codereview.chromium.org/1181263005/diff/140001/base/process/private_w...
File base/process/private_working_set_snapshot.h (right):
https://codereview.chromium.org/1181263005/diff/140001/base/process/private_w...
base/process/private_working_set_snapshot.h:48: class PDHDummyVerifierTraits {
On 2015/06/26 21:50:46, ncarter wrote:
> Does using the preexisting DummyVerifier not compile? I thought that
PDH_HQUERY
> was just ultimately a typedef for HANDLE.
You're right, it is. I also followed the pattern you linked to and got a much
more concise PDHHandleTraits. The line-count overhead seems much more reasonable
now.
The CQ bit was checked by brucedawson@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/1181263005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Reduced PdhGetFormattedCounterArray calls and now presubmit clean
PTAL - it should be perfect now. The ng build errors are now fixed (the fix was easy once I realized that they were 'gn' build errors not 'ng' build errors). The include order is now presubmit clean. Checking of the presubmit.py code confirms that #if defined(OS_WIN) in private_working_set_snapshot_win.cc is required in order to break the includes into separate scopes: https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=_Ch... In private_working_set_snapshot_win.h the include order is similarly forced. The #if defined(OS_WIN) would be best at the top of the file but it needs to be farther down in order to allow having the two Windows-specific files in their own sorted order. This change points out that the presubmit checks for include order are rather fragile, but fixing that is *well* beyond the scope of this change. The only remaining question is whether private_ws/private_ws_handle/private_ws_query/private_ws_data locals and struct members need to be changed to the more verbose private_working_set prefix.
lgtm with just a couple small comments. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... File base/process/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:59: NULL, &new_counters.process_id_handle) != ERROR_SUCCESS) { On 2015/06/27 00:13:05, brucedawson wrote: > "git cl format" did it somewhat differently from that and it looks tidy so I'm > going to defer to it. That is, the new version of this is indented according to > git cl format. As a rule you won't see me disagreeing with git cl format's choices. It produces really tidy code. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:102: if (PdhGetFormattedCounterArray(counter_pair.private_ws_handle, On 2015/06/27 00:13:05, brucedawson wrote: > The calls to PdhGetFormattedCounterArray are very cheap (they never show up on > the profile, only PdhCollectQueryData does) so there is not much gain. However > it should allow slightly less code, which is a good thing. I'll try it. > > Aside: the check for matching buffer sizes/counts was broken. It was intended to > continue if *either* value differed, not if both. Probably irrelevant, but > *oops*. I'm glad you caught this. Thanks for pointing it out. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:38: } // namespace win Add a blank line before the namespace close https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:46: PrivateWorkingSetSnapshot(); You should still declare a destructor in the .h with an empty body in the .cc. An implicitly-declared destructor is an inline public member of its class, and there's a fatwa issued against nontrivial inlines: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:74: PDH_HCOUNTER private_ws_handle = nullptr; I'm okay with leaving these as ws, but there's a good chance the base/ OWNER for this CL may insist on expansion. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:13: #endif I'm fine For questions like this the best thing to do is to consult code search for examples. I did a search for [file:_win.cc "include <windows"] https://code.google.com/p/chromium/codesearch#search/&q=file:_win.cc%20%22inc... And it looks like what many other _win.cc files do, is to put the the std:: and win32 #includes in the same block, with no OS ifdef. It stands to reason that if the presubmit doesn't know that windows.h is platform specific, it is treating it the same as <algorithm>, etc. https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce... https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/wi... So try like this? #include "base/process/private_working_set_snapshot.h" #include <algorithm> #include <pdh.h> #include <pdhmsg.h> #include <windows.h> #include "base/numerics/safe_conversions.h" #include "base/win/windows_version.h" --- having said all that, I'm fine with the #if defined(OS_WIN) too, as you have done. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:95: reinterpret_cast<PDH_FMT_COUNTERVALUE_ITEM*>(&buffer[0]); I'm fine leaving this as &buffer[0], but do we need to do something to defend against buffer_size1 against being zero? Lots of ways to solve that, but my understanding was that buffer[0], if empty, would be undefined behavior in theory, and could hit debug-mode STL checks in practice. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot_win_unittest.cc (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:51: << "GetPrivateWorkingSet should increase as we allocate more memory"; Fingers crossed on this test not being flaky.
Include order fixes, outline destructor, zero check
brucedawson@chromium.org changed reviewers: + thakis@chromium.org - rvargas@chromium.org
Resurrce having two query calls to PdhGetFormattedCounterArray
ncarter - I fixed the remaining nits. I also had to resurrect the second query call to PdhGetFormattedCounterArray, since otherwise the results are flaky (but only when running multi-process tests). Very odd API contract. Nico - can you review the base changes? https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:38: } // namespace win On 2015/06/29 20:03:19, ncarter wrote: > Add a blank line before the namespace close Done. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:46: PrivateWorkingSetSnapshot(); On 2015/06/29 20:03:19, ncarter wrote: > You should still declare a destructor in the .h with an empty body in the .cc. > An implicitly-declared destructor is an inline public member of its class, and > there's a fatwa issued against nontrivial inlines: > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Ah - right. Avoiding nontrivial hidden inlined functions. Done. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot.h:74: PDH_HCOUNTER private_ws_handle = nullptr; On 2015/06/29 20:03:19, ncarter wrote: > I'm okay with leaving these as ws, but there's a good chance the base/ OWNER for > this CL may insist on expansion. Let's see what happens... https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:13: #endif > --- having said all that, I'm fine with the #if defined(OS_WIN) too, as you have > done. I guess if I think of Windows header files as being C system header files then it all makes sense. I wonder if that is intended? Note that <pdh.h and <pdhmsg.h> are treated as C system headers so they have to go in front of <algorithm> (C++ system header). <windows.h> has an exception and can go literally anywhere! There is no consistent answer as to whether it should go before other Windows/system header files or after. I'm punting on that question by removing it (pdh.h includes what it needs) which is also a popular solution. Armed with all this knowledge I've also changed the includes in the header file - on non-Windows compiles it will use a bare minimum of includes. I'm working on a CL which will give more detail when the include order presubmit check fails - the sort of information I wish it had given me. It will distinguish between alphabetization errors and grouping errors - those are too subtle right now. Done. https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win.cc:95: reinterpret_cast<PDH_FMT_COUNTERVALUE_ITEM*>(&buffer[0]); On 2015/06/29 20:03:19, ncarter wrote: > I'm fine leaving this as &buffer[0], but do we need to do something to defend > against buffer_size1 against being zero? Lots of ways to solve that, but my > understanding was that buffer[0], if empty, would be undefined behavior in > theory, and could hit debug-mode STL checks in practice. Good point. I meant to add a zero-size check to avoid this. I've added one now (after the first call to PdhGetFormattedCounterArray). https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... File base/process/private_working_set_snapshot_win_unittest.cc (right): https://codereview.chromium.org/1181263005/diff/180001/base/process/private_w... base/process/private_working_set_snapshot_win_unittest.cc:51: << "GetPrivateWorkingSet should increase as we allocate more memory"; On 2015/06/29 20:03:19, ncarter wrote: > Fingers crossed on this test not being flaky. Done.
I'll give you the usual base review question "does this have to be in base?" :-) It looks like it's used in only one place in chrome/, no code in base/ depends on this new code, and the code adds a library dependency to base. (See also base/OWNERS for another "prefer to put code in places that aren't base" variant) https://codereview.chromium.org/1181263005/diff/220001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1181263005/diff/220001/base/BUILD.gn#newcode757 base/BUILD.gn:757: "pdh.lib", Hm, it's a bit unfortunate that base now forces clients to depend on pdh. https://codereview.chromium.org/1181263005/diff/220001/base/process/private_w... File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/220001/base/process/private_w... base/process/private_working_set_snapshot.h:86: bool operator<(const PidAndPrivateWorkingSet& other) const { Should this have a `CHECK(process_id != other.process_id || private_ws == other.private_ws) (or whatever the condition is, as this isn't a general operator< implementation)
The initial suggestion to put this in base came from comment #3: https://codereview.chromium.org/1181263005/#msg3 One quote from there is: "One strong immediate reason to structure this code for reuse, is that we currently have two task manager implementations (task_manager/ and task_management/) due to a refactor-rewrite that's in progress, and we want them to be querying the same APIs. A second consideration is that there are other memory UIs, like about:memory and about:memory-internals, that may benefit from a batched process stats gathering API." But, that doesn't mean it must be in base - just somewhere where it can be reused. Regarding the pdh.lib dependency, the delay load should ensure that processes that don't use this API won't pay for it. The appropriate sanity check in operator< (assuming it will only ever be used for Pdh* results) would be: CHECK(process_id != other.process_id); That is, any duplicate entries in the array are highly surprising. I'm not sure if such a check would really add value - I could go either way. Maybe a DCHECK to avoid over-reacting.
On 2015/06/30 00:27:09, brucedawson wrote: > The initial suggestion to put this in base came from comment #3: > https://codereview.chromium.org/1181263005/#msg3 > > One quote from there is: > > "One strong immediate reason to structure this code for reuse, is that we > currently have two task manager implementations (task_manager/ and > task_management/) due to a refactor-rewrite that's in progress, and we want them > to be querying the same APIs. They're both below chrome/ though, right? > A second consideration is that there are other > memory UIs, like about:memory and about:memory-internals, that may benefit from > a batched process stats gathering API." That's the "might be useful in the future" argument that base/OWNERS explicitly advises against :-) > But, that doesn't mean it must be in base - just somewhere where it can be > reused. > > Regarding the pdh.lib dependency, the delay load should ensure that processes > that don't use this API won't pay for it. > > The appropriate sanity check in operator< (assuming it will only ever be used > for Pdh* results) would be: > CHECK(process_id != other.process_id); > That is, any duplicate entries in the array are highly surprising. I'm not sure > if such a check would really add value - I could go either way. Maybe a DCHECK > to avoid over-reacting. DCHECK sounds fine to me. Without anything, it looks like an incorrect operator<, so a comment saying that this is safe because ids are always different is good, and if the comment is executable that's even better.
Adding missing files and 'git cl format'
PTAL. The core implementation has not changed - most code is untouched. Here are some notes on what has changed: - I moved the implementation from base to chrome\browser, with all the tweaks and movement which that entails. - pdh.lib is now pulled in using #pragma comment(lib, "pdh.lib") - this seems to be the way that it is done in chrome.dll. - I didn't add a DCHECK to the comparison function because I'm not sure why it isn't a "general operator< implementation" - it seems normal to me, it just treats private_ws as payload rather than part of the key. Adding a DCHECK would constrain its use in an odd way - probably harmless, but odd. The unit_tests pass, git cl try is looking good, and git cl format is happy.
latest patch set lgtm with one fix https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... File chrome/browser/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot.h:38: ScopedPDH; This could be a "using =" alias. https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot.h:86: return process_id < other.process_id; I read the comment about "incomplete operator< impl" as saying that one usually expects an operator< to on a pair-like struct to be lexicographic comparison over the two data members. At least 3 times in my career have I have seen bugs due to incorrectly implemented lexicographic comparison on structs (when it's not a strict weak ordering, quicksort does out-of-bounds reads), so seeing "operator<" is something like a PTSD trigger for me, and as a result of that I do support documenting that not looking at |private_ws| is what's actually intended. (We all agree that the code is correct) https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... File chrome/browser/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot_win.cc:111: if (buffer_size1 != buffer_size2 && item_count1 != item_count2) I think you regressed here back to the && bug. Should be ||
Comments, using, and one bug fix.
Thanks Nick, especially for finding the logic regression. Nico - I think it's all up to you now. https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... File chrome/browser/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot.h:38: ScopedPDH; On 2015/07/01 16:50:26, ncarter wrote: > This could be a "using =" alias. Done. https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot.h:86: return process_id < other.process_id; On 2015/07/01 16:50:26, ncarter wrote: > I read the comment about "incomplete operator< impl" as saying that one usually > expects an operator< to on a pair-like struct to be lexicographic comparison > over the two data members. > > At least 3 times in my career have I have seen bugs due to incorrectly > implemented lexicographic comparison on structs (when it's not a strict weak > ordering, quicksort does out-of-bounds reads), so seeing "operator<" is > something like a PTSD trigger for me, and as a result of that I do support > documenting that not looking at |private_ws| is what's actually intended. (We > all agree that the code is correct) I'll add a comment to make that crystal clear. I think the CHECK/DCHECK would be wrong but a comment to clarify intent is never a bad thing. https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... File chrome/browser/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private... chrome/browser/private_working_set_snapshot_win.cc:111: if (buffer_size1 != buffer_size2 && item_count1 != item_count2) On 2015/07/01 16:50:26, ncarter wrote: > I think you regressed here back to the && bug. Should be || Good catch. Thank you!
lgtm
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/1181263005/#ps300001 (title: "Comments, using, and one bug fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/f66d1ecf0cd7426393088c91f2af78ff1889bab4 Cr-Commit-Position: refs/heads/master@{#337096}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1219233002/ by sky@chromium.org. The reason for reverting is: This appears to have triggered a unit test failure on the vista bot. https://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/bu... output: (view as text) PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (run #1): [ RUN ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(32): error: Expected: (private_ws) > (2000000u), actual: 0 vs 2000000 c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(50): error: Expected: (private_ws3) > (private_ws2 + alloc_size / 2), actual: 0 vs 5000000 GetPrivateWorkingSet should increase as we allocate more memory [ FAILED ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (78 ms) PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (run #2): [ RUN ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(32): error: Expected: (private_ws) > (2000000u), actual: 0 vs 2000000 c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(50): error: Expected: (private_ws3) > (private_ws2 + alloc_size / 2), actual: 0 vs 5000000 GetPrivateWorkingSet should increase as we allocate more memory [ FAILED ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (47 ms) PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (run #3): [ RUN ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(32): error: Expected: (private_ws) > (2000000u), actual: 0 vs 2000000 c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(50): error: Expected: (private_ws3) > (private_ws2 + alloc_size / 2), actual: 0 vs 5000000 GetPrivateWorkingSet should increase as we allocate more memory [ FAILED ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (47 ms) PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (run #4): [ RUN ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(32): error: Expected: (private_ws) > (2000000u), actual: 0 vs 2000000 c:\b\build\slave\win_builder\build\src\chrome\browser\private_working_set_snapshot_win_unittest.cc(50): error: Expected: (private_ws3) > (private_ws2 + alloc_size / 2), actual: 0 vs 5000000 GetPrivateWorkingSet should increase as we allocate more memory [ FAILED ] PrivateWorkingSetSnapshotWinTest.FindPidSelfTest (47 ms).
I manually tested the Pdh APIs on Vista and found that they supported Private Working Set so I was surprised to see this Vista failure. I've fired off a Vista try-bot to make sure I can reproduce the problem, then I'll experiment on Vista to make sure I understand what's going on, but ultimately the version check will probably just get moved up one OS version - not a big deal since we have very few Vista users and the fallback code will work as before. Note that this was a test failure but it would not have affected the task manager on Vista which would silently have fallen back to the old technique, but the test is useful for pointing out that this is happening. The default try bots do not include XP or Vista - something to be aware of.
Pdh to get private working set apparently doesn't work on Vista. A surprise (I had tested it manually), but good to know. I've uploaded a patch which changes the version check to use <= instead of < so that Vista is excluded from the tests and from being attempted at all. I reproed the failure on the try bots and I just fired off win8 and Vista try bots with the fix to verify that it works. I won't try relanding this until after the long weekend.
PTAL. I can't reproduce the Pdh* failures on my Vista test machine, but the try bots certainly agree that Pdh* is not a reliable way to get private working set on Vista. So, the code and test is now disabled on Vista. Note that if this failure had happened in the wild instead of on the try bots then it would have been a graceful failure -- the old code would have automatically run.
Adjust high threshold so it works with Dr Memory.
still lgtm
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1181263005/#ps340001 (title: "Adjust high threshold so it works with Dr Memory.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/a762bc76dccf669d5a1cdfb588d8dafe1d0bd18f Cr-Commit-Position: refs/heads/master@{#337886} |
