Bug 200159

Summary: Make NetworkCache cache shrink logic take account of the configured capacity
Product: WebKit Reporter: Loïc Yhuel <loic.yhuel>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: achristensen, beidson, cdumez, cgarcia, darin, ews-watchlist, koivisto, olivier.blin, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch loic.yhuel: review?, loic.yhuel: commit-queue?

Description Loïc Yhuel 2019-07-26 02:16:02 PDT
Make NetworkCache cache shrink logic take account of the configured capacity
Comment 1 Loïc Yhuel 2019-07-26 02:19:52 PDT
Created attachment 374958 [details]
Patch
Comment 2 Loïc Yhuel 2019-07-26 02:22:48 PDT
I'm open to comments about the logic, since :
 - I had to remove the "effectiveWorth" adjustment : keeping it would prevent an unspecified amount of records to be deleted, and I don't know how we could reimplement something similar.
 - The computed deletion probability can be greater than 1 for a part of the entries (if the cache is much bigger than the capacity, either due to issues or if we are reducing it, or if the average worth is high), which means the deletionProbabilityFactor could in fact be not big enough to delete entries with high worth.
 - If the average worth is high, the shrink can also be too aggressive.
Comment 3 Loïc Yhuel 2019-07-26 03:39:01 PDT
Created attachment 374959 [details]
Patch
Comment 4 Chris Dumez 2019-07-26 06:21:27 PDT
Comment on attachment 374959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374959&action=review

> Source/WebKit/ChangeLog:9
> +        more than 33%, we should target 67% of the capacity instead.

Why? 

This change log does not explain why this change is needed.
Comment 5 Loïc Yhuel 2019-07-26 06:46:21 PDT
(In reply to Chris Dumez from comment #4)
> > Source/WebKit/ChangeLog:9
> > +        more than 33%, we should target 67% of the capacity instead.
> 
> Why? 
> 
> This change log does not explain why this change is needed.
It's "when reducing the cache capacity". The target shrink size must depend on the desired capacity, not the current size.

For example if you have a 200MB cache (capacity >= 200MB), and you call WebKit::NetworkCache::Cache::setCapacity(100MB), the original code has a maximum deletion probability of 33% so to cache would be reduced to 134MB at best, so you would need at laest a second shrink to be under 100MB.