Closed Bug 913820 Opened 7 years ago Closed 6 years ago

HTTP cache v2: reintroduce all HTTP cache preferences

Categories

(Core :: Networking: Cache, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [cache2])

Attachments

(2 files)

No description provided.
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
- 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)
Attachment #803114 - Flags: review?(michal.novotny)
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.
Blocks: 920573
No longer blocks: cache2enable
Summary: HTTP cache v2: reintroduce all cache size limits as preferences → HTTP cache v2: reintroduce all HTTP cache preferences
Blocks: 913808
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.
Attachment #803114 - Flags: review?(michal.novotny)
(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 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.
Attachment #803114 - Flags: review+
Attached patch v1.1 [as landed]Splinter Review
https://hg.mozilla.org/mozilla-central/rev/e0db5f09d041
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 968860
Depends on: 969160
Blocks: 970011
You need to log in before you can comment on or make changes to this bug.