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

Issue 1219233002: Revert of Make task manager memory data more efficient and meaningful. (Closed)

Created:
5 years, 5 months ago by sky
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

Revert of Make task manager memory data more efficient and meaningful. (patchset #16 id:300001 of https://codereview.chromium.org/1181263005/) Reason for revert: 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/builds/57386 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) Original issue's 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} TBR=nick@chromium.org,jschuh@chromium.org,rsesek@chromium.org,thakis@chromium.org,brucedawson@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=484433, 494210 Committed: https://crrev.com/b98685d8a6254479c846441ca8d7acfc374abf67 Cr-Commit-Position: refs/heads/master@{#337117}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -378 lines) Patch
M base/process/process_metrics.h View 1 chunk +1 line, -2 lines 0 comments Download
M base/process/process_metrics_win.cc View 1 chunk +1 line, -1 line 0 comments Download
D chrome/browser/private_working_set_snapshot.h View 1 chunk +0 lines, -110 lines 0 comments Download
D chrome/browser/private_working_set_snapshot_win.cc View 1 chunk +0 lines, -160 lines 0 comments Download
D chrome/browser/private_working_set_snapshot_win_unittest.cc View 1 chunk +0 lines, -52 lines 0 comments Download
M chrome/browser/task_manager/task_manager.h View 3 chunks +0 lines, -9 lines 0 comments Download
M chrome/browser/task_manager/task_manager.cc View 5 chunks +4 lines, -41 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
sky
Created Revert of Make task manager memory data more efficient and meaningful.
5 years, 5 months ago (2015-07-01 21:36:22 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219233002/1
5 years, 5 months ago (2015-07-01 21:36:48 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 5 months ago (2015-07-01 21:38:37 UTC) #3
commit-bot: I haz the power
5 years, 5 months ago (2015-07-01 21:39:25 UTC) #4
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/b98685d8a6254479c846441ca8d7acfc374abf67
Cr-Commit-Position: refs/heads/master@{#337117}

Powered by Google App Engine
This is Rietveld 408576698