913820 - HTTP cache v2: reintroduce all HTTP cache preferences

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- this is not 100% of what the current cache is doing
- e.g. dooming an entry when data are over per-entry limit is not implemented ; I don't think we necessarily need it for the first deployment and we should rather first discuss some smarter strategy for caching or not-caching larger files (mainly media)
- when disk cache is disabled, we store to memory cache (when enabled): nsICacheStorageService.diskCacheStorage returns a storage configured to store to memory ; when disk cache is enabled later, the entries are found in a disk-enabled storage as well, but not persisted (I personally think it's OK, I don't expect users will switch this often at runtime, and still.. this is just a cache)

Comment 2

[Honza Bambas (:mayhemer)]

Blocks 920573 since this patch introduces a code to easily disable the whole disk cache.  The (intermediate) plan for bug 920573 is to use only memory cache, hence there is no need to do any stupid cleanups at shutdown.

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 803114 [details] [diff] [review]
v1

Review of attachment 803114 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheEntry.cpp
@@ +868,5 @@
>    mPredictedDataSize = aPredictedDataSize;
> +
> +  if (CacheObserver::EntryIsTooBig(mPredictedDataSize, mUseDisk)) {
> +    LOG(("CacheEntry::SetPredictedDataSize [this=%p] too big, dooming", this));
> +    AsyncDoom(nullptr);

This seems to be a different behavior when compared with the old cache. In the old cache, calling SetPredictedDataSize() with too big size ensured that we didn't write any data to disk. The new cache just dooms the entry but this won't prevent generating unnecessary IO.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #3)
> Comment on attachment 803114 [details] [diff] [review]
> v1
> 
> Review of attachment 803114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheEntry.cpp
> @@ +868,5 @@
> >    mPredictedDataSize = aPredictedDataSize;
> > +
> > +  if (CacheObserver::EntryIsTooBig(mPredictedDataSize, mUseDisk)) {
> > +    LOG(("CacheEntry::SetPredictedDataSize [this=%p] too big, dooming", this));
> > +    AsyncDoom(nullptr);
> 
> This seems to be a different behavior when compared with the old cache. In
> the old cache, calling SetPredictedDataSize() with too big size ensured that
> we didn't write any data to disk. The new cache just dooms the entry but
> this won't prevent generating unnecessary IO.

Can you please suggest how to do this then?  Should we just throw the entry away from the channel when it's too large + doom it what would effectively remove the data?

As I can see in the old cache, we doom + don't ensure a device (@nsCacheService::EnsureEntryHasDevice).  So, yes, that is different.  Also there is monitoring of size changes when it's not known via Content-length before data fetch start.  I don't understand the code (overcomplicated) and I don't know what happens in that case.  I assume the we doom the entry?  I think the preferences patch is not doing it and I'd like to do it in a followup bug.

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 803114 [details] [diff] [review]
v1

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Can you please suggest how to do this then?  Should we just throw the entry
> away from the channel when it's too large + doom it what would effectively
> remove the data?

Yes, this should work. r+ with this change.

> As I can see in the old cache, we doom + don't ensure a device
> (@nsCacheService::EnsureEntryHasDevice).  So, yes, that is different.  Also
> there is monitoring of size changes when it's not known via Content-length
> before data fetch start.  I don't understand the code (overcomplicated) and
> I don't know what happens in that case.  I assume the we doom the entry?  I
> think the preferences patch is not doing it and I'd like to do it in a
> followup bug.

Yes, we doom the entry once its size exceeds this limit. I'll implement this feature in CacheFile or in CacheFileIOManager.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 803114 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0db5f09d041

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [as landed]

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0db5f09d041