New issue
Advanced search Search tips

Issue 829044 link

Starred by 9 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Download history being loaded at browser startup

Project Member Reported by asanka@chromium.org, Apr 4 2018

Issue description

Looks like we have a dependency chain from chrome::ProfileImpl -> content::StoragePartitionImpl -> content::BackgroundFetchContext -> chrome::BackgroundFetchDelegateImpl -> download::DownloadService -> download::BuildDownloadService() -> content::DownloadManagerImpl

The action of getting the download manager in download::BuildDownloadService() results in invoking DownloadCoreServiceImpl::GetDownloadManagerDelegate().

The latter instantiates the HistoryService and DownloadHistory.

DownloadHistory loads the downloads history, and upon completion, initiates the download history file existence check (DownloadManager::CheckForHistoryFilesRemoval)

All this happens at every browser startup. Loading the entire downloads history and checking for the existence of all the downloads is quite expensive.

We could make this a little easier on browser startup by not loading the download history, or at least forgoing the file existence check until someone loads chrome://downloads. Note that opening the downloads page or running an extension that invokes download.search() independently invoke the history file existence check. We may be able to trivially get rid of the same check at browser startup.

 
This came up in Issue 828081.
Owner: qin...@chromium.org
Status: Assigned (was: Untriaged)
Another thing to think about is to delay loading the background fetch service. Unless it needs to start work right away (unlikely), it could be delayed until sometime after browser startup. 
Cc: jsc...@chromium.org
Labels: -Pri-2 M-68 Pri-1
Perhaps I'm misreading the suggestion, but we shouldn't be doing the existence checks at all if we don't need to. Even if it's out of the immediate startup path, it would still impact performance generally (shortly after startup). And on top of that, we really should have a baseline of not accessing a resource until and unless we need to (both for performance and to avoid the kind of confusion we just dealt with).

I'm also bumping the priority on this bug and setting a target milestone, but ideally I'd like to see this fixed sooner and merged to both M66 and M67.

#4: Yup. The suggestion in the short run is to skip the existence check at the time downloads history finishes loading. The places where it's needed (chrome://downloads, download home, and download extensions that use download.search()) already invoke the existence check manually.

The additional suggestion for delaying background fetch service is to reduce the work the browser has to do at profile creation time. Background fetches don't need to activate immediately, so it can yield to other higher priority tasks.
asanka@ - Gotcha, sorry for misreading that.
Labels: Restrict-AddIssueComment-EditIssue
If you are hitting this issue and you want a fix right now then go to chrome://downloads in your browser, go to the menu in the top right, and select Clear All. That will clear Chrome's list of downloaded files so that it won't have any files to existence-check at startup. If you have a large list of downloaded files then this will improve startup time slightly.

This will be fixed so if you do nothing the problem will also go away, once the fix ships to your version of Chrome.
Labels: Hotlist-ConOps
+ConOps hotlist as this may be related to some of our "Chrome is slow at startup" reports.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 11 2018

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

commit dee931eba042a6023229ec856c987f195956aacb
Author: Min Qin <qinmin@chromium.org>
Date: Wed Apr 11 02:09:47 2018

Don't perform file existence check when loading DownloadHistory

DownloadHistory is currently loaded on browser start up.
And that will trigger file existence check, which is very costly.
File existence check is not needed until user opens chrome://downloads page
or loads a download extension.
Currently android download home and desktop downloads page already
checks for file existence on startup.
And download search api will also perform file existence check.
So this file existence check on DownloadHistory ctor is not needed.

BUG=829044

Change-Id: I3558d2baa6952eda6adde4eec1f3c79efe8e4d11
Reviewed-on: https://chromium-review.googlesource.com/996576
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549725}
[modify] https://crrev.com/dee931eba042a6023229ec856c987f195956aacb/chrome/browser/download/download_browsertest.cc
[modify] https://crrev.com/dee931eba042a6023229ec856c987f195956aacb/chrome/browser/download/download_history.cc
[modify] https://crrev.com/dee931eba042a6023229ec856c987f195956aacb/chrome/browser/download/download_history_unittest.cc
[modify] https://crrev.com/dee931eba042a6023229ec856c987f195956aacb/chrome/browser/download/download_ui_controller_unittest.cc
[modify] https://crrev.com/dee931eba042a6023229ec856c987f195956aacb/chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc

Sign in to add a comment