913813 - HTTP cache v2: cache miss telemetry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- adds HIT and MISS timing telemetry to both the new and old code
- in the new code it wraps the time needed to load a cache file
- in the old code it wraps opening a cache entry (so it may also include measurements for already opened entries ; your judgment will say if this is acceptable or not, I personally think it is also according the first tests and fact we have the distribution graphs)
- when the cache file is new or the access == WRITE we accumulate the time as MISS
- when the cache file is found (not new) or access != WRITE (i.e. is R/W or R/O) we accumulate as HIT
- all measurements include main thread -> I/O thread -> main thread hops, so it includes all wait times that users perceive and thus giving a complete picture

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 802960 [details] [diff] [review]
v1

Review of attachment 802960 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/OldWrappers.cpp
@@ +560,5 @@
>        LOG(("  duplicate call, bypassed"));
>        return NS_OK;
>      }
>  
> +    if (!mLoadStart.IsNull() && NS_SUCCEEDED(mStatus)) {

mLoadStart can't be null here, right?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #2)
> > +    if (!mLoadStart.IsNull() && NS_SUCCEEDED(mStatus)) {
> 
> mLoadStart can't be null here, right?

True.

Comment 4

[Honza Bambas (:mayhemer)]

Michal, should we bypass accumulation for OPEN_TRUNCATE?  I think we should...

Comment 5

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Michal, should we bypass accumulation for OPEN_TRUNCATE?  I think we
> should...

In the new cache code, there is quite a different code path for OPEN_TRUNCATE and we return the entry immediately. So it makes sense to not include these numbers to NETWORK_CACHE_V2_HIT_TIME_MS. In the old cache, there isn't much difference between ACCESS_READ_WRITE and ACCESS_WRITE. Maybe we should create a new telemetry probe for OPEN_TRUNCATE for both, the old as well as the new cache? This would give as some numbers showing how much better the new cache behaves in this case.

Comment 6

[Honza Bambas (:mayhemer)]

Yes, I'm having a similar idea.  

FYI: I'm rewriting wysiwyg channel to use the new cache API.  Found to be not that trivial.  I had to introduce a synchronous way to open cache entries when access == WRITE and invoked on the cache thread.  Clearly, old synchronous OpenCacheEntry with WRITE only access gives an entry immediately too as the new cache does, but only when on the cache thread.

However, there is a major prevalence of cases where AsyncOpenURI is called on main thread or non-cache thread where we have to do the whole dance.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [as landed]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1bd7d5dae4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee1bd7d5dae4