Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507753003:
Do not precache when the cache size is small (Closed)
|
Created:
4 years, 1 month ago by jamartin Modified:
4 years ago CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, darin-cc_chromium.org, wifiprefetch-reviews_google.com Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not precache when the cache size is small
BUG=649495
Committed: https://crrev.com/305b67f08c9aca25fc0abdaf4399deb3dd89e2b8
Cr-Commit-Position: refs/heads/master@{#437985}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Use IO thread and report UMA on the cache size #
Total comments: 8
Patch Set 3 : All data member are accessed from the UI thread #Patch Set 4 : Comment on the IO thread #
Total comments: 2
Patch Set 5 : scoped_refptr #
Total comments: 8
Patch Set 6 : Readability improvements #Patch Set 7 : Sync'ed to HEAD #Patch Set 8 : Cannot use WeakPtr across threads #Patch Set 9 : Forgot dependency on variations #Patch Set 10 : Wrong dependency name #Patch Set 11 : Sync'ed to HEAD #
Messages
Total messages: 72 (40 generated)
jamartin@chromium.org changed reviewers: + twifkak@chromium.org
The CQ bit was checked by jamartin@chromium.org
The CQ bit was unchecked by jamartin@chromium.org
The CQ bit was checked by jamartin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:140: OnDone(); Add an UMA for the cache_size_bytes when this happens. (Ideally we'd log all the error conditions (for which this is currently 0) in an enum, but maybe wait until we discover that's a problem?) https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:168: // Continue with OnGetUnfinishedWorkDone only if the size of the cache is Perhaps this check should be inside OnGetUnfinishedWorkDone only if the prefetch is new, rather than before it. If there is unfinished work, then the client had a big enough cache at one point, so it is safe to proceed without checking again. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:175: ->GetURLRequestContext() May return null? https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c... https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:176: ->GetURLRequestContext() May return null: https://cs.chromium.org/chromium/src/net/url_request/url_request_context_gett... https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:177: ->http_transaction_factory() May return null? https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?rc... https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.h:58: constexpr int kMinCacheSizeBytes = 40e6; FYI 40e6 is a float literal, which (by way of the constexpr) is getting cast at compile time to an int. In this case, 40e6 is precise, so it works out. It feels weird, but 53 bits ought to be enough for anybody, so I think it's fine. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.h:167: void OnCacheBackendReceived(int net_error_code); Add function doc. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:278: ASSERT_EQ(cache_backend, cache->GetCurrentBackend()); (nit/opt) These asserts just verify that the test was written correctly (and that its preconditions continue to be true). For that, I usually use CHECK instead of ASSERT, just for documentation's sake. (Ditto below.) https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:290: ->GetCurrentBackend() Use cache_backend from above?
https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:182: base::Unretained(this))); I think you should use AsWeakPtr() instead?
https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:118: if (net_error_code == net::OK) { Add a DCHECK_CURRENTLY_ON here? Also, are you sure the callback is called on the same thread? I can't find evidence to convince myself one way or the other.
https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:168: // Continue with OnGetUnfinishedWorkDone only if the size of the cache is On 2016/11/17 21:52:04, twifkak wrote: > Perhaps this check should be inside OnGetUnfinishedWorkDone only if the prefetch > is new, rather than before it. If there is unfinished work, then the client had > a big enough cache at one point, so it is safe to proceed without checking > again. Actually, my next CL will want to pass the cache backend into PrecacheFetcher (even for resumes), so it doesn't matter.
Description was changed from ========== Do not precache when the cache size is small BUG=649495 ========== to ========== Do not precache when the cache size is small BUG=649495 ==========
jamartin@chromium.org changed reviewers: + gavinp@chromium.org, rkaplow@chromium.org
Addressed twifkak's comments. Added gavinp@ as reviewer due to the new DEPS on the cache side of net. QQ: Is there an easier way to get the maximum size of the cache? Added rkaplow@ as reviewer due to new histogram definition. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:118: if (net_error_code == net::OK) { On 2016/11/18 00:18:00, twifkak wrote: > Add a DCHECK_CURRENTLY_ON here? Also, are you sure the callback is called on the > same thread? I can't find evidence to convince myself one way or the other. Discussed offline. It seems that most of this needs to happen on the IO thread. Fixed. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:140: OnDone(); On 2016/11/17 21:52:04, twifkak wrote: > Add an UMA for the cache_size_bytes when this happens. (Ideally we'd log all the > error conditions (for which this is currently 0) in an enum, but maybe wait > until we discover that's a problem?) Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:168: // Continue with OnGetUnfinishedWorkDone only if the size of the cache is On 2016/11/18 01:26:41, twifkak wrote: > On 2016/11/17 21:52:04, twifkak wrote: > > Perhaps this check should be inside OnGetUnfinishedWorkDone only if the > prefetch > > is new, rather than before it. If there is unfinished work, then the client > had > > a big enough cache at one point, so it is safe to proceed without checking > > again. > > Actually, my next CL will want to pass the cache backend into PrecacheFetcher > (even for resumes), so it doesn't matter. Acknowledged. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:175: ->GetURLRequestContext() On 2016/11/17 21:52:04, twifkak wrote: > May return null? > https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c... Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:176: ->GetURLRequestContext() On 2016/11/17 21:52:04, twifkak wrote: > May return null: > https://cs.chromium.org/chromium/src/net/url_request/url_request_context_gett... Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:177: ->http_transaction_factory() On 2016/11/17 21:52:04, twifkak wrote: > May return null? > https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?rc... Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.cc:182: base::Unretained(this))); On 2016/11/17 23:15:19, twifkak wrote: > I think you should use AsWeakPtr() instead? Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.h:58: constexpr int kMinCacheSizeBytes = 40e6; On 2016/11/17 21:52:04, twifkak wrote: > FYI 40e6 is a float literal, which (by way of the constexpr) is getting cast at > compile time to an int. In this case, 40e6 is precise, so it works out. It feels > weird, but 53 bits ought to be enough for anybody, so I think it's fine. Acknowledged. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager.h:167: void OnCacheBackendReceived(int net_error_code); On 2016/11/17 21:52:04, twifkak wrote: > Add function doc. Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... File components/precache/content/precache_manager_unittest.cc (right): https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:278: ASSERT_EQ(cache_backend, cache->GetCurrentBackend()); On 2016/11/17 21:52:04, twifkak wrote: > (nit/opt) These asserts just verify that the test was written correctly (and > that its preconditions continue to be true). For that, I usually use CHECK > instead of ASSERT, just for documentation's sake. (Ditto below.) Done. https://codereview.chromium.org/2507753003/diff/1/components/precache/content... components/precache/content/precache_manager_unittest.cc:290: ->GetCurrentBackend() On 2016/11/17 21:52:04, twifkak wrote: > Use cache_backend from above? Done.
lgtm new histogram lg
https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:123: &PrecacheManager::OnCacheSizeReceived, base::AsWeakPtr(this))); s/base::AsWeakPtr(this)/AsWeakPtr()/. Provided by the SupportsWeakPtr superclass. Ditto below. https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:154: base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr())); AFAICT, this means OnGetUnfinishedWorkDone may be called in the IO thread. This is a problem, as it accesses some instance vars. https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:213: url_request_context_getter_ = Why is this an instance var? Pass via base::Bind(..., base::Unretained(...)). (BTW, when setting an instance var on one thread and reading it on another, you need a memory barrier at least; usually a mutex is the safest bet.)
https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.h:222: // They are not owned and are reset on demand via callbacks. Add a note that they should only be accessed on the IO thread.
The CQ bit was checked by jamartin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Very thorough review. Thank you :-) https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:123: &PrecacheManager::OnCacheSizeReceived, base::AsWeakPtr(this))); On 2016/11/18 20:27:37, twifkak wrote: > s/base::AsWeakPtr(this)/AsWeakPtr()/. Provided by the SupportsWeakPtr > superclass. Ditto below. Done. https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:154: base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr())); On 2016/11/18 20:27:37, twifkak wrote: > AFAICT, this means OnGetUnfinishedWorkDone may be called in the IO thread. This > is a problem, as it accesses some instance vars. Good catch. https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.cc:213: url_request_context_getter_ = On 2016/11/18 20:27:37, twifkak wrote: > Why is this an instance var? Pass via base::Bind(..., base::Unretained(...)). > > (BTW, when setting an instance var on one thread and reading it on another, you > need a memory barrier at least; usually a mutex is the safest bet.) Way better. Thanks. https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/20001/components/precache/con... components/precache/content/precache_manager.h:222: // They are not owned and are reset on demand via callbacks. On 2016/11/18 23:50:01, twifkak wrote: > Add a note that they should only be accessed on the IO thread. Done.
lgtm https://codereview.chromium.org/2507753003/diff/60001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/60001/components/precache/con... components/precache/content/precache_manager.cc:227: base::Unretained(url_request_context_getter))); Actually, on further thought, use scoped_refptr<> above instead of bare pointer, and std::move() here instead of base::Unretained().
The CQ bit was checked by jamartin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done. Waiting on gavinp@ for the DEP on net. https://codereview.chromium.org/2507753003/diff/60001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/60001/components/precache/con... components/precache/content/precache_manager.cc:227: base::Unretained(url_request_context_getter))); On 2016/11/19 02:15:06, twifkak wrote: > Actually, on further thought, use scoped_refptr<> above instead of bare pointer, > and std::move() here instead of base::Unretained(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
generally lgtm, but I'm curious about if it's possible not to have an http caching layer. https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:134: } else { // net_error_code != net::OK Readability nit: couldn't this be moved up to the top if (net_error_code != net::OK) { OnCacheSizeReceived(0); return; } // rest of function here. Avoids nesting things; I think conforms better to style. wdyt? https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:188: if (cache) { Is this test more of an assert? Could you ever really have an HttpNetworkLayer here? If not, suggest removing the test and error handling, and just crashing. But if it's possible that your context doesn't include an Http caching layer, the test must stay. https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:199: OnCacheSizeReceived(0); If keeping this test (see above), for readability I suggest moving this to just after initializing the cache, like this: if (!cache) { OnCacheSizeReceived(0); return; } https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.h:60: constexpr int kMinCacheSizeBytes = 40e6; minor nit: I find 40 * 1000 * 1000 an itsy bitsy bit more readable here. I just can't quite count the e6 the same way; but that is likely my own problem.
Thanks for the review. Submitting... https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... File components/precache/content/precache_manager.cc (right): https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:134: } else { // net_error_code != net::OK On 2016/12/08 17:33:46, gavinp wrote: > Readability nit: couldn't this be moved up to the top > > if (net_error_code != net::OK) { > OnCacheSizeReceived(0); > return; > } > > // rest of function here. > > Avoids nesting things; I think conforms better to style. wdyt? Done. I don't have a strong opinion. Devin (twifkak) kind of convinced me that early returns can make difficult control flows. Besides, one can argue that if you avoid early-returns, and you end up having several nested levels, chances are that you should refactorize the code into more functions. https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:188: if (cache) { On 2016/12/08 17:33:46, gavinp wrote: > Is this test more of an assert? Could you ever really have an HttpNetworkLayer > here? If not, suggest removing the test and error handling, and just crashing. > But if it's possible that your context doesn't include an Http caching layer, > the test must stay. I don't know. I don't know enough about Chromium network layers and their default settings. My gut feeling is that this is an unlikely scenario but it may happen in forks, experiments and rare devices with so little memory that may disable the cache. Anyway, I'd rather not to risk crashing the browser. I believe the safest course of action is to disable precaching in these situations. https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.cc:199: OnCacheSizeReceived(0); On 2016/12/08 17:33:46, gavinp wrote: > If keeping this test (see above), for readability I suggest moving this to just > after initializing the cache, like this: > > if (!cache) { > OnCacheSizeReceived(0); > return; > } Done. https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... File components/precache/content/precache_manager.h (right): https://codereview.chromium.org/2507753003/diff/80001/components/precache/con... components/precache/content/precache_manager.h:60: constexpr int kMinCacheSizeBytes = 40e6; On 2016/12/08 17:33:47, gavinp wrote: > minor nit: I find 40 * 1000 * 1000 an itsy bitsy bit more readable here. I just > can't quite count the e6 the same way; but that is likely my own problem. Done.
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, twifkak@chromium.org, gavinp@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps100001 (title: "Readability improvements")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/precache/content/precache_manager.cc:
While running git apply --index -p1;
error: patch failed: components/precache/content/precache_manager.cc:11
error: components/precache/content/precache_manager.cc: patch does not apply
Patch: components/precache/content/precache_manager.cc
Index: components/precache/content/precache_manager.cc
diff --git a/components/precache/content/precache_manager.cc
b/components/precache/content/precache_manager.cc
index
f4333c12d64e9b02779ca6a8de304a99f2e8bcf5..636de27a5912a15b0d098de02956855cf4338405
100644
--- a/components/precache/content/precache_manager.cc
+++ b/components/precache/content/precache_manager.cc
@@ -11,7 +11,9 @@
#include "base/bind.h"
#include "base/command_line.h"
#include "base/logging.h"
+#include "base/memory/ref_counted.h"
#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
#include "base/strings/string_util.h"
#include "base/time/time.h"
#include "components/history/core/browser/history_service.h"
@@ -26,6 +28,9 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
#include "net/base/network_change_notifier.h"
+#include "net/http/http_cache.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
using content::BrowserThread;
@@ -111,6 +116,93 @@ PrecacheManager::AllowedType
PrecacheManager::PrecachingAllowed() const {
return AllowedType::DISALLOWED;
}
+void PrecacheManager::OnCacheBackendReceived(int net_error_code) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ if (net_error_code != net::OK) {
+ // Assume there is no cache.
+ cache_backend_ = nullptr;
+ OnCacheSizeReceived(0);
+ return;
+ }
+ DCHECK(cache_backend_);
+ int result = cache_backend_->CalculateSizeOfAllEntries(
+ base::Bind(&PrecacheManager::OnCacheSizeReceived, AsWeakPtr()));
+ if (result == net::ERR_IO_PENDING) {
+ // Wait for the callback.
+ } else if (result >= 0) {
+ // The result is the expected bytes already.
+ OnCacheSizeReceived(result);
+ } else {
+ // Error occurred. Couldn't get the size. Assume there is no cache.
+ OnCacheSizeReceived(0);
+ }
+ cache_backend_ = nullptr;
+}
+
+void PrecacheManager::OnCacheSizeReceived(int cache_size_bytes) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+ BrowserThread::PostTask(
+ BrowserThread::UI, FROM_HERE,
+ base::Bind(&PrecacheManager::OnCacheSizeReceivedInUIThread, AsWeakPtr(),
+ cache_size_bytes));
+}
+
+void PrecacheManager::OnCacheSizeReceivedInUIThread(int cache_size_bytes) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+ UMA_HISTOGRAM_MEMORY_KB("Precache.CacheSize.AllEntries",
+ cache_size_bytes / 1024);
+ if (cache_size_bytes < min_cache_size_bytes_) {
+ OnDone(); // Do not continue.
+ } else {
+ BrowserThread::PostTaskAndReplyWithResult(
+ BrowserThread::DB, FROM_HERE,
+ base::Bind(&PrecacheDatabase::GetUnfinishedWork,
+ base::Unretained(precache_database_.get())),
+ base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr()));
+ }
+}
+
+void PrecacheManager::PrecacheIfCacheIsBigEnough(
+ scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ CHECK(url_request_context_getter);
+
+ // Continue with OnGetUnfinishedWorkDone only if the size of the cache is
+ // at least min_cache_size_bytes_.
+ // Class disk_cache::Backend does not expose its maximum size. However,
caches
+ // are usually full, so we can use the size of all the entries stored in the
+ // cache (via CalculateSizeOfAllEntries) as a proxy of its maximum size.
+ net::URLRequestContext* context =
+ url_request_context_getter->GetURLRequestContext();
+ if (!context) {
+ OnCacheSizeReceived(0);
+ return;
+ }
+ net::HttpTransactionFactory* factory = context->http_transaction_factory();
+ if (!factory) {
+ OnCacheSizeReceived(0);
+ return;
+ }
+ net::HttpCache* cache = factory->GetCache();
+ if (!cache) {
+ // There is no known cache. Assume that there is no cache.
+ // TODO(jamartin): I'm not sure this can be an actual posibility. Consider
+ // making this a CHECK(cache).
+ OnCacheSizeReceived(0);
+ return;
+ }
+ const int net_error_code = cache->GetBackend(
+ &cache_backend_,
+ base::Bind(&PrecacheManager::OnCacheBackendReceived, AsWeakPtr()));
+ if (net_error_code != net::ERR_IO_PENDING) {
+ // No need to wait for the callback. The callback hasn't been called with
+ // the appropriate code, so we call it directly.
+ OnCacheBackendReceived(net_error_code);
+ }
+}
+
void PrecacheManager::StartPrecaching(
const PrecacheCompletionCallback& precache_completion_callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -128,12 +220,18 @@ void PrecacheManager::StartPrecaching(
base::Bind(&PrecacheDatabase::SetLastPrecacheTimestamp,
base::Unretained(precache_database_.get()),
base::Time::Now()));
- BrowserThread::PostTaskAndReplyWithResult(
- BrowserThread::DB,
- FROM_HERE,
- base::Bind(&PrecacheDatabase::GetUnfinishedWork,
- base::Unretained(precache_database_.get())),
- base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr()));
+
+ scoped_refptr<net::URLRequestContextGetter> url_request_context_getter(
+ content::BrowserContext::GetDefaultStoragePartition(browser_context_)
+ ->GetURLRequestContext());
+ if (url_request_context_getter) {
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&PrecacheManager::PrecacheIfCacheIsBigEnough, AsWeakPtr(),
+ std::move(url_request_context_getter)));
+ } else { // !url_request_context_getter_.
+ OnCacheSizeReceivedInUIThread(0);
+ }
}
void PrecacheManager::OnGetUnfinishedWorkDone(
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, rkaplow@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps120001 (title: "Sync'ed to HEAD")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by twifkak@chromium.org
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, rkaplow@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps140001 (title: "Cannot use WeakPtr across threads")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Explanation for patch set 8: 1. We discovered that this DCHECK-fails because WeakPtr can't be used on multiple threads. PrecacheManager is a BCKS so it will not be destroyed except on shutdown, and it is Android-only, so there is no shutdown [1], so I think we're safe using base::Unretained. 2. Added a variation param as an escape hatch; the default behavior is no minimum. [1] https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/07RhjCgpL5U...
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, rkaplow@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps160001 (title: "Forgot dependency on variations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, rkaplow@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps180001 (title: "Wrong dependency name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by twifkak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by twifkak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/precache/content/precache_manager.cc:
While running git apply --index -p1;
error: patch failed: components/precache/content/precache_manager.cc:11
error: components/precache/content/precache_manager.cc: patch does not apply
Patch: components/precache/content/precache_manager.cc
Index: components/precache/content/precache_manager.cc
diff --git a/components/precache/content/precache_manager.cc
b/components/precache/content/precache_manager.cc
index
327fa2b6b83ceafdd748f932467358cb30803ee7..d1135ad3f4c006ab33a856e08817a629b92a47b7
100644
--- a/components/precache/content/precache_manager.cc
+++ b/components/precache/content/precache_manager.cc
@@ -11,7 +11,10 @@
#include "base/bind.h"
#include "base/command_line.h"
#include "base/logging.h"
+#include "base/memory/ref_counted.h"
#include "base/metrics/field_trial.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/time/time.h"
#include "components/history/core/browser/history_service.h"
@@ -26,12 +29,19 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
#include "net/base/network_change_notifier.h"
+#include "net/http/http_cache.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
using content::BrowserThread;
-namespace {
+namespace precache {
const char kPrecacheFieldTrialName[] = "Precache";
+const char kMinCacheSizeParam[] = "min_cache_size";
+
+namespace {
+
const char kPrecacheFieldTrialEnabledGroup[] = "Enabled";
const char kPrecacheFieldTrialControlGroup[] = "Control";
const char kConfigURLParam[] = "config_url";
@@ -40,8 +50,6 @@ const size_t kNumTopHosts = 100;
} // namespace
-namespace precache {
-
size_t NumTopHosts() {
return kNumTopHosts;
}
@@ -111,6 +119,94 @@ PrecacheManager::AllowedType
PrecacheManager::PrecachingAllowed() const {
return AllowedType::DISALLOWED;
}
+void PrecacheManager::OnCacheBackendReceived(int net_error_code) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ if (net_error_code != net::OK) {
+ // Assume there is no cache.
+ cache_backend_ = nullptr;
+ OnCacheSizeReceived(0);
+ return;
+ }
+ DCHECK(cache_backend_);
+ int result = cache_backend_->CalculateSizeOfAllEntries(base::Bind(
+ &PrecacheManager::OnCacheSizeReceived, base::Unretained(this)));
+ if (result == net::ERR_IO_PENDING) {
+ // Wait for the callback.
+ } else if (result >= 0) {
+ // The result is the expected bytes already.
+ OnCacheSizeReceived(result);
+ } else {
+ // Error occurred. Couldn't get the size. Assume there is no cache.
+ OnCacheSizeReceived(0);
+ }
+ cache_backend_ = nullptr;
+}
+
+void PrecacheManager::OnCacheSizeReceived(int cache_size_bytes) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+ BrowserThread::PostTask(
+ BrowserThread::UI, FROM_HERE,
+ base::Bind(&PrecacheManager::OnCacheSizeReceivedInUIThread,
+ base::Unretained(this), cache_size_bytes));
+}
+
+void PrecacheManager::OnCacheSizeReceivedInUIThread(int cache_size_bytes) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+ UMA_HISTOGRAM_MEMORY_KB("Precache.CacheSize.AllEntries",
+ cache_size_bytes / 1024);
+
+ if (cache_size_bytes < min_cache_size_bytes_) {
+ OnDone(); // Do not continue.
+ } else {
+ BrowserThread::PostTaskAndReplyWithResult(
+ BrowserThread::DB, FROM_HERE,
+ base::Bind(&PrecacheDatabase::GetUnfinishedWork,
+ base::Unretained(precache_database_.get())),
+ base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr()));
+ }
+}
+
+void PrecacheManager::PrecacheIfCacheIsBigEnough(
+ scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ CHECK(url_request_context_getter);
+
+ // Continue with OnGetUnfinishedWorkDone only if the size of the cache is
+ // at least min_cache_size_bytes_.
+ // Class disk_cache::Backend does not expose its maximum size. However,
caches
+ // are usually full, so we can use the size of all the entries stored in the
+ // cache (via CalculateSizeOfAllEntries) as a proxy of its maximum size.
+ net::URLRequestContext* context =
+ url_request_context_getter->GetURLRequestContext();
+ if (!context) {
+ OnCacheSizeReceived(0);
+ return;
+ }
+ net::HttpTransactionFactory* factory = context->http_transaction_factory();
+ if (!factory) {
+ OnCacheSizeReceived(0);
+ return;
+ }
+ net::HttpCache* cache = factory->GetCache();
+ if (!cache) {
+ // There is no known cache. Assume that there is no cache.
+ // TODO(jamartin): I'm not sure this can be an actual posibility. Consider
+ // making this a CHECK(cache).
+ OnCacheSizeReceived(0);
+ return;
+ }
+ const int net_error_code = cache->GetBackend(
+ &cache_backend_, base::Bind(&PrecacheManager::OnCacheBackendReceived,
+ base::Unretained(this)));
+ if (net_error_code != net::ERR_IO_PENDING) {
+ // No need to wait for the callback. The callback hasn't been called with
+ // the appropriate code, so we call it directly.
+ OnCacheBackendReceived(net_error_code);
+ }
+}
+
void PrecacheManager::StartPrecaching(
const PrecacheCompletionCallback& precache_completion_callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -128,12 +224,30 @@ void PrecacheManager::StartPrecaching(
base::Bind(&PrecacheDatabase::SetLastPrecacheTimestamp,
base::Unretained(precache_database_.get()),
base::Time::Now()));
- BrowserThread::PostTaskAndReplyWithResult(
- BrowserThread::DB,
- FROM_HERE,
- base::Bind(&PrecacheDatabase::GetUnfinishedWork,
- base::Unretained(precache_database_.get())),
- base::Bind(&PrecacheManager::OnGetUnfinishedWorkDone, AsWeakPtr()));
+
+ // Ignore boolean return value. In all documented failure cases, it sets the
+ // int to a reasonable value.
+ base::StringToInt(variations::GetVariationParamValue(kPrecacheFieldTrialName,
+ kMinCacheSizeParam),
+ &min_cache_size_bytes_);
+ if (min_cache_size_bytes_ <= 0) {
+ // Skip looking up the cache size, because it doesn't matter.
+ OnCacheSizeReceivedInUIThread(0);
+ return;
+ }
+
+ scoped_refptr<net::URLRequestContextGetter> url_request_context_getter(
+ content::BrowserContext::GetDefaultStoragePartition(browser_context_)
+ ->GetURLRequestContext());
+ if (!url_request_context_getter) {
+ OnCacheSizeReceivedInUIThread(0);
+ return;
+ }
+
+ BrowserThread::PostTask(
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&PrecacheManager::PrecacheIfCacheIsBigEnough, AsWeakPtr(),
+ std::move(url_request_context_getter)));
}
void PrecacheManager::OnGetUnfinishedWorkDone(
The CQ bit was checked by jamartin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org, rkaplow@chromium.org, twifkak@chromium.org Link to the patchset: https://codereview.chromium.org/2507753003/#ps200001 (title: "Sync'ed to HEAD")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by twifkak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481589047811050,
"parent_rev": "0aee784c24c825e4e5508c21cfc49f56d6199f38", "commit_rev":
"ea20ecb56f7895e32f5cfdb77a7bedde5933a746"}
Message was sent while issue was closed.
Description was changed from ========== Do not precache when the cache size is small BUG=649495 ========== to ========== Do not precache when the cache size is small BUG=649495 Review-Url: https://codereview.chromium.org/2507753003 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Do not precache when the cache size is small BUG=649495 Review-Url: https://codereview.chromium.org/2507753003 ========== to ========== Do not precache when the cache size is small BUG=649495 Committed: https://crrev.com/305b67f08c9aca25fc0abdaf4399deb3dd89e2b8 Cr-Commit-Position: refs/heads/master@{#437985} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/305b67f08c9aca25fc0abdaf4399deb3dd89e2b8 Cr-Commit-Position: refs/heads/master@{#437985} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
