TabRanker is not recording chromeos discards correctly. |
|||||
Issue descriptionTabRanker query time was landed in this cl: https://chromium-review.googlesource.com/c/chromium/src/+/1350445 In order to avoid unwanted call to TabManager::GetSortedLifecycleUnits() We have created this TabManager::GetSortedLifecycleUnitsFromTabRanker() to log the query we want. But this was wrong on two aspect: (1) TabManagerDelegate::LowMemoryKillImpl is still calling GetSortedLifecycleUnits(), so this won't be logged. (2) TabManagerDelegate::Candidate is calling lifecycle_unit_->GetSortKey() for inferencing sorting, Which means TabRanker model won't work for these cases. The fix: (1) Change TabManager, TabLifecycleUnit back to their previous code, which are sort by TabRanker when it's enabled. (2) Add TabActivityWatcher::LogOldestNTabFeatures() and call this function in TabManager::DiscardTab
,
Dec 10
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f66a1b2eed922f76f1dd13af6d76cf84f494d84 commit 0f66a1b2eed922f76f1dd13af6d76cf84f494d84 Author: Charles Zhao <charleszhao@chromium.org> Date: Tue Dec 11 00:28:29 2018 TabRanker: Fix Query time logging. TabRanker query time logging was landed in this cl: https://chromium-review.googlesource.com/c/chromium/src/+/1350445 TabManager::GetSortedLifecycleUnitsFromTabRanker() was created to bypass calls to TabManager::GetSortedLifecycleUnits() But this was wrong on two aspects: (1) TabManagerDelegate::LowMemoryKillImpl is still calling GetSortedLifecycleUnits(), so ChromeOS discards are all skipped. (2) TabManagerDelegate::Candidate is calling lifecycle_unit_->GetSortKey() for inferring sorting, Which means TabRanker model won't work for ChromeOS discards. The fix: (1) Change TabManager, TabLifecycleUnit back to their previous code, which were sorting LifecycleUnits by TabRanker when it's enabled. (2) Add TabActivityWatcher::LogOldestNTabFeatures() and call this function in TabManager::DiscardTab for query time logging. Bug: 911474 TBR=charleszhao@chromium.org (cherry picked from commit 6378752b810e6122744cefac544c8d84278dcc71) Change-Id: I103e8237cd96ea87667536b7b44b27fe8772abb4 Reviewed-on: https://chromium-review.googlesource.com/c/1360471 Reviewed-by: Christopher Morin <cmtm@google.com> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org> Commit-Queue: Charles . <charleszhao@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#614623} Reviewed-on: https://chromium-review.googlesource.com/c/1370153 Reviewed-by: Charles . <charleszhao@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#245} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_activity_watcher.cc [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_activity_watcher.h [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_manager.cc [modify] https://crrev.com/0f66a1b2eed922f76f1dd13af6d76cf84f494d84/chrome/browser/resource_coordinator/tab_manager.h
,
Dec 19
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 0f66a1b2eed922f76f1dd13af6d76cf84f494d84 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f66a1b2eed922f76f1dd13af6d76cf84f494d84 Commit: 0f66a1b2eed922f76f1dd13af6d76cf84f494d84 Author: charleszhao@chromium.org Commiter: charleszhao@chromium.org Date: 2018-12-11 00:28:29 +0000 UTC TabRanker: Fix Query time logging. TabRanker query time logging was landed in this cl: https://chromium-review.googlesource.com/c/chromium/src/+/1350445 TabManager::GetSortedLifecycleUnitsFromTabRanker() was created to bypass calls to TabManager::GetSortedLifecycleUnits() But this was wrong on two aspects: (1) TabManagerDelegate::LowMemoryKillImpl is still calling GetSortedLifecycleUnits(), so ChromeOS discards are all skipped. (2) TabManagerDelegate::Candidate is calling lifecycle_unit_->GetSortKey() for inferring sorting, Which means TabRanker model won't work for ChromeOS discards. The fix: (1) Change TabManager, TabLifecycleUnit back to their previous code, which were sorting LifecycleUnits by TabRanker when it's enabled. (2) Add TabActivityWatcher::LogOldestNTabFeatures() and call this function in TabManager::DiscardTab for query time logging. Bug: 911474 TBR=charleszhao@chromium.org (cherry picked from commit 6378752b810e6122744cefac544c8d84278dcc71) Change-Id: I103e8237cd96ea87667536b7b44b27fe8772abb4 Reviewed-on: https://chromium-review.googlesource.com/c/1360471 Reviewed-by: Christopher Morin <cmtm@google.com> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org> Commit-Queue: Charles . <charleszhao@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#614623} Reviewed-on: https://chromium-review.googlesource.com/c/1370153 Reviewed-by: Charles . <charleszhao@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#245} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Dec 7