Closed Bug 1325341 Opened 8 years ago Closed 7 years ago

Implement RCWN decision algorithm

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: michal, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-next])

Attachments

(3 files)

Bug 1325341 - Add code that triggers racing the network and cache requests
59 bytes, text/x-review-board-request
michal
: review+
Details
Bug 1325341 - Racing cache with network should be disabled by default
59 bytes, text/x-review-board-request
michal
: review+
Details
Bug 1325341 - Query the cache index to get if the entry has alt data and its fileSize
59 bytes, text/x-review-board-request
michal
: review+
Details 
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
Whiteboard: [necko-next]
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?
Assignee: nobody → valentin.gosu
(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.
> > >   - 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.
> > 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.
> >   - 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 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.
Attachment #8846686 - Flags: review?(michal.novotny) → feedback+
Attachment #8846687 - Flags: review?(michal.novotny) → review+
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.
Flags: needinfo?(juhsu)
bug 1325088 fixed some cache index setting.
I bet it will work after patching it.
Flags: needinfo?(juhsu)
(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 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)) {}
Attachment #8852520 - Flags: review?(michal.novotny) → feedback+
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?
Attachment #8852520 - Flags: review?(michal.novotny) → review+
Attachment #8846687 - Flags: review?(michal.novotny) → review+
Attachment #8846686 - Flags: review?(michal.novotny) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: