Refactor ScriptData class for cached compile data.

Refactor ScriptData class for cached compile data.

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

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

[marja, 2014-07-09 14:42:30]

https://codereview.chromium.org/376223002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/376223002/diff/1/src/api.cc#newcode1709
src/api.cc:1709: ASSERT(source->cached_data == NULL);
Shouldn't you crash if source->cached_data != NULL? Now we'll just overwrite it
silently. ASSERT -> CHECK?

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

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode217
src/parser.cc:217: if (HasError()) {
Afaics we'll never have an error in normal operation, just in tests. So maybe
you could just return false here?

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode259
src/parser.cc:259: int symbol_data_offset =
Omg, this is messier than I thought. We don't have symbol data any more! Could
you remove it since you're at it?

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

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:14833: TEST(ExpectFailPreCompileDeserializationError) {
I'd just remove this test, it makes no sense to keep it as a failing test.

If we cannot assert that a CHECK is hit, but we assert that the test fails, then
it rots very easily. E.g., it can start failing due to some other reason than
ScriptData, and nobody will notice. And after that point, it doesn't test
anything.

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

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:1260: if (!pd.HasError()) {
Alright, now I remember why I didn't remove the "ScriptData can have an error"
functionality; it's used in tests but it will never occur in normal operation
(we won't give the data to the embedder if the compilation has failed).

[vogelheim, 2014-07-09 17:23:37]

lgtm

Mostly nitpicks... I do think the crash-on-CacheData-from-the-wrong-version
thing needs to be documented.

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

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode267
src/parser.cc:267: }
nitpick, and not your code, but... this duplicates a bit of logic, which took me
a while to figure out. I'd have written it something like this:

int offset = (data_length > symbol_data_offset)
    ? symbol_data_offset : data_length;
unsigned* data = ...
symbol_data_ = reinterpret_cast<byte*>(data[offset]);
symbol_data_end_ = reinterpret_cast<byte*>(data[data_length]);

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode3663
src/parser.cc:3663: // Invalid cached data if checks fail.
I can't really parse the comment.

https://codereview.chromium.org/376223002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode62
src/parser.h:62: // Wrapper around ScriptData to provide parser-specific
functionality,
super nitpick: '.' instead of ','.

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode81
src/parser.h:81: return
reinterpret_cast<unsigned*>(const_cast<byte*>(script_data_->data()));
Why is this a const method?

(I guess one can define const-ness this way, in that the actual object isn't
changed. But I don't think const-access to a pointer that allows to manipulate
the dependent state is a super readable design. If you really want to do that,
maybe declare script_data_ mutable.)

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode85
src/parser.h:85: bool Sanity();
nitpick: IsSane() ?

(I can see how CHECK(Sanity()) is attractive, but I think IsSane() would be the
more consistent naming. Either way, you pick...)

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode93
src/parser.h:93: int Length() const { return script_data_->length() /
sizeof(unsigned); }
nitpick: round up?

https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h#newcode9
src/preparse-data.h:9: #include "src/compiler.h"
If this is only for CompleteParserRecorder::GetScriptData(), then a forward
declaration of ScriptData would be enough.

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

https://codereview.chromium.org/376223002/diff/1/src/serialize.cc#newcode1910
src/serialize.cc:1910: return GetHeaderValue(kVersionHashOffset) ==
Version::Hash() &&
So... we CHECK(Sanity()) in some places. I had assumed the contract with the
embedder was went something like: Only ever pass in cache data that V8 has
previously given you, or else we shall crash & burn.

But since this checks the version hash, we may *also* crash on data we *have*
previously given to the embedder, namely if it was produced by a different
version of V8. I'm not against that, but I think we need to document that rather
clearly.

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

https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode586
src/serialize.h:586: owns_script_data_ = false;
nitpick: maybe set owns_script_data_ in initializer list?

https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode597
src/serialize.h:597: ScriptData* GetScriptData() {
What if owns_script_data_ is already false when this is called? Maybe add a
CHECK.

[vogelheim, 2014-07-09 17:23:37]

lgtm

Mostly nitpicks... I do think the crash-on-CacheData-from-the-wrong-version
thing needs to be documented.

[marja, 2014-07-09 17:28:51]

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

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:1224: i::ScriptData* sd = log.GetScriptData();
Btw, in these tests, would it be possible to extract the needed information
directly from "log" instead of constructing a ScriptData?

[marja, 2014-07-10 07:14:20]

(Also, feel free to commit this despite my further cleanup suggestions, I don't
want to block your work because of some cleanup that I should've done earlier...
and I can still do it after.)

[Yang, 2014-07-10 08:28:46]

Addressed comments. Please take another look. (Especially Marja, since I changed
a lot of code in the cctests.

https://codereview.chromium.org/376223002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/376223002/diff/1/src/api.cc#newcode1709
src/api.cc:1709: ASSERT(source->cached_data == NULL);
On 2014/07/09 14:42:30, marja wrote:
> Shouldn't you crash if source->cached_data != NULL? Now we'll just overwrite
it
> silently. ASSERT -> CHECK?

Done.

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

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode217
src/parser.cc:217: if (HasError()) {
On 2014/07/09 14:42:30, marja wrote:
> Afaics we'll never have an error in normal operation, just in tests. So maybe
> you could just return false here?

Done.

But if that's the case, couldn't we just remove that HasError field entirely?

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode259
src/parser.cc:259: int symbol_data_offset =
On 2014/07/09 14:42:30, marja wrote:
> Omg, this is messier than I thought. We don't have symbol data any more! Could
> you remove it since you're at it?

Done.

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode267
src/parser.cc:267: }
On 2014/07/09 17:23:36, vogelheim wrote:
> nitpick, and not your code, but... this duplicates a bit of logic, which took
me
> a while to figure out. I'd have written it something like this:
> 
> int offset = (data_length > symbol_data_offset)
>     ? symbol_data_offset : data_length;
> unsigned* data = ...
> symbol_data_ = reinterpret_cast<byte*>(data[offset]);
> symbol_data_end_ = reinterpret_cast<byte*>(data[data_length]);

All gone now anyways.

https://codereview.chromium.org/376223002/diff/1/src/parser.cc#newcode3663
src/parser.cc:3663: // Invalid cached data if checks fail.
On 2014/07/09 17:23:36, vogelheim wrote:
> I can't really parse the comment.

Changed.

https://codereview.chromium.org/376223002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode62
src/parser.h:62: // Wrapper around ScriptData to provide parser-specific
functionality,
On 2014/07/09 17:23:36, vogelheim wrote:
> super nitpick: '.' instead of ','.

Done.

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode81
src/parser.h:81: return
reinterpret_cast<unsigned*>(const_cast<byte*>(script_data_->data()));
On 2014/07/09 17:23:36, vogelheim wrote:
> Why is this a const method?
> 
> (I guess one can define const-ness this way, in that the actual object isn't
> changed. But I don't think const-access to a pointer that allows to manipulate
> the dependent state is a super readable design. If you really want to do that,
> maybe declare script_data_ mutable.)

Done.

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode85
src/parser.h:85: bool Sanity();
On 2014/07/09 17:23:36, vogelheim wrote:
> nitpick: IsSane() ?
> 
> (I can see how CHECK(Sanity()) is attractive, but I think IsSane() would be
the
> more consistent naming. Either way, you pick...)

Done.

https://codereview.chromium.org/376223002/diff/1/src/parser.h#newcode93
src/parser.h:93: int Length() const { return script_data_->length() /
sizeof(unsigned); }
On 2014/07/09 17:23:36, vogelheim wrote:
> nitpick: round up?

No need. Sanity check makes sure the length is a multiple of pointer size. Added
comment.

https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/1/src/preparse-data.h#newcode9
src/preparse-data.h:9: #include "src/compiler.h"
On 2014/07/09 17:23:36, vogelheim wrote:
> If this is only for CompleteParserRecorder::GetScriptData(), then a forward
> declaration of ScriptData would be enough.

Done.

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

https://codereview.chromium.org/376223002/diff/1/src/serialize.cc#newcode1910
src/serialize.cc:1910: return GetHeaderValue(kVersionHashOffset) ==
Version::Hash() &&
On 2014/07/09 17:23:36, vogelheim wrote:
> So... we CHECK(Sanity()) in some places. I had assumed the contract with the
> embedder was went something like: Only ever pass in cache data that V8 has
> previously given you, or else we shall crash & burn.
> 
> But since this checks the version hash, we may *also* crash on data we *have*
> previously given to the embedder, namely if it was produced by a different
> version of V8. I'm not against that, but I think we need to document that
rather
> clearly.

Well you could argue that you can only rely on data given to you by the same V8.
I'll add a comment to V8::ScriptCompiler::CompileUnbound.

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

https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode586
src/serialize.h:586: owns_script_data_ = false;
On 2014/07/09 17:23:37, vogelheim wrote:
> nitpick: maybe set owns_script_data_ in initializer list?

Done.

https://codereview.chromium.org/376223002/diff/1/src/serialize.h#newcode597
src/serialize.h:597: ScriptData* GetScriptData() {
On 2014/07/09 17:23:37, vogelheim wrote:
> What if owns_script_data_ is already false when this is called? Maybe add a
> CHECK.

I think ASSERT should be sufficient. Add that.

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

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:14833: TEST(ExpectFailPreCompileDeserializationError) {
On 2014/07/09 14:42:30, marja wrote:
> I'd just remove this test, it makes no sense to keep it as a failing test.
> 
> If we cannot assert that a CHECK is hit, but we assert that the test fails,
then
> it rots very easily. E.g., it can start failing due to some other reason than
> ScriptData, and nobody will notice. And after that point, it doesn't test
> anything.

Done.

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

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:1224: i::ScriptData* sd = log.GetScriptData();
On 2014/07/09 17:28:51, marja wrote:
> Btw, in these tests, would it be possible to extract the needed information
> directly from "log" instead of constructing a ScriptData?

Done. Also got rid of a bunch of methods from ParseData that became unnecessary.

https://codereview.chromium.org/376223002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:1260: if (!pd.HasError()) {
On 2014/07/09 14:42:30, marja wrote:
> Alright, now I remember why I didn't remove the "ScriptData can have an error"
> functionality; it's used in tests but it will never occur in normal operation
> (we won't give the data to the embedder if the compilation has failed).

Changed to use the log.

[marja, 2014-07-10 08:48:11]

lgtm with comments

https://codereview.chromium.org/376223002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/376223002/diff/40001/include/v8.h#newcode1095
include/v8.h:1095: * produced the the same version of V8.
Nit: the the -> by the

https://codereview.chromium.org/376223002/diff/40001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/376223002/diff/40001/src/parser.h#newcode90
src/parser.h:90: byte* symbol_data_;
symbol_data_ and symbol_data_end_ are now unnecessary, no?

https://codereview.chromium.org/376223002/diff/40001/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/40001/src/preparse-data.h#newc...
src/preparse-data.h:122: // Forward declaration for
CompleteParserRecorder::GetScriptData.
Nit: This comment is unnecessary.

https://codereview.chromium.org/376223002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/376223002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:14833: TEST(ExpectFailPreCompileDeserializationError) {
You removed the other ExpectFail tests, this one could be removed too.. doesn't
add so much value after your changes.

[vogelheim, 2014-07-10 10:05:32]

lgtm

https://codereview.chromium.org/376223002/diff/60001/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/60001/src/preparse-data.h#newc...
src/preparse-data.h:122: class ScriptData;
stlye nitpick: Forward declarations near always follow the #includes and precede
actual class definitions. Not sure if the style guide demands it, but it's
certainly common.

[Yang, 2014-07-10 10:27:58]

https://codereview.chromium.org/376223002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/376223002/diff/40001/include/v8.h#newcode1095
include/v8.h:1095: * produced the the same version of V8.
On 2014/07/10 08:48:10, marja wrote:
> Nit: the the -> by the

Done.

https://codereview.chromium.org/376223002/diff/40001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/376223002/diff/40001/src/parser.h#newcode90
src/parser.h:90: byte* symbol_data_;
On 2014/07/10 08:48:10, marja wrote:
> symbol_data_ and symbol_data_end_ are now unnecessary, no?

Done.

https://codereview.chromium.org/376223002/diff/40001/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/40001/src/preparse-data.h#newc...
src/preparse-data.h:122: // Forward declaration for
CompleteParserRecorder::GetScriptData.
On 2014/07/10 08:48:11, marja wrote:
> Nit: This comment is unnecessary.

Done.

https://codereview.chromium.org/376223002/diff/60001/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/376223002/diff/60001/src/preparse-data.h#newc...
src/preparse-data.h:122: class ScriptData;
On 2014/07/10 10:05:32, vogelheim wrote:
> stlye nitpick: Forward declarations near always follow the #includes and
precede
> actual class definitions. Not sure if the style guide demands it, but it's
> certainly common.

Done.

[Yang, 2014-07-10 10:28:22]

Committed patchset #3 manually as r22314 (presubmit successful).