Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in CacheStorage

Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is loaded for the first time.

BUG=581613
Committed: https://crrev.com/244fe932fce068141e9c52c9f8969b57eb6ccf8c
Cr-Commit-Position: refs/heads/master@{#392522}

[horo, 2016-04-25 09:16:46]

Patchset #1 (id:1) has been deleted

[horo, 2016-04-25 09:21:37]

Patchset #1 (id:20001) has been deleted

[horo, 2016-04-25 09:21:54]

Description was changed from

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

BUG=581613
==========

to

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
ScriptResource::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

[horo, 2016-04-25 09:26:20]

horo@chromium.org changed reviewers:
+ jochen@chromium.org

[horo, 2016-04-25 09:26:20]

jochen@
Could you please review this?

[jochen (gone - plz use gerrit), 2016-04-27 13:21:03]

jochen@chromium.org changed reviewers:
+ vogelheim@chromium.org

[jochen (gone - plz use gerrit), 2016-04-27 13:21:04]

Daniel, can you review this?

Happy to rubberstamp afterwards.

https://codereview.chromium.org/1915833002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/V8CacheStrategiesForCacheStorage.h
(right):

https://codereview.chromium.org/1915833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8CacheStrategiesForCacheStorage.h:10:
enum V8CacheStrategiesForCacheStorage {
what about making this an enum class?

[vogelheim, 2016-04-27 18:16:36]

I'm happy with what this does, but not how. 

The problem I have is centered around the decision making. Some background: The
V8 parse cache existed before the v8 code cache, and at the time some components
- e.g. Resource::setCachedMetadata - would make ad-hoc decisions about what to
do with the data, which wasn't apparent to the caller. This made in hard to
implement the new feature - code caching - and so we pulled out all of that
logic and made it explicit to the caller:
- Resource::setCachedMetadata receives a CacheType parameter, telling it what to
do.
- V8ScriptRunner::compileScript receives a V8CacheOptions parameter, telling it
what to do.
- Later, we added the CachedMetadataHandler as a parameter, telling
V8ScriptRunner::compileScript how to save things.

In all those cases, the caller explicitly decides what to do, and the 'lower
levels' don't have to somehow examine the context in order to figure out what
it's supposed to accomplish.

With this change, much of the previous code's problems are re-introduced: The
decisions are made fairly far down in the call stack, in ways that aren't
readily visible to the parts in the middle. It all works out as you've written
it, but I'm afraid this isn't super maintainable.

Would it be possible to change this so that:
- A ServiceWorkerCacheStorageCachedMetadataHandler does the right thing for SW
cache storage resources.
  - This thing would then probably 'know' the right cache storage cache name
(and origin?)
- V8CacheOptions gets a new option, V8CacheOptionsAggressive (or
V8CacheOptionsAlways)
  - which would be implemented in V8ScriptRunner, along with all the other
options
  - The --v8-cache-options command-line switch probably wouldn't use it; there's
no need to add all that boilerplate.
- Then, whatever thing calls V8ScriptRunner::compileScript for service worker
cache storage scripts, would examine V8CacheStrategiesForCacheStorage and
translate this into the right combination of CacheMetadataHandler +
V8CacheOptions.
- Ideally, no code down-stream from this place would have to know about any of
this.


Sorry for the long-winded feedback... does this explanation make any sense? (I
guess if my request is excessively burdensome we can go ahead with the change
as-is; but I'd be happier if you could give it a try.)

[horo, 2016-04-30 05:30:41]

vogelheim@
I uploaded Patch Set 3.
ScriptController::executeScriptAndReturnValue() sets v8CacheOptions when the
resource is from CacheStorage.
How about it?

https://codereview.chromium.org/1915833002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/V8CacheStrategiesForCacheStorage.h
(right):

https://codereview.chromium.org/1915833002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8CacheStrategiesForCacheStorage.h:10:
enum V8CacheStrategiesForCacheStorage {
On 2016/04/27 13:21:04, jochen wrote:
> what about making this an enum class?

Done.

[horo, 2016-04-30 10:03:33]

Description was changed from

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
ScriptResource::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

to

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

[vogelheim, 2016-05-03 11:36:05]

lgtm

I like this version better. Thank you for doing the extra work.

I found only two nitpicks, where I suspect you got it right, but I'm not quite
sure. Just have a look & proceed as you see fit. :)

https://codereview.chromium.org/1915833002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/1915833002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_unittest.cc:256: nullptr) {
The implementation will blindly deref this pointer rather than check for
nullptr. Not sure if this is ok for a test...

https://codereview.chromium.org/1915833002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1915833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:134: if
(source.resource() &&
!source.resource()->response().cacheStorageCacheName().isNull()) {
patch set #2 implemented this so that the ServiceWorker app-cahce would use the
code cache if V8CacheOptions was V8CacheOptionsCode (or *Default) AND
V8CacheStrategiesForCacheStorage were Normal or Aggressive. With this setting,
V8CacheStrategiesForCacheStorage is independent of V8CacheOptions.

Either is fine; please pick as you see fit. I just can't tell whether this is an
intentional change or not. :)

https://codereview.chromium.org/1915833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:141:
v8CacheOptions = V8CacheOptionsDefault;
V8CacheOptionsCode?

(Loosely related to the previous comment: *Default is (currently) the same as
*Code, but the idea is that *Default can be changed if we change the default,
while *Code describes a specific behaviour.)

[jochen (gone - plz use gerrit), 2016-05-03 11:37:29]

lgtm

[horo, 2016-05-09 04:00:04]

https://codereview.chromium.org/1915833002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/1915833002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_view_host_unittest.cc:256: nullptr) {
On 2016/05/03 11:36:05, vogelheim wrote:
> The implementation will blindly deref this pointer rather than check for
> nullptr. Not sure if this is ok for a test... 

Sorry, I don't understand what is the problem here...

https://codereview.chromium.org/1915833002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/1915833002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:141:
v8CacheOptions = V8CacheOptionsDefault;
On 2016/05/03 11:36:05, vogelheim wrote:
> V8CacheOptionsCode?
> 
> (Loosely related to the previous comment: *Default is (currently) the same as
> *Code, but the idea is that *Default can be changed if we change the default,
> while *Code describes a specific behaviour.)

Yes.
I think V8CacheStrategiesForCacheStorage should be independent of
V8CacheOptions.
So I changed it to V8CacheOptionsCode.

[horo, 2016-05-09 04:06:46]

horo@chromium.org changed reviewers:
+ mkwst@chromium.org

[horo, 2016-05-09 04:06:46]

mkwst@
Could you please review  content/common/render_process_messages.h?

[Mike West, 2016-05-09 18:48:49]

IPC LGTM % sanity check question.

https://codereview.chromium.org/1915833002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1915833002/diff/120001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:581:
scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(data.size()));
How much data do you expect to receive here? Can you add in any sanity checks?

[horo, 2016-05-10 01:15:51]

Thank you!

https://codereview.chromium.org/1915833002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/1915833002/diff/120001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:581:
scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(data.size()));
On 2016/05/09 18:48:48, Mike West wrote:
> How much data do you expect to receive here? Can you add in any sanity checks?

I couldn’t find the UMA for the V8 code cache size.
But the mean of V8.CodeCacheSizeRatio is 103.
So I think that the size of it is similar to the JS file size.
(+CC:yangguo@)

The data is generated by V8.
So it is hard to do sanity checks in the browser process.

[horo, 2016-05-10 01:15:57]

The CQ bit was checked by horo@chromium.org

[horo, 2016-05-10 01:15:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1915833002/#ps120001
(title: "add comments in render_process_messages.h")

[commit-bot: I haz the power, 2016-05-10 01:16:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915833002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915833002/120001

[commit-bot: I haz the power, 2016-05-10 02:28:01]

Description was changed from

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

to

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

[commit-bot: I haz the power, 2016-05-10 02:28:03]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-05-10 02:29:17]

Description was changed from

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.


BUG=581613
==========

to

==========
Introduce Platform::cacheMetadataInCacheStorage() to store V8 code cache in
CacheStorage

We will introduce v8-cache-strategies-for-cache-storage flag (none, normal and
aggressive) in https://codereview.chromium.org/1914963002/.
The flag will be used to call
WebSettings::setV8CacheStrategiesForCacheStorage().

- When the flag is "none", we don't store the V8 code cache in CacheStorage.
- When the flag is "normal", V8 code cache will be stored when the script is
loaded twice in 72 hours. This behavior is same as V8 code cache in HTTPCache
and implemented in V8ScriptRunner.
- When the flag is "aggressive", V8 code cache will be stored when the script is
loaded for the first time.

BUG=581613

Committed: https://crrev.com/244fe932fce068141e9c52c9f8969b57eb6ccf8c
Cr-Commit-Position: refs/heads/master@{#392522}
==========

[commit-bot: I haz the power, 2016-05-10 02:29:19]

Patchset 5 (id:??) landed as
https://crrev.com/244fe932fce068141e9c52c9f8969b57eb6ccf8c
Cr-Commit-Position: refs/heads/master@{#392522}