New issue
Advanced search Search tips

Issue 729679 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Store zero-freshness + response non-conditionality in in-memory index

Project Member Reported by morlovich@chromium.org, Jun 5 2017

Issue description

This is a variant of the ideas in issue 292923:

It turns out that the vast majority of the time when we can't conditionalize fetches, it's for resources that have zero freshness.
The fact of zero-freshness is storable in only one bit; an another can store some of the common causes of non-conditionability, so it may make sense to store this information in the in-memory index, to make it really cheap to fail those cases, w/o reading in the entry.

Sketching this out in https://codereview.chromium.org/2922973003/




 
Correction:  issue 292914  is the right reference.

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 30 2017

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

commit e63bba9024d3397e80e393aed842ba9957385796
Author: morlovich <morlovich@chromium.org>
Date: Wed Aug 30 16:29:02 2017

Interface for associating in-memory hint bytes with disk cache entries.
Implementation for it in the mock cache (with off switch)

The intent is to implement it in simple cache, and to use it in
HttpCache::Transaction to avoid opening entries that would not be usable
under present load flags due to caching-hostile headers.

BUG= 729679 

Review-Url: https://codereview.chromium.org/2958033002
Cr-Commit-Position: refs/heads/master@{#498491}

[modify] https://crrev.com/e63bba9024d3397e80e393aed842ba9957385796/net/disk_cache/disk_cache.cc
[modify] https://crrev.com/e63bba9024d3397e80e393aed842ba9957385796/net/disk_cache/disk_cache.h
[modify] https://crrev.com/e63bba9024d3397e80e393aed842ba9957385796/net/http/mock_http_cache.cc
[modify] https://crrev.com/e63bba9024d3397e80e393aed842ba9957385796/net/http/mock_http_cache.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 30 2017

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

commit 0f701ce1c09dd85eb1c175aa953f8078b0ce8349
Author: morlovich <morlovich@chromium.org>
Date: Wed Aug 30 19:21:25 2017

Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

BUG= 729679 

Review-Url: https://codereview.chromium.org/2957133002
Cr-Commit-Position: refs/heads/master@{#498566}

[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/backend_unittest.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_backend_impl.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_backend_impl.h
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_backend_version.h
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index.h
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index_file.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index_file.h
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index_file_unittest.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_index_unittest.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_synchronous_entry.cc
[modify] https://crrev.com/0f701ce1c09dd85eb1c175aa953f8078b0ce8349/net/disk_cache/simple/simple_version_upgrade.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 27 2017

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

commit b6d2d88636508af23b09cec87bbbd57af4778407
Author: Maks Orlovich <morlovich@chromium.org>
Date: Fri Oct 27 12:09:21 2017

SimpleCache: Permit optimistic create while doom is pending. 

This is desirable if we use in memory hints to avoid opening files in CantConditionalize
case --- in that case we still want a cache entry, so the simplest way of doing that is
to Doom the old one and Create a new one, which before this change would block until
Doom completes. This lets CreateEntry return an Entry object optimistically, as long as 
the create is the first operation after Doom. 

This works by creating the entry in STATE_IO_PENDING state, and transitioning away from
that and actually starting creation I/O via an entries_pending_doom_ callback.

(This may be also be me trying to be too clever, I am worried this may be a bit too cute)

Bug:  729679 
Change-Id: I210f7ae5686a1a04e0930ea5c13e67c81131bd1a
Reviewed-on: https://chromium-review.googlesource.com/684855
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512151}
[modify] https://crrev.com/b6d2d88636508af23b09cec87bbbd57af4778407/net/disk_cache/disk_cache_test_base.cc
[modify] https://crrev.com/b6d2d88636508af23b09cec87bbbd57af4778407/net/disk_cache/entry_unittest.cc
[modify] https://crrev.com/b6d2d88636508af23b09cec87bbbd57af4778407/net/disk_cache/simple/simple_backend_impl.cc
[modify] https://crrev.com/b6d2d88636508af23b09cec87bbbd57af4778407/net/disk_cache/simple/simple_entry_impl.cc
[modify] https://crrev.com/b6d2d88636508af23b09cec87bbbd57af4778407/net/disk_cache/simple/simple_entry_impl.h

I wonder if the HttpCache should just doom unconditionalizable zero-freshness resources and hang onto the entry for a few minutes to allow for short-term back-forward navigation.

That way they're not taking up space in the cache longer than necessary.
Hmm, would have to be potentially things other than HTML / more than a singular resource, even if those are wayyy less likely to be CantConditionalize, but it's plausible that'd indeed be a better approach, and there is some level of tracking of existing entries in HCT level, after all.

How hard would invalidation be, though? 

(The approach I've had in mind is actually quite straightforward...)


Sorry, didn't see your response until now. What I suggested in #5 wouldn't quite work as I'd suggested. Once doomed, the entry can't be used again for back-forward navigation. So we'd have to doom after the timeout. 

What I'm looking for is to improve cache eviction as well as fast-fail. So, as a follow-up CL to https://chromium-review.googlesource.com/c/chromium/src/+/732841 we could change the eviction algorithm to first evict entries older than X minutes that have the HINT_UNUSABLE_PER_CACHING_HEADERS bit set.

I think it'd be simpler (than marking the entries and changing the eviction algorithm) however to keep a set of entries in HttpCache::Transaction that are to be doomed either on ~HttpCache::Transaction or after X time has elapsed.
The timeout thing is a nice idea, and one I am probably insufficiently friendly towards due to being nearly-done with the different approach, but I do think it does have downsides that are not just my mind's inertia --- in particular:
1) Would you cancel the timer if circumstances change? On new use? That would require some non-trivial bookkeeping.
2) It seems a little hard to reason about/keep stats on, due to how hands off it is from the use time.

As for eviction idea... well, layering-wise a delegate would probably be desirable, though an API for one seems tricky... though we could experiment with a quick hack easily enough.

Okay, after F2F discussion we agreed to stick with the bits in https://chromium-review.googlesource.com/c/chromium/src/+/732841 and punt evicting such resources to another day.

Some options we have for eviction:

1) Consumer uses a delayed task to doom the entry
2) Backend does the delayed doom (e.g., cache_entry_->DoomLater(base::TimeDelta)
3) Backend takes a hint from the client that the resource is low priority and should be evicted first at next eviction.
Side note: we don't seem to have any metrics for memory caches? Is that intentional due to their use in incognito? 

I don't actually know if we're trying to avoid collection in incognito or if it was too painful to double the UMA metrics.
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 17 2017

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

commit d036d702fce56b666ba6f1904721756f19c68b15
Author: Maks Orlovich <morlovich@chromium.org>
Date: Fri Nov 17 15:51:18 2017

Use in-memory hints to avoid opening already expired non-conditionalizable entries.

HttpCache.CantConditionalizeZeroFreshnessFromMemHint is logged along
HttpCache.CantConditianlizeCause == ZERO_FRESHNESS to denote this working.

Since this is implemented as dooming entries, it messes somewhat with the "open stale"
feature on error page. It's my understanding that feature is not turned on, so this just relaxes
tests for it to use things not eligible for the behavior change.

Bug:  729679 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie9a489fb6fa53a76691615c789c7373650180de0
Reviewed-on: https://chromium-review.googlesource.com/732841
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517405}
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/chrome/browser/net/errorpage_browsertest.cc
[add] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/chrome/test/data/nocache-with-etag.html
[add] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/chrome/test/data/nocache-with-etag.html.mock-http-headers
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/content/renderer/browser_render_view_browsertest.cc
[add] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/content/test/data/nocache-with-etag.html
[add] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/content/test/data/nocache-with-etag.html.mock-http-headers
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/base/net_error_list.h
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/http/http_cache.cc
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/http/http_cache.h
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/http/http_cache_transaction.cc
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/http/http_cache_transaction.h
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/net/http/http_cache_unittest.cc
[modify] https://crrev.com/d036d702fce56b666ba6f1904721756f19c68b15/tools/metrics/histograms/histograms.xml

 Issue 292914  has been merged into this issue.
Status: Fixed (was: Started)

Sign in to add a comment