predictors: Expose the initialization call to Android.

predictors: Expose the initialization call to Android.

The resource_prefetch_predictor needs to be initialized before issuing
prefetches. The initialization is usually triggered by the first page
load completion, but this is not suitable for external triggering on
Android. This exposes the initialization method to the Java side.

BUG=650250, 631966
Committed: https://crrev.com/88af26a7ceb6bf5a35b12f14d70f761360d98b56
Cr-Commit-Position: refs/heads/master@{#432213}

[Benoit L, 2016-11-15 13:24:58]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[Benoit L, 2016-11-15 13:25:03]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 13:25:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 14:00:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 14:00:41]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-11-15 14:29:17]

I think it would be a simpler API to ensure initialization in
::StartPrefetching. The ResourcePrefetchPredictor already knows whether it is
initialized, so it feels natural to me to work with this state in
ResourcePrefetchPredictor.

The startInitialization() Java method does not tell us when initialization is
done, does it mean it does not reliably prefetch afterwards? I am guessing
(please confirm) that this prefers not doing a prefetch rather than doing it too
slowly on initialization path. Not sure it is justified by data though. We can
mitigate that in the predictor as well, and this would be more
cross-platform-testable and tune-able.

I guess my main concern is that there is some risk that we'd forget to call
startInitialization() in some codepath, or it would duplicate the initialization
state bit on the java side.

The Cons of my proposal is that initializing always properly in async code is
annoying. It would require something like adding a flag that initialization was
already requested, and having a queue of requests blocked by initialization. We
are doing it in simplecache, so the concept is not too alien to me, but I agree
it does not look very readable.

[Benoit L, 2016-11-15 15:07:13]

On 2016/11/15 14:29:17, pasko wrote:
> I think it would be a simpler API to ensure initialization in
> ::StartPrefetching. The ResourcePrefetchPredictor already knows whether it is
> initialized, so it feels natural to me to work with this state in
> ResourcePrefetchPredictor.
> 
> The startInitialization() Java method does not tell us when initialization is
> done, does it mean it does not reliably prefetch afterwards? I am guessing
> (please confirm) that this prefers not doing a prefetch rather than doing it
too
> slowly on initialization path. Not sure it is justified by data though. We can
> mitigate that in the predictor as well, and this would be more
> cross-platform-testable and tune-able.
> 
> I guess my main concern is that there is some risk that we'd forget to call
> startInitialization() in some codepath, or it would duplicate the
initialization
> state bit on the java side.
> 
> The Cons of my proposal is that initializing always properly in async code is
> annoying. It would require something like adding a flag that initialization
was
> already requested, and having a queue of requests blocked by initialization.
We
> are doing it in simplecache, so the concept is not too alien to me, but I
agree
> it does not look very readable.

Per offline discussion, the concerns expressed here are all valid, though there
is no issue for the local tests. Let's revisit when we move beyond local tests
for the predictor.

[pasko, 2016-11-15 15:11:07]

discussed offline and agreed that this externally requested initialization is
simpler in short term. We should reconsider it when other entry points to the
ResourcePrefetchPredictor are introduced.

Also agreed to throw a RuntimeException if the startInitialization() was not
called before startPrefetching() - plz do.

[Benoit L, 2016-11-15 15:28:07]

On 2016/11/15 15:11:07, pasko wrote:
> discussed offline and agreed that this externally requested initialization is
> simpler in short term. We should reconsider it when other entry points to the
> ResourcePrefetchPredictor are introduced.
> 
> Also agreed to throw a RuntimeException if the startInitialization() was not
> called before startPrefetching() - plz do.

Done.

[Benoit L, 2016-11-15 15:28:08]

On 2016/11/15 15:11:07, pasko wrote:
> discussed offline and agreed that this externally requested initialization is
> simpler in short term. We should reconsider it when other entry points to the
> ResourcePrefetchPredictor are introduced.
> 
> Also agreed to throw a RuntimeException if the startInitialization() was not
> called before startPrefetching() - plz do.

Done.

[Benoit L, 2016-11-15 15:29:17]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 15:29:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2016-11-15 15:56:42]

lgtm

[commit-bot: I haz the power, 2016-11-15 16:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 16:55:26]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-11-15 17:32:52]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-11-15 17:33:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 17:51:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-15 18:02:29]

Description was changed from

==========
predictors: Expose the initialization call to Android.

The resource_prefetch_predictor needs to be initialized before issuing
prefetches. The initialization is usually triggered by the first page
load completion, but this is not suitable for external triggering on
Android. This exposes the initialization method to the Java side.

BUG=650250,631966
==========

to

==========
predictors: Expose the initialization call to Android.

The resource_prefetch_predictor needs to be initialized before issuing
prefetches. The initialization is usually triggered by the first page
load completion, but this is not suitable for external triggering on
Android. This exposes the initialization method to the Java side.

BUG=650250,631966

Committed: https://crrev.com/88af26a7ceb6bf5a35b12f14d70f761360d98b56
Cr-Commit-Position: refs/heads/master@{#432213}
==========

[commit-bot: I haz the power, 2016-11-15 18:02:30]

Patchset 2 (id:??) landed as
https://crrev.com/88af26a7ceb6bf5a35b12f14d70f761360d98b56
Cr-Commit-Position: refs/heads/master@{#432213}