DescriptionRevert 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 #
Created: 5 years, 5 months ago
(Patch set is too large to download)
Messages
Total messages: 4 (0 generated)
|