New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 899002 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Failed to reschedule eviction if flushed due to deadline

Project Member Reported by backer@chromium.org, Oct 25

Issue description

crrev.com/c/1284435 introduced a bug where if we flushed due to deadline
but had potentially evictable resources for the future, we would not
ScheduleEvictExpiredResourcesIn

This could leave us with evictable resources being expired but not freed until some other resource was released. This explains multiple memory regressions on the bots (to be deduped here). go/chromiumdash says bug only exists on head (haven't branched M72 yet).
 
Cc: maxlg@chromium.org backer@chromium.org
 Issue 898898  has been merged into this issue.
Cc: hjd@google.com
 Issue 897713  has been merged into this issue.
Cc: tmathmeyer@google.com
 Issue 897330  has been merged into this issue.
 Issue 897129  has been merged into this issue.
Cc: sullivan@chromium.org
 Issue 896344  has been merged into this issue.
Issue 896779 has been merged into this issue.
Issue 896809 has been merged into this issue.
Issue 896811 has been merged into this issue.
 Issue 896816  has been merged into this issue.
 Issue 896823  has been merged into this issue.
 Issue 896824  has been merged into this issue.
Issue 896825 has been merged into this issue.
 Issue 897131  has been merged into this issue.
 Issue 900011  has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 30

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

commit 4c0d206402adde38587d8663c7db491f2b366387
Author: Jonathan Backer <backer@chromium.org>
Date: Tue Oct 30 16:12:21 2018

Reschedule EvictExpiredResources

crrev.com/c/1284435 introduced a bug where if we flushed due to deadline
but had potentially evictable resources for the future, we would not
ScheduleEvictExpiredResourcesIn

Also added a micro-optimization where if we evict all resources
OnMemoryPressure, we also flush to reclaim it immediately.

Added unittests to cover flushing logic. Dependency injected fake
clock to make test robust.

Bug:  899002 
Change-Id: I50ca934170ebe76e7f48326e53e60df14c32c078
Reviewed-on: https://chromium-review.googlesource.com/c/1299601
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603921}
[modify] https://crrev.com/4c0d206402adde38587d8663c7db491f2b366387/cc/resources/resource_pool.cc
[modify] https://crrev.com/4c0d206402adde38587d8663c7db491f2b366387/cc/resources/resource_pool.h
[modify] https://crrev.com/4c0d206402adde38587d8663c7db491f2b366387/cc/resources/resource_pool_unittest.cc

Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
Doesn't look like the fix reduced memory usage back to previous values.
@#9: Can you be more specific? This fixed some of the regressions (e.g. https://chromeperf.appspot.com/group_report?bug_id=897713) 
Status: Fixed (was: Assigned)
Ah, sorry. https://chromeperf.appspot.com/group_report?bug_id=900011 doesn't look to have improved, but it hasn't gotten results uploaded for several runs, so it might be actually fixed there and just not visible yet.

As for the other ones, the visible area on the graph didn't go to the most recent results, so I didn't see the improvements. I'll re-open again if 90001 doesn't improve after getting more results.
Cc: tdres...@chromium.org
 Issue 902019  has been merged into this issue.
Issue 903292 has been merged into this issue.
Cc: alexclarke@chromium.org
 Issue 903108  has been merged into this issue.
 Issue 903107  has been merged into this issue.
Issue 898909 has been merged into this issue.

Sign in to add a comment