Only Rank Oldest N Tabs for TabRanker |
|||||||
Issue descriptionCurrent TabRanker https://cs.chromium.org/chromium/src/chrome/browser/resource_coordinator/tab_manager.cc?rcl=036350672c7cc81d99da59975900b7a07331aaea&l=276 either rerank all tabs (Enabled) or none (Disabled). But the TabManager only discards one tab very time. So there could be the a potential problem: Let us say the user has opened 1000 tabs. He is in memory pressure. Suppose: (1) Tabs are sorted by least recent used as: tab_0, tab_1, tab_2, ..., tab_1000 (2) Tabs are sorted by TabRanker score as: tab_1000, tab_20, tab_22, tab_23, tab_24, ... (3) The user eventually reload page tab_1000, tab_2, tab_4, tab_5, and then closed the browser. In the case above, TabRanker have averagely better result if TabManager discards 5 tabs every time. But currently TabManager discards one tab very time (which is meant to be), then there is a chance the bad first one makes the TabRanker result much worse. Currently it's hard to verify this unless we reconstruct a users' full tab list. But it can be very helpful if we have the options to set how many tabs to rerank.
,
Oct 24
,
Oct 24
,
Oct 24
,
Oct 24
,
Oct 24
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0669c3d40b0789c6f94e7967821f422e46721630 commit 0669c3d40b0789c6f94e7967821f422e46721630 Author: Charles Zhao <charleszhao@chromium.org> Date: Thu Oct 25 06:48:09 2018 Only re-rank oldest N tabs with TabRanker. We use least recently used index: lru_index = mru.total - mru.index - 1 To only Score the oldest N tabs. N is configurable from Finch and defaulted as kMaxInt. The reason for this change: Since TabManager discards one tab every time, we suspect any error of TabRanker among all tabs can make a bad first candidate which leads to not so good tab discarding result. By only re-rank oldest N tabs, we can shrink the chance of making such mistakes (in the mean time, less likely to find the best candidate to discards). Related histogram: "TabManager.Discarding.ReloadCount", (expect to go down) "TabManager.Discarding.DiscardToReloadTime", (expect to go up) Bug:897590 Change-Id: I76d51605cea75f5db28250594bba35ae0a59cae5 Reviewed-on: https://chromium-review.googlesource.com/c/1293060 Commit-Queue: Charles . <charleszhao@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org> Cr-Commit-Position: refs/heads/master@{#602625} [modify] https://crrev.com/0669c3d40b0789c6f94e7967821f422e46721630/chrome/browser/resource_coordinator/tab_activity_watcher.cc [modify] https://crrev.com/0669c3d40b0789c6f94e7967821f422e46721630/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc [modify] https://crrev.com/0669c3d40b0789c6f94e7967821f422e46721630/chrome/browser/resource_coordinator/tab_manager_features.cc [modify] https://crrev.com/0669c3d40b0789c6f94e7967821f422e46721630/chrome/browser/resource_coordinator/tab_manager_features.h
,
Dec 10
,
Dec 10
What are the results of this work? Did histograms or other observations moved in the expected direction? |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by charleszhao@chromium.org
, Oct 23