New issue
Advanced search Search tips

Issue 911474 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

TabRanker is not recording chromeos discards correctly.

Project Member Reported by charleszhao@chromium.org, Dec 4

Issue description

TabRanker 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



 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6378752b810e6122744cefac544c8d84278dcc71

commit 6378752b810e6122744cefac544c8d84278dcc71
Author: Charles Zhao <charleszhao@chromium.org>
Date: Fri Dec 07 06:26:07 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 

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-Commit-Position: refs/heads/master@{#614623}
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_activity_watcher.cc
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_activity_watcher.h
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_activity_watcher_browsertest.cc
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_manager.cc
[modify] https://crrev.com/6378752b810e6122744cefac544c8d84278dcc71/chrome/browser/resource_coordinator/tab_manager.h

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 11

Labels: merge-merged-3626
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

Labels: CommitLog-Audit-Violation Merge-Without-Approval M-72
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 -- 
Labels: Merge-Merged-72-3626
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