Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(337)

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.

Description

Make 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+382 lines, -6 lines) Patch
M base/process/process_metrics.h View 1 chunk +2 lines, -1 line 0 comments Download
M base/process/process_metrics_win.cc View 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/private_working_set_snapshot.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +110 lines, -0 lines 0 comments Download
A chrome/browser/private_working_set_snapshot_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +161 lines, -0 lines 0 comments Download
A chrome/browser/private_working_set_snapshot_win_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +55 lines, -0 lines 0 comments Download
M chrome/browser/task_manager/task_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/task_manager/task_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +41 lines, -4 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 55 (11 generated)
brucedawson
I want to improve the task manager's memory column on Windows (it currently burns a ...
5 years, 6 months ago (2015-06-13 00:09:22 UTC) #1
ncarter (slow)
+jschuh -- to weigh in on whether these new-to-chrome win32 API calls are a good ...
5 years, 6 months ago (2015-06-17 22:19:11 UTC) #3
brucedawson
Thanks for the detailed feedback. The reason this code is faster is that Windows keeps ...
5 years, 6 months ago (2015-06-17 22:42:38 UTC) #4
jschuh
We chatted a bit in the office about the implementation and some API refinement. I ...
5 years, 6 months ago (2015-06-18 00:12:14 UTC) #5
brucedawson
Add spacing for better readability.
5 years, 6 months ago (2015-06-26 00:05:53 UTC) #7
brucedawson
Comment tweak
5 years, 6 months ago (2015-06-26 00:09:17 UTC) #8
brucedawson
PTAL After getting initial feedback from nick@ and jschuh@ I've reworked this code that it ...
5 years, 6 months ago (2015-06-26 00:12:10 UTC) #9
brucedawson
Add missing include
5 years, 6 months ago (2015-06-26 00:46:30 UTC) #10
brucedawson
Use GenericScopedHandle to clean up PDH_HQUERY
5 years, 6 months ago (2015-06-26 21:32:48 UTC) #11
brucedawson
I just updated the patch to use GenericScopedHandle instead of manually calling PdhCloseQuery. - Pros ...
5 years, 6 months ago (2015-06-26 21:36:01 UTC) #12
ncarter (slow)
I just finished reviewing the older patch set. My feedback is mostly stylistic -- from ...
5 years, 6 months ago (2015-06-26 21:46:17 UTC) #13
ncarter (slow)
https://codereview.chromium.org/1181263005/diff/140001/base/process/private_working_set_snapshot.h File base/process/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/140001/base/process/private_working_set_snapshot.h#newcode48 base/process/private_working_set_snapshot.h:48: class PDHDummyVerifierTraits { Does using the preexisting DummyVerifier not ...
5 years, 6 months ago (2015-06-26 21:50:46 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/140001
5 years, 6 months ago (2015-06-26 22:26:14 UTC) #16
commit-bot: I haz the power
Dry run: Exceeded global retry quota
5 years, 6 months ago (2015-06-26 22:59:36 UTC) #18
brucedawson
Lots of updates based on feedback - thanks. The code is now "git cl format" ...
5 years, 6 months ago (2015-06-27 00:13:06 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/160001
5 years, 6 months ago (2015-06-27 00:15:16 UTC) #21
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 5 months ago (2015-06-27 01:19:01 UTC) #23
brucedawson
Reduced PdhGetFormattedCounterArray calls and now presubmit clean
5 years, 5 months ago (2015-06-29 18:11:03 UTC) #24
brucedawson
PTAL - it should be perfect now. The ng build errors are now fixed (the ...
5 years, 5 months ago (2015-06-29 18:24:53 UTC) #25
ncarter (slow)
lgtm with just a couple small comments. https://codereview.chromium.org/1181263005/diff/100001/base/process/private_working_set_snapshot_win.cc File base/process/private_working_set_snapshot_win.cc (right): https://codereview.chromium.org/1181263005/diff/100001/base/process/private_working_set_snapshot_win.cc#newcode59 base/process/private_working_set_snapshot_win.cc:59: NULL, &new_counters.process_id_handle) ...
5 years, 5 months ago (2015-06-29 20:03:19 UTC) #26
brucedawson
Include order fixes, outline destructor, zero check
5 years, 5 months ago (2015-06-29 20:53:52 UTC) #27
brucedawson
Resurrce having two query calls to PdhGetFormattedCounterArray
5 years, 5 months ago (2015-06-29 21:47:46 UTC) #29
brucedawson
ncarter - I fixed the remaining nits. I also had to resurrect the second query ...
5 years, 5 months ago (2015-06-29 22:09:52 UTC) #30
Nico
I'll give you the usual base review question "does this have to be in base?" ...
5 years, 5 months ago (2015-06-30 00:04:43 UTC) #31
brucedawson
The initial suggestion to put this in base came from comment #3: https://codereview.chromium.org/1181263005/#msg3 One quote ...
5 years, 5 months ago (2015-06-30 00:27:09 UTC) #32
Nico
On 2015/06/30 00:27:09, brucedawson wrote: > The initial suggestion to put this in base came ...
5 years, 5 months ago (2015-06-30 00:38:54 UTC) #33
brucedawson
Adding missing files and 'git cl format'
5 years, 5 months ago (2015-06-30 23:18:20 UTC) #34
brucedawson
PTAL. The core implementation has not changed - most code is untouched. Here are some ...
5 years, 5 months ago (2015-07-01 00:21:01 UTC) #35
ncarter (slow)
latest patch set lgtm with one fix https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private_working_set_snapshot.h File chrome/browser/private_working_set_snapshot.h (right): https://codereview.chromium.org/1181263005/diff/280001/chrome/browser/private_working_set_snapshot.h#newcode38 chrome/browser/private_working_set_snapshot.h:38: ScopedPDH; This ...
5 years, 5 months ago (2015-07-01 16:50:27 UTC) #36
brucedawson
Comments, using, and one bug fix.
5 years, 5 months ago (2015-07-01 17:29:48 UTC) #37
brucedawson
Thanks Nick, especially for finding the logic regression. Nico - I think it's all up ...
5 years, 5 months ago (2015-07-01 17:47:40 UTC) #38
Nico
lgtm
5 years, 5 months ago (2015-07-01 17:54:43 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/300001
5 years, 5 months ago (2015-07-01 18:03:11 UTC) #42
commit-bot: I haz the power
Committed patchset #16 (id:300001)
5 years, 5 months ago (2015-07-01 19:40:47 UTC) #43
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/f66d1ecf0cd7426393088c91f2af78ff1889bab4 Cr-Commit-Position: refs/heads/master@{#337096}
5 years, 5 months ago (2015-07-01 19:41:38 UTC) #44
sky
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/1219233002/ by sky@chromium.org. ...
5 years, 5 months ago (2015-07-01 21:36:22 UTC) #45
brucedawson
I manually tested the Pdh APIs on Vista and found that they supported Private Working ...
5 years, 5 months ago (2015-07-01 22:16:22 UTC) #46
brucedawson
Pdh to get private working set apparently doesn't work on Vista. A surprise (I had ...
5 years, 5 months ago (2015-07-02 00:10:39 UTC) #47
brucedawson
PTAL. I can't reproduce the Pdh* failures on my Vista test machine, but the try ...
5 years, 5 months ago (2015-07-06 17:51:32 UTC) #48
brucedawson
Adjust high threshold so it works with Dr Memory.
5 years, 5 months ago (2015-07-06 19:58:04 UTC) #49
ncarter (slow)
still lgtm
5 years, 5 months ago (2015-07-08 17:57:36 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263005/340001
5 years, 5 months ago (2015-07-08 17:58:24 UTC) #53
commit-bot: I haz the power
Committed patchset #18 (id:340001)
5 years, 5 months ago (2015-07-08 19:37:06 UTC) #54
commit-bot: I haz the power
5 years, 5 months ago (2015-07-08 19:38:13 UTC) #55
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/a762bc76dccf669d5a1cdfb588d8dafe1d0bd18f
Cr-Commit-Position: refs/heads/master@{#337886}

Powered by Google App Engine
This is Rietveld 408576698