1325341 - Implement RCWN decision algorithm

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Use all available data to make a decision how to fetch the resource. Here are few examples of what we need to take into account:

prefer network if:
  - cache is slow due to excessive IO
  - cache is slower than network even if the disk is not stressed and the resource is small enough

prefer disk if:
  - mobile data connection is used
  - the resource is big
  - the cache entry contains alternative data representation
  - fetching the resource from network was previously slow

The result should be one of these:
1) start network and cache request at the same time
2) go first to disk but start network request if we don't get the entry after X ms.
3) go to disk and wait for the entry

Comment 1

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce63b594bc17

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1325341 - Add code that triggers racing the network and cache requests

The racing algorithm is quite simple at this point:
If racing is enabled, the request is allowed to hit the network, and the cache queue size is bigger than a certain threshold, then we trigger the network right before we query the cache.

Review commit: https://reviewboard.mozilla.org/r/119696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119696/

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1325341 - Racing cache with network should be disabled by default

Review commit: https://reviewboard.mozilla.org/r/119698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119698/

Comment 4

[Valentin Gosu [:valentin] (he/him)]

These initial patches implement a decision algorithm that is quite simple, that we may expand upon later.

> prefer network if:
>   - cache is slow due to excessive IO

This is the primary rule. If the cache queues are longer that a preset limit, we trigger a race before trying to find the cache entry. From what I can tell, cache queue size is the main indicator of who would win.

>   - cache is slower than network even if the disk is not stressed and the
> resource is small enough

This would be a little more tricky. If we trigger the racing before cache->AsyncOpenURI, we don't know the size of the resource yet.

> prefer disk if:
>   - mobile data connection is used

For now I'm just preffing off the racing on mobile. I'm considering this as a followup.

>   - the resource is big
>   - the cache entry contains alternative data representation

Do we have a way to determine if the resource is big, or has alt data before hitting the cache?

>   - fetching the resource from network was previously slow

Similar to the previous question. Do we have this in the cache index?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f483583374f0

Comment 6

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #4)
> This would be a little more tricky. If we trigger the racing before
> cache->AsyncOpenURI, we don't know the size of the resource yet.

We can check the index, it might not be ready yet, but if it is, the size is known.

> >   - the resource is big
> >   - the cache entry contains alternative data representation
> 
> Do we have a way to determine if the resource is big, or has alt data before
> hitting the cache?

Index is in memory, so this information can be fetched synchronously.

> >   - fetching the resource from network was previously slow
> 
> Similar to the previous question. Do we have this in the cache index?

Not yet, bug 1325088.

Comment 7

[Junior [inactive]]

> > >   - fetching the resource from network was previously slow
> > 
> > Similar to the previous question. Do we have this in the cache index?
> 
> Not yet, bug 1325088.

I'm working on Bug 1325091 and will catch this bug soon.

Comment 8

[Junior [inactive]]

> > prefer network if:
> >   - cache is slow due to excessive IO
> 
> This is the primary rule. If the cache queues are longer that a preset
> limit, we trigger a race before trying to find the cache entry. From what I
> can tell, cache queue size is the main indicator of who would win.
> 

The threshold also depends on the priority.
For those normal priority resource, network has a big win with more chance.

Comment 9

[Junior [inactive]]

> >   - cache is slow due to excessive IO
> The threshold also depends on the priority.
> For those normal priority resource, network has a big win with more chance.

Oops, I see it's in the patch :)

> >   - fetching the resource from network was previously slow
Do we need to collect more telemetry for a good threadhold?
A random thought is to collect the distribution for a 100ms net-win.

Comment 10

[Michal Novotny [:michal]]

Comment on attachment 8846686 [details]
Bug 1325341 - Add code that triggers racing the network and cache requests

Looks good, but I think even this first preliminary version should work with all data we have, i.e. size and alt-data presence.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize

Review commit: https://reviewboard.mozilla.org/r/124710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124710/

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8846686 [details]
Bug 1325341 - Add code that triggers racing the network and cache requests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119696/diff/1-2/

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8846687 [details]
Bug 1325341 - Racing cache with network should be disabled by default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119698/diff/1-2/

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Junior, I ran the alt-data unit tests with the latest patch, and hasAltData was always false. I'm not sure the CacheIndex is picking up the flag properly.

Comment 15

[Junior [inactive]]

bug 1325088 fixed some cache index setting.
I bet it will work after patching it.

Comment 16

[Junior [inactive]]

(In reply to Junior[:junior] from comment #15)
> bug 1325088 fixed some cache index setting.
> I bet it will work after patching it.

Confirmed.

Comment 17

[Michal Novotny [:michal]]

Comment on attachment 8852520 [details]
Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize

https://reviewboard.mozilla.org/r/124710/#review127652

::: netwerk/cache2/CacheStorageService.cpp:1651
(Diff revision 1)
> +
> +  nsAutoCString contextKey;
> +  CacheFileUtils::AppendKeyPrefix(aStorage->LoadInfo(), contextKey);
> +
> +  if (!aStorage->WriteToDisk()) {
> +    AppendMemoryStorageID(contextKey);

Memory only entries are not in the index. It also doesn't make sense to race memory cache with the network.

::: netwerk/protocol/http/nsHttpChannel.cpp:3617
(Diff revision 1)
>          mCacheQueueSizeWhenOpen = CacheStorageService::CacheQueueSize(mCacheOpenWithPriority);
>  
> -        if (sRCWNEnabled && mInterceptCache != INTERCEPTED) {
> +        bool hasAltData = false;
> +        uint32_t sizeInKb = 0;
> +        rv = cacheStorage->GetCacheIndexEntryAttrs(openURI, extension, &hasAltData, &sizeInKb);
> +        LOG(("Found a cache index entry with: hasAltData:%d sizeInKb: %d\n",

Looks like you forgot to enclose this into if (NS_SUCEEDED(rv)) {}

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8846686 [details]
Bug 1325341 - Add code that triggers racing the network and cache requests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119696/diff/2-3/

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8846687 [details]
Bug 1325341 - Racing cache with network should be disabled by default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119698/diff/2-3/

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8852520 [details]
Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124710/diff/1-2/

Comment 21

[Michal Novotny [:michal]]

Comment on attachment 8852520 [details]
Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize

https://reviewboard.mozilla.org/r/124710/#review130300

::: netwerk/protocol/http/nsHttpChannel.cpp:3624
(Diff revision 2)
> +
> +        // We will attempt to race the network vs the cache if we've found this
> +        // entry in the cache index, and it has appropriate
> +        // attributes (doesn't have alt-data, and has a small size)
> +        if (sRCWNEnabled && mInterceptCache != INTERCEPTED &&
> +            NS_SUCCEEDED(rv) && !hasAltData && sizeInKb < sRCWNSmallResourceSizeKB) {

It might make sense to use hasAltData flag only in case mPreferredCachedAltDataType isn't empty, right?

Comment 22

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c773bfb7638d0eb21a3982136fed29c328210a
Bug 1325341 - Add code that triggers racing the network and cache requests r=michal

https://hg.mozilla.org/integration/mozilla-inbound/rev/4547e7e50aeba5f3b001da14c71b8e72f8fa45b1
Bug 1325341 - Racing cache with network should be disabled by default r=michal

https://hg.mozilla.org/integration/mozilla-inbound/rev/56af34f86b180aac4b01775fad19ce49ee3ed71f
Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize r=michal

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22c773bfb763
https://hg.mozilla.org/mozilla-central/rev/4547e7e50aeb
https://hg.mozilla.org/mozilla-central/rev/56af34f86b18