Bug 200159 - Make NetworkCache cache shrink logic take account of the configured capacity
Summary: Make NetworkCache cache shrink logic take account of the configured capacity
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-26 02:16 PDT by Loïc Yhuel
Modified: 2019-07-26 06:46 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2019-07-26 02:19 PDT, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2019-07-26 03:39 PDT, Loïc Yhuel
loic.yhuel: review?
loic.yhuel: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.