1368675 - Add a heuristics for triggering the bytecode cache

Status: RESOLVED FIXED

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

Description

[Nicolas B. Pierron [:nbp]]

Currently (after Bug 1364117), the bytecode cache has no heuristics except for the eager flag used for testing purposes.

For the record, v8 is using a heuristic[1, 2] based on that fact that they see the cache entry for the second time within the last 72 hours, and that the code size is above 1kB, which would also make sense for us to do.

Our CacheEntry, as part of necko implements a fetchCount attribute which does not seems to have been tested by their experiments.  I am tempted to add multiple strategies, as they did, in order to check against the lastFecthTime, the fetchCount, and the size of the source.

Once we have these multiple strategies, and telemetry (Bug 1362114), we should be able to do some A/B testing to decide which strategy is the best, and maybe refine them.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=399580
[2] https://src.chromium.org/viewvc/blink/trunk/Source/bindings/core/v8/V8ScriptRunner.cpp#l262

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel.

This patch makes copy of the fetchCount and lastFetched fields of the
nsICacheEntry, such that we can read them on the HttpChannelChild instances,
by using the nsICacheInfoChannel interface.

These fields would then be used by the following patch to implement
heuristics for the bytecode cache.

Note: Ideally, I would have wanted to expose the creationTime of the cache
entry, in order to compute a frequency out of the creation time and the
fetchCount, but for the moment I would stick to using the information which
is already present.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Add multiple heuristics to trigger the JS bytecode cache encoding.

This patch replaces the IsEagerBytecodeCache by a function which implements
multiple heuristics based on 3 parameters: fetchCount, last time since we
fetched, and the size of the source.

IsEagerBytecodeCache is now implemented as one strategy which provides none
of the parameters, thus defaulting to saying yes everytime.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8872682 [details] [diff] [review]
Add multiple heuristics to trigger the JS bytecode cache encoding.

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

::: dom/script/ScriptLoader.cpp
@@ +1897,5 @@
> +  size_t sourceLengthMin = 100;
> +  int32_t fetchCountMin = 5;
> +  uint32_t timeSinceLastFecthed = 0;  // Relative time in seconds.
> +
> +  switch (sBytecodeCacheStrategy) {

self-nit: I will add a -2 value, which would correspond to a "reader mode" by always returning false, as it would keep requesting existing saved bytecode but not saved any new one.  This might help reproduce issues such as Bug 1368887.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8872682 [details] [diff] [review]
Add multiple heuristics to trigger the JS bytecode cache encoding.

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

::: dom/script/ScriptLoader.cpp
@@ +1903,5 @@
> +      // Eager mode, skip heuristics!
> +      hasSourceLengthMin = false;
> +      hasFetchCountMin = false;
> +      hasTimeSinceLastFecthed = false;
> +    }

self-nit: missing "break;" statement. idem below. Otherwise the test case is failing because the eager mode is no longer eager.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8872680 [details]
Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel.

Review of attachment 8872680 [details]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2576,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(_retval);
> +  // Do not check mCacheEntryAvailable, as we want to get this value after the
> +  // OnStopRequest call.
> +  *_retval = mCacheFetchCount;

self-nit: I should guard with mAfterOnStartRequestBegun, as this information is provided as a StartRequest argument.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8872682 [details] [diff] [review]
Add multiple heuristics to trigger the JS bytecode cache encoding.

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

This looks good. r=me with my comments addressed (and your comments fixed as well).

::: dom/script/ScriptLoader.cpp
@@ +1867,5 @@
>    return NS_OK;
>  }
>  
> +/* static */ bool
> +ScriptLoader::BytecodeCacheHeuristics(ScriptLoadRequest* aRequest)

Nit (and feel free to ignore this one!): Is there any reason not to call this ScriptLoader::ShouldCacheBytecode? I think it reads better at the callsite and any cache is, by definition, based on heuristics.

@@ +1869,5 @@
>  
> +/* static */ bool
> +ScriptLoader::BytecodeCacheHeuristics(ScriptLoadRequest* aRequest)
> +{
> +  // We need the nsICacheInfoChannel to exists to be able to open the laternate

Nits: "to exist" and "alternate"

@@ +1884,5 @@
> +  static int32_t sBytecodeCacheStrategy = 0;
> +  static bool sBytecodeCacheStrategyPrefCached = false;
> +  if (!sBytecodeCacheStrategyPrefCached) {
> +    sBytecodeCacheStrategyPrefCached = true;
> +    Preferences::AddIntVarCache(&sBytecodeCacheStrategy,

Now that this isn't a debug/test-only pref, it seems worth it to add it to the list of prefs in nsContentUtils, which will let this code read a little more cleanly.

@@ +1892,5 @@
> +
> +  // List of parameters used by the strategies.
> +  bool hasSourceLengthMin = false;
> +  bool hasFetchCountMin = false;
> +  bool hasTimeSinceLastFecthed = false;

Here and below: Fetched* (not Fecthed)!

@@ +1990,5 @@
> +    if (NS_FAILED(aRequest->mCacheInfo->GetCacheTokenLastFetched(&lastFecthed))) {
> +      LOG(("ScriptLoadRequest (%p): Bytecode-cache: Cannot get lastFetched.", aRequest));
> +      return false;
> +    }
> +    uint32_t now = PR_Now() / PR_USEC_PER_SEC;

I think we're being encouraged to use mozilla::TimeStamp these days. You could also probably use TimeTamp::NowLowRes in this case, too, which has less of a performance impact.

::: modules/libpref/init/all.js
@@ +224,5 @@
> +//          seen for the first time, independently of the size or last access
> +//          time.
> +//   *  0 : (default) The bytecode would be saved if the script is larger than
> +//          1kB, has been accessed within the last 72 hours, and fetched at
> +//          least 5 times.

Nit: this comment seems quite likely to bitrot relatively quickly. It might be better to just point to nsScriptLoader.cpp for the latest heuristics.

Comment 7

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

Comment on attachment 8872680 [details]
Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel.

Review of attachment 8872680 [details]:
-----------------------------------------------------------------

::: netwerk/base/nsICacheInfoChannel.idl
@@ +11,5 @@
>  {
>    /**
> +   * Get the number of times the cache entry has been opened. This attribute is
> +   * equivalent to nsICachingChannel.cacheToken.fetchCount.
> +   */

Please state when these metods return error codes.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2586,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(_retval);
> +  // Do not check mCacheEntryAvailable, as we want to get this value after the
> +  // OnStopRequest call.
> +  *_retval = mCacheLastFetched;

Are you OK with returning fetchCount=0,lastFetched=0 when there's no cache entry in the parent?

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Valentin Gosu [:valentin] from comment #7)
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +2586,5 @@
> > +{
> > +  NS_ENSURE_ARG_POINTER(_retval);
> > +  // Do not check mCacheEntryAvailable, as we want to get this value after the
> > +  // OnStopRequest call.
> > +  *_retval = mCacheLastFetched;
> 
> Are you OK with returning fetchCount=0,lastFetched=0 when there's no cache
> entry in the parent?

A 0 fetchCount would probably void any heuristics, and a lastFetched timestamp set to EPOCH will likely void any heuristics too.  So, yes I am ok with these default values when we "had" no cache entry in the parent at the beginning of the OnStartRequest.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Valentin Gosu [:valentin] from comment #7)
> ::: netwerk/base/nsICacheInfoChannel.idl
> @@ +11,5 @@
> >  {
> >    /**
> > +   * Get the number of times the cache entry has been opened. This attribute is
> > +   * equivalent to nsICachingChannel.cacheToken.fetchCount.
> > +   */
> 
> Please state when these metods return error codes.

Thinking twice while writing the documentation, I copied [1] condition to the HttpChannelChild::DoOnStopRequest function, and copied the flag the same way the cache entry is copied in the nsHttpChannel class before the mCacheEntryAvailable flag is reset [2].

Thus, I removed the comments from GetCacheTokenFetchCount and GetCacheTokenLastFetched and added a check for to return NS_ERROR_NOT_AVAILABLE if neither the cache entry nor the alternate data one are set.  Which makes more sense in terms of documentation.

I will submit an updated version of the patch for review.

[1] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#7375-7381
[2] http://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1117

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel.

Delta:
This patch add the mAltDataCacheEntryAvailable flag to the HttpChannelChild,
which mimic the mAltDataCacheEntry pointer from nsHttpChannel.

The accessors from nsHttpChannel and HttpChannelChild are thus modified to
be available when either the cache entry, or the alt-data cache entry are
available.

Comment 11

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

Comment on attachment 8873916 [details] [diff] [review]
Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel.

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1149,5 @@
> +  // the cache entry in case the child calls openAlternativeOutputStream().
> +  // (see nsHttpChannel::OnStopRequest)
> +  if (!mPreferredCachedAltDataType.IsEmpty()) {
> +    mAltDataCacheEntryAvailable = mCacheEntryAvailable;
> +  }

nit: I think you can init this in DoOnStartRequest. Then we can have the condition in the extra methods just be if (!mAltDataCacheEntryAv) ...

@@ +2684,5 @@
>  NS_IMETHODIMP
> +HttpChannelChild::GetCacheTokenFetchCount(int32_t *_retval)
> +{
> +  NS_ENSURE_ARG_POINTER(_retval);
> +  if (!mCacheEntryAvailable || !mAltDataCacheEntryAvailable) {

nit: If you make the change above, you can keep only the second condition.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Valentin Gosu [:valentin] from comment #11)
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1149,5 @@
> > +  // the cache entry in case the child calls openAlternativeOutputStream().
> > +  // (see nsHttpChannel::OnStopRequest)
> > +  if (!mPreferredCachedAltDataType.IsEmpty()) {
> > +    mAltDataCacheEntryAvailable = mCacheEntryAvailable;
> > +  }
> 
> nit: I think you can init this in DoOnStartRequest. Then we can have the
> condition in the extra methods just be if (!mAltDataCacheEntryAv) ...

The reason I added it in the DoOnStopRequest function is to mimic what is done in the nsHttpChannel, which initializes the mAltDataCacheEntry in the OnStopRequest function.  With this code, mAltDataCacheEntryAvailable child state mirrors mAltDataCacheEntry non-null parent condition.

I would be ok to do this change, if we were to do it as well in nsHttpChannel too.  What do you think?

> @@ +2684,5 @@
> >  NS_IMETHODIMP
> > +HttpChannelChild::GetCacheTokenFetchCount(int32_t *_retval)
> > +{
> > +  NS_ENSURE_ARG_POINTER(_retval);
> > +  if (!mCacheEntryAvailable || !mAltDataCacheEntryAvailable) {
> 
> nit: If you make the change above, you can keep only the second condition.

If I were to record the fetchCount and lastFetched only when we are trying to register an alternate cache entry, I would be ok.  For telemetry reasons, I want to record these values when no alternate data type are registered (yet).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5d32c4c0ac
Expose nsICacheEntry fetchCount and lastFetched time as read-only on the nsICacheInfoChannel. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/95405c5cf513
Add multiple heuristics to trigger the JS bytecode cache encoding. r=mrbkap

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed5d32c4c0ac
https://hg.mozilla.org/mozilla-central/rev/95405c5cf513

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> @@ +1990,5 @@
> > +    if (NS_FAILED(aRequest->mCacheInfo->GetCacheTokenLastFetched(&lastFecthed))) {
> > +      LOG(("ScriptLoadRequest (%p): Bytecode-cache: Cannot get lastFetched.", aRequest));
> > +      return false;
> > +    }
> > +    uint32_t now = PR_Now() / PR_USEC_PER_SEC;
> 
> I think we're being encouraged to use mozilla::TimeStamp these days. You
> could also probably use TimeTamp::NowLowRes in this case, too, which has
> less of a performance impact.

Apparently the modifications I made to the cache prevented this code from actually running.  Now that I fixed (locally) the condition for GetCacheTokenLastFetched, I see that TimeStamp() does not correspond to Epoch (and asserts), which is the time reference used by the lastFetched value.

Working around this issue would imply adding much more code, and to still query a RealTime clock to get the same reference in order to construct a TimeDuration, so I will revert part of this change and use PR_Now() to construct the TimeDuration locally, instead of TimeStamp::NowLoRes().