Implement Blink-side changes to enable V8 code caching.

Implement Blink-side changes to enable V8 code caching.

Further info:
- Design doc etc. is found in the referenced issue.
- Main part is V8ScriptRunner. Most of the rest is merely there to
  support getting the settings string in the right place.
- Depends on a Chromium CL (forthcoming) for the new setting.
- Support for Workers not yet implemented.

BUG=399580
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179643

[jochen (gone - plz use gerrit), 2014-08-04 09:27:29]

https://codereview.chromium.org/432273004/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/432273004/diff/1/Source/core/fetch/Resource.c...
Source/core/fetch/Resource.cpp:426: m_cachedMetadata.clear();
have you verified that this works across reloads, or will we get both, the old
and the new meta data back from the disk cache?

https://codereview.chromium.org/432273004/diff/1/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/432273004/diff/1/public/web/WebSettings.h#new...
public/web/WebSettings.h:192: virtual void setV8CacheOptions(const WebString&) =
0;
i think it would be preferable to use an enum instead of a string here, wdyt?

[haraken, 2014-08-04 14:10:46]

Looked at the design document. This is fantastic work!

[vogelheim, 2014-08-05 16:03:40]

PTAL.

https://codereview.chromium.org/432273004/diff/1/Source/core/fetch/Resource.cpp
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/432273004/diff/1/Source/core/fetch/Resource.c...
Source/core/fetch/Resource.cpp:426: m_cachedMetadata.clear();
On 2014/08/04 09:27:29, jochen wrote:
> have you verified that this works across reloads, or will we get both, the old
> and the new meta data back from the disk cache?

Not yet. :) Will do before proceeding with the CL.

https://codereview.chromium.org/432273004/diff/1/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/432273004/diff/1/public/web/WebSettings.h#new...
public/web/WebSettings.h:192: virtual void setV8CacheOptions(const WebString&) =
0;
On 2014/08/04 09:27:29, jochen wrote:
> i think it would be preferable to use an enum instead of a string here, wdyt?

Done.

[jochen (gone - plz use gerrit), 2014-08-05 16:10:01]

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8CacheOptionsType.h (right):

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8CacheOptionsType.h:36: enum V8CacheOptionsType {
i'd name this V8CacheOptions (and the values V8CacheOptionsOff, etc..)

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:75: if (!resource ||
!resource->url().protocolIsInHTTPFamily() || code->Length() < 1024) {
nit. no { }

https://codereview.chromium.org/432273004/diff/20001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/432273004/diff/20001/public/web/WebSettings.h...
public/web/WebSettings.h:56: V8CacheOff,
typically, the enum values start with the entire name of the enum, i.e.
V8CacheOptionsOff

[vogelheim, 2014-08-05 17:02:52]

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8CacheOptionsType.h (right):

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8CacheOptionsType.h:36: enum V8CacheOptionsType {
On 2014/08/05 16:10:01, jochen wrote:
> i'd name this V8CacheOptions (and the values V8CacheOptionsOff, etc..)

Done.

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/432273004/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:75: if (!resource ||
!resource->url().protocolIsInHTTPFamily() || code->Length() < 1024) {
On 2014/08/05 16:10:01, jochen wrote:
> nit. no { }

Done.

https://codereview.chromium.org/432273004/diff/20001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/432273004/diff/20001/public/web/WebSettings.h...
public/web/WebSettings.h:56: V8CacheOff,
On 2014/08/05 16:10:01, jochen wrote:
> typically, the enum values start with the entire name of the enum, i.e.
> V8CacheOptionsOff

Done.

[jochen (gone - plz use gerrit), 2014-08-05 17:06:15]

the code changes look good

did you check the disk cache interaction?

[blink-reviews, 2014-08-05 17:29:54]

On Tue, Aug 5, 2014 at 7:06 PM, <jochen@chromium.org> wrote:

> the code changes look good
>
> did you check the disk cache interaction?
>

It's kinda weird: The cache update seems to work as expected, but the
about:cache page for the page seems to crash...  once and then it works.
:-P  Maybe just a fluke, but I think I'll look into this a bit more... :)

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2014-08-06 11:26:34]

lgtm

[vogelheim, 2014-08-06 16:17:00]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-08-06 16:18:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vogelheim@chromium.org/432273004/60001

[vogelheim, 2014-08-06 16:24:50]

The CQ bit was unchecked by vogelheim@chromium.org

[vogelheim, 2014-08-06 16:51:57]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-08-06 16:52:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vogelheim@chromium.org/432273004/60001

[commit-bot: I haz the power, 2014-08-06 18:43:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[commit-bot: I haz the power, 2014-08-06 19:01:23]

Change committed as 179643

[marja, 2014-08-18 10:46:53]

Post-commit comment:

Huh, afaics this disables the parser cache. There's no place that would set it
the cache options to something else than "off". :/

(And empirically, the cache indeed seems to be off..)

[vogelheim, 2014-08-18 13:25:08]

I hope not... :) (I'm on vacation for a week so I hope this isn't urgent or
anything)

- There's a Chrome-side companion CL (referenced in the tracking bug) with
implements a --v8-cache-options flag to turn on different options. (Should
probably be renamed --js-cache-options for consistency, but that's besides
the point.)
- This could not be in the same CL since it's in a different repo.
- Before submit, I tested (with BOTH patches) that the switch will put
either code or parser cache data into the chrome cache, and that the data
makes its way back to V8. I have not tested for realz whether V8 does
something useful with it, though. I planned to use benchmarks for that.
- I do believe it was default-disabled before, so this would make no user
visible difference, right?

HTH... I'll look into this when I'm back.

Daniel


On Mon, Aug 18, 2014 at 12:46 PM, <marja@chromium.org> wrote:

> Post-commit comment:
>
> Huh, afaics this disables the parser cache. There's no place that would
> set it
> the cache options to something else than "off". :/
>
> (And empirically, the cache indeed seems to be off..)
>
> https://codereview.chromium.org/432273004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[marja, 2014-08-18 13:44:51]

Do not read this e-mail, go back to vacation :)

It wasn't default-disabled before. It was just never put to disk, but the data
was produced and kept in a in-memory cache. Afaics, now we don't do even that
any more.