Closed Bug 1371333 Opened 7 years ago Closed 7 years ago

CacheEntry::lastFecthed is updated before it is provided to the HttpChannel.

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

Bug 1368675 added a way to get the lastFetched value out of the cache entry, in order to ideally use it the same way as Chrome is doing, for implementing heuristics for the caching JS bytecode.

Telemetry from Bug 1362114 reported that all the lastFetched value that we can read from the CacheEntry are all below or equal to 1 second.

Investigating the code, it seems that the lastFetched attribute from the metadata is being updated when the CacheEntry::OnFetched function is called [1], but only later provided as a result to the nsHttpChannel::OnCacheEntryAvailable [1].

Thus, the lastFetched value is not observable by the nsHttpChannel nor the HttpChannelChild instances.

So, I suggest 2 things:

1/ We remove the code to get the lastFetched information from nsICacheChannelInfo.

2/ We expose the predictor confidence (0 - 100) [2], computed based on the lastFecthed info, to the nsICacheChannelInfo such that we can use the same base metric [3] as necko for feeding the alternate data cache.

The Predictor::CalculatePredictions function, can attach the confidence on the nsICacheEntry, which can then be mirrored by the nsICacheChannelInfo.


Valentin, do you know how the confidence metrics evolves over time? Do you think it would be good to re-use this metric for alternate data heuristics?  Or should we cache the lastFetched value and pass-it on?

[1] http://searchfox.org/mozilla-central/source/netwerk/cache2/CacheEntry.cpp#863,866,886,889
[2] http://searchfox.org/mozilla-central/source/netwerk/base/Predictor.cpp#115,118,121
[3] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=1&end_date=2017-06-07&keys=__none__!__none__!__none__&max_channel_version=nightly%252F55&measure=PREDICTOR_CONFIDENCE&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-03-08&table=0&trim=1&use_submission_date=0
Flags: needinfo?(valentin.gosu)
I'm not sure the predictor heuristic is the best for the JS bytecode. Nick and Honza know more about this topic than I do and maybe they can suggest something here.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(hurley)
Flags: needinfo?(honzab.moz)
I'm not sure if this has been filed before or after I talked with nbp about using frecency instead.  If after, then it seems the plan to use has been abandoned?

I don't know the predictor logic at all, I believe Nick has chosen it more or less arbitrarily.

nbb, can you please clearly (re)state what problem the heuristic has to solve?  It's probably a good start to make some decision here.
Flags: needinfo?(honzab.moz) → needinfo?(nicolas.b.pierron)
(In reply to Honza Bambas (:mayhemer) from comment #2)
> I'm not sure if this has been filed before or after I talked with nbp about
> using frecency instead.  If after, then it seems the plan to use has been
> abandoned?

This happened after, I went to the shortest implementation time, by exposing the information which was already present, and only noticed the lastFetch being constantly 0 while looking at the telemetry (Bug 1362114 comment 4)

> nbb, can you please clearly (re)state what problem the heuristic has to
> solve?  It's probably a good start to make some decision here.

The heuristics has to determine if the resource is going to be used in a near future to decide whether it is worth to spend time encoding the bytecode, and memory to save it.

In Bug 1362114, I am trying to collect and correlate all these data and see if some of these metrics can highlight a sweet spot for a simple test. In Bug 1362114 comment 3, I defined the heuristics as a way to minimize a cost function, but I literally have no idea what to expect from the correlation, and that is what I want to study with this telemetry.

I have no particular fondness nor intuition in the lastFetch metric, except that this is what is currently used in Chrome.

Note, this is exploration work, so this can definitely wait and this is not a blocker for any of the work of the JS bytecode cache.  I can definitely settled on implementing something based on the fetchCount as only metric, and refine the heuristics later.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > nbb, can you please clearly (re)state what problem the heuristic has to
> > solve?  It's probably a good start to make some decision here.
> 
> The heuristics has to determine if the resource is going to be used in a
> near future to decide whether it is worth to spend time encoding the
> bytecode, and memory to save it.

this is not exactly the answer I wanted.  I am more interested in why you can't just always store.  I know we might be talking about it on IRC, but listing the reasons for record here would be good.

Thanks.
(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > > nbb, can you please clearly (re)state what problem the heuristic has to
> > > solve?  It's probably a good start to make some decision here.
> > 
> > The heuristics has to determine if the resource is going to be used in a
> > near future to decide whether it is worth to spend time encoding the
> > bytecode, and memory to save it.
> 
> this is not exactly the answer I wanted.  I am more interested in why you
> can't just always store.  I know we might be talking about it on IRC, but
> listing the reasons for record here would be good.

The problem, is that the bytecode cache takes time to generate and slow down the execution by ~10%, in order to later reduce it on future visits.  Adding this cost on the first visit ""might"" be a bad user experiences for web-sites which are not visited frequently.

The result of telemetry probes (Bug 1362114) should help us see what the threshold should be, and if any of this work is actually needed.
I'm not a fan of exposing the predictor confidence. This value is, as Honza mentioned, essentially arbitrary - I/we reserve the right to change how that's calculated *at any time*. That's why we never store it anywhere - it's a transient property designed to be calculated when it's used and never again (and, in fact, the confidence has a pretty high likelihood of changing every time it's calculated).

Essentially, it's an internal implementation detail of the predictor, and not for use by anyone else.
Flags: needinfo?(hurley)
Given:

(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #6)
> I'm not a fan of exposing the predictor confidence. [...]
> 
> Essentially, it's an internal implementation detail of the predictor, and
> not for use by anyone else.

is this bug just down to the following, now?

(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> 
> 1/ We remove the code to get the lastFetched information from
> nsICacheChannelInfo.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Andrew Overholt [:overholt] from comment #7)
> is this bug just down to the following, now?
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #0)
> > 
> > 1/ We remove the code to get the lastFetched information from
> > nsICacheChannelInfo.

Likely yes, I will have to check if the telemetry results suggest that the fetchCount is enough for the JSBC heuristic.
Comment on attachment 8890457 [details] [diff] [review]
Remove unused CacheTokenLastFetched from CacheInfoChannel.

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

Look good. r=me
Attachment #8890457 - Flags: review?(valentin.gosu) → review+
I will land this patch as soon as I remove the last uses in Bug 900784.
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d578eb28a8e2
Remove unused CacheTokenLastFetched from CacheInfoChannel. r=valentin
https://hg.mozilla.org/mozilla-central/rev/d578eb28a8e2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(nicolas.b.pierron)
Assignee: nobody → nicolas.b.pierron
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.