Optionally compress V8 code cache data with snappy.

Optionally compress V8 code cache data with snappy.

R=jochen@chromium.org, vogelheim@chromium.org

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

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

[Yang, 2014-11-12 13:17:07]

Corresponding Chrome CL is here: https://codereview.chromium.org/721683002/

[marja, 2014-11-12 14:09:36]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2014-11-12 14:09:37]

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:148:
resource->setCachedMetadata(streamer->cachedDataType(), reinterpret_cast<const
char*>(newCachedData->data), newCachedData->length, Resource::CacheLocally);
This needs to be changed too, no? Now the streaming code path is not in sync
with non-streaming code path; streaming never produces compressed data. Doesn't
sound right.

Another question is though whether streaming should produce the code cache ever.
Ultimately probably yes, but we'll probably anyway need to run separate Finch
experiments for them.

[marja, 2014-11-12 14:11:43]

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata* cachedMetadata =
resource->cachedMetadata(cacheTag);
Actually... doesn't this break... so if the user first runs with the "produce
code cache" flag, then the data gets written to the disk, then the user runs
with the "produce compressed code cache flag", and we read the data and try to
decompress -> fail.

I think the cache tag needs to change depending on whether the data is
compressed or not.

[Yang, 2014-11-12 14:17:36]

On 2014/11/12 14:11:43, marja wrote:
>
https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
> File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
> Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata* cachedMetadata
=
> resource->cachedMetadata(cacheTag);
> Actually... doesn't this break... so if the user first runs with the "produce
> code cache" flag, then the data gets written to the disk, then the user runs
> with the "produce compressed code cache flag", and we read the data and try to
> decompress -> fail.
> 
> I think the cache tag needs to change depending on whether the data is
> compressed or not.

That's correct. I talked to Daniel about this. He is going to rework the whole
tagging system so this will change. We think we can live with this state for
now.

[jochen (gone - plz use gerrit), 2014-11-12 15:04:41]

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

https://codereview.chromium.org/720783002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:548: if (settings->v8CacheOptions()
== V8CacheOptionsCode
nit. everything in one line. blink has no line length limit

https://codereview.chromium.org/720783002/diff/20001/Source/core/core.gyp
File Source/core/core.gyp (right):

https://codereview.chromium.org/720783002/diff/20001/Source/core/core.gyp#new...
Source/core/core.gyp:193: '<(DEPTH)/third_party/snappy/snappy.gyp:snappy',
alphabetical ordering.

plz also update the BUILD.gn file

[vogelheim, 2014-11-12 18:53:52]

lgtm

lgtm (with Jochen's comments. I think he and/or Marja need to owner-approve this
anyhow.)

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata* cachedMetadata =
resource->cachedMetadata(cacheTag);
On 2014/11/12 14:11:43, marja wrote:
> Actually... doesn't this break... so if the user first runs with the "produce
> code cache" flag, then the data gets written to the disk, then the user runs
> with the "produce compressed code cache flag", and we read the data and try to
> decompress -> fail.
> 
> I think the cache tag needs to change depending on whether the data is
> compressed or not.


This indeed kinda breaks this. As I'm about to change the cache tag logic
anyhow, I saw two options: Make this CL wait until I fixed the other parts, or
accept temporary edge case breakage (in flag-protected code) and just adapt this
when I do my own follow-up CL a few days afterwards. The latter seemed the
easiest.

If that is not the way to do this, then I'd suggest I'll simply merge this
locally into my change.

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

https://codereview.chromium.org/720783002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunnerTest.cpp:151: //        Enable EXPECT when
code caching lands.
Would there be any way for the test to verify that compression has indeed
occurred?

(In this form, the test doesn't really distinguish between code and
code-compressed.)

[haraken, 2014-11-13 01:31:27]

>
https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
> Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata* cachedMetadata
=
> resource->cachedMetadata(cacheTag);
> On 2014/11/12 14:11:43, marja wrote:
> > Actually... doesn't this break... so if the user first runs with the
"produce
> > code cache" flag, then the data gets written to the disk, then the user runs
> > with the "produce compressed code cache flag", and we read the data and try
to
> > decompress -> fail.
> > 
> > I think the cache tag needs to change depending on whether the data is
> > compressed or not.
> 
> 
> This indeed kinda breaks this. As I'm about to change the cache tag logic
> anyhow, I saw two options: Make this CL wait until I fixed the other parts, or
> accept temporary edge case breakage (in flag-protected code) and just adapt
this
> when I do my own follow-up CL a few days afterwards. The latter seemed the
> easiest.

Given that we've caused tag breakage recently, I'd prefer fixing the tag system
first in order not to pile up problems.

[Yang, 2014-11-13 08:57:45]

On 2014/11/13 01:31:27, haraken wrote:
> >
>
https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
> > Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata*
cachedMetadata
> =
> > resource->cachedMetadata(cacheTag);
> > On 2014/11/12 14:11:43, marja wrote:
> > > Actually... doesn't this break... so if the user first runs with the
> "produce
> > > code cache" flag, then the data gets written to the disk, then the user
runs
> > > with the "produce compressed code cache flag", and we read the data and
try
> to
> > > decompress -> fail.
> > > 
> > > I think the cache tag needs to change depending on whether the data is
> > > compressed or not.
> > 
> > 
> > This indeed kinda breaks this. As I'm about to change the cache tag logic
> > anyhow, I saw two options: Make this CL wait until I fixed the other parts,
or
> > accept temporary edge case breakage (in flag-protected code) and just adapt
> this
> > when I do my own follow-up CL a few days afterwards. The latter seemed the
> > easiest.
> 
> Given that we've caused tag breakage recently, I'd prefer fixing the tag
system
> first in order not to pile up problems.

@jochen: I addressed your comments.
@haraken: I don't think this counts as piling up problems. Neither of the two
colliding options are enabled so far and I'd like to get compression landed
before the cache tag overhaul that Daniel is planning. In the meantime I'll
change V8 to gracefully catch unexpected cache data instead of crashing.

[haraken, 2014-11-13 09:29:15]

On 2014/11/13 08:57:45, Yang wrote:
> On 2014/11/13 01:31:27, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/720783002/diff/1/Source/bindings/core/v8/V8Sc...
> > > Source/bindings/core/v8/V8ScriptRunner.cpp:96: CachedMetadata*
> cachedMetadata
> > =
> > > resource->cachedMetadata(cacheTag);
> > > On 2014/11/12 14:11:43, marja wrote:
> > > > Actually... doesn't this break... so if the user first runs with the
> > "produce
> > > > code cache" flag, then the data gets written to the disk, then the user
> runs
> > > > with the "produce compressed code cache flag", and we read the data and
> try
> > to
> > > > decompress -> fail.
> > > > 
> > > > I think the cache tag needs to change depending on whether the data is
> > > > compressed or not.
> > > 
> > > 
> > > This indeed kinda breaks this. As I'm about to change the cache tag logic
> > > anyhow, I saw two options: Make this CL wait until I fixed the other
parts,
> or
> > > accept temporary edge case breakage (in flag-protected code) and just
adapt
> > this
> > > when I do my own follow-up CL a few days afterwards. The latter seemed the
> > > easiest.
> > 
> > Given that we've caused tag breakage recently, I'd prefer fixing the tag
> system
> > first in order not to pile up problems.
> 
> @jochen: I addressed your comments.
> @haraken: I don't think this counts as piling up problems. Neither of the two
> colliding options are enabled so far and I'd like to get compression landed
> before the cache tag overhaul that Daniel is planning. In the meantime I'll
> change V8 to gracefully catch unexpected cache data instead of crashing.

ok, LGTM.

[marja, 2014-11-13 09:36:30]

And wdyt about my previous comment about the streaming code path now never
producing compressed data?

[Yang, 2014-11-13 13:03:12]

On 2014/11/13 09:36:30, marja wrote:
> And wdyt about my previous comment about the streaming code path now never
> producing compressed data?

I tried to address this for now (I assume this is also going to change in the
near future anyways).

[marja, 2014-11-13 13:15:09]

lgtm otherwise

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

https://codereview.chromium.org/720783002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:151: writeToCache(resource,
streamer->cachedDataType(), Resource::CacheLocally, newCachedData, compressed);
So this doesn't write the code cache to disk... at least you should add a todo
for that. vogelheim@'s tag cleanup will probably make fixing that easier.

[Yang, 2014-11-13 13:20:03]

On 2014/11/13 13:15:09, marja wrote:
> lgtm otherwise
> 
>
https://codereview.chromium.org/720783002/diff/80001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/V8ScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/720783002/diff/80001/Source/bindings/core/v8/...
> Source/bindings/core/v8/V8ScriptRunner.cpp:151: writeToCache(resource,
> streamer->cachedDataType(), Resource::CacheLocally, newCachedData,
compressed);
> So this doesn't write the code cache to disk... at least you should add a todo
> for that. vogelheim@'s tag cleanup will probably make fixing that easier.

Added TODO as suggested.

I guess I should wait for jochen's ok?

[marja, 2014-11-13 13:28:56]

idk; on one hand, haraken@ and me are owners, but on the other hand, jochen@
already gave comments...

[jochen (gone - plz use gerrit), 2014-11-13 15:11:56]

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

[jochen (gone - plz use gerrit), 2014-11-13 15:11:57]

lgtm with nits

https://codereview.chromium.org/720783002/diff/100001/Source/bindings/core/DEPS
File Source/bindings/core/DEPS (right):

https://codereview.chromium.org/720783002/diff/100001/Source/bindings/core/DE...
Source/bindings/core/DEPS:10: "+third_party/snappy/src",
nit third_party/snappy

https://codereview.chromium.org/720783002/diff/100001/Source/core/BUILD.gn
File Source/core/BUILD.gn (right):

https://codereview.chromium.org/720783002/diff/100001/Source/core/BUILD.gn#ne...
Source/core/BUILD.gn:66: "//third_party/snappy",
should not be required

[Yang, 2014-11-13 15:42:49]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-11-13 15:43:33]

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

[commit-bot: I haz the power, 2014-11-13 17:06:58]

Committed patchset #7 (id:120001) as 185305

[Stephen White, 2014-11-13 19:53:12]

On 2014/11/13 17:06:58, I haz the power (commit-bot) wrote:
> Committed patchset #7 (id:120001) as 185305

I'm suspecting this change may be breaking the AOSP build:

http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...

Is that possible?

stdio is:

Export includes file: external/chromium_org/GypAndroid.linux-arm.mk --
out/target/product/generic/obj/STATIC_LIBRARIES/third_party_openmax_dl_dl_openmax_dl_armv7_gyp_intermediates/export_includes
Import includes file:
out/target/product/generic/obj/STATIC_LIBRARIES/gin_gin_gyp_intermediates/import_includes
make: *** No rule to make target
`external/chromium_org/third_party/snappy/src/snappy-sinksource.cc', needed by
`out/target/product/generic/obj/STATIC_LIBRARIES/third_party_snappy_snappy_gyp_intermediates/third_party/snappy/src/snappy-sinksource.o'.
 Stop.
make: *** Waiting for unfinished jobs....
Export includes file: external/chromium_org/GypAndroid.linux-arm.mk --
out/target/product/generic/obj/STATIC_LIBRARIES/gin_gin_gyp_intermediates/export_includes

[Yang, 2014-11-18 05:30:30]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-11-18 05:30:49]

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

[commit-bot: I haz the power, 2014-11-18 05:31:36]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185498