Verify that source string matches serialized code.

Verify that source string matches serialized code.

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

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

[vogelheim, 2014-07-15 13:42:31]

lgtm

https://codereview.chromium.org/394793002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/394793002/diff/1/src/objects.h#newcode8867
src/objects.h:8867: };
design nitpick: Not sure if exposing this class is the best choice. Given that -
because of the private constructor - the only use is to call
IteratingStringHasher::Hash, maybe one could simply add the static method to
StringHasher, and then IteratingStringHasher could remain an anonymous helper
class in objects.cc.

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

https://codereview.chromium.org/394793002/diff/1/test/cctest/test-serialize.c...
test/cctest/test-serialize.cc:711: 
Just for my understanding: Previously, this would check that the 2nd compile
would use the cache (because otherwise we'd get a different result). This CL
would prevent that from working. What this checks now is that caching will work
with a string that's physically different but logically the same?

[Yang, 2014-07-15 13:47:31]

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

https://codereview.chromium.org/394793002/diff/1/test/cctest/test-serialize.c...
test/cctest/test-serialize.cc:711: 
On 2014/07/15 13:42:31, vogelheim wrote:
> Just for my understanding: Previously, this would check that the 2nd compile
> would use the cache (because otherwise we'd get a different result). This CL
> would prevent that from working. What this checks now is that caching will
work
> with a string that's physically different but logically the same?

You are right. Currently, if CompileScript was to simply ignore
CONSUME_CACHED_DATA, we wouldn't notice anymore. I have no idea how we would
test that though. Unless we still use a wrong source string in release mode,
where we don't have the source string verification.

[Yang, 2014-07-15 14:15:09]

On 2014/07/15 13:47:31, Yang wrote:
> https://codereview.chromium.org/394793002/diff/1/test/cctest/test-serialize.cc
> File test/cctest/test-serialize.cc (right):
> 
>
https://codereview.chromium.org/394793002/diff/1/test/cctest/test-serialize.c...
> test/cctest/test-serialize.cc:711: 
> On 2014/07/15 13:42:31, vogelheim wrote:
> > Just for my understanding: Previously, this would check that the 2nd compile
> > would use the cache (because otherwise we'd get a different result). This CL
> > would prevent that from working. What this checks now is that caching will
> work
> > with a string that's physically different but logically the same?
> 
> You are right. Currently, if CompileScript was to simply ignore
> CONSUME_CACHED_DATA, we wouldn't notice anymore. I have no idea how we would
> test that though. Unless we still use a wrong source string in release mode,
> where we don't have the source string verification.

Updated test case to verify that when we consume cached data, compile does not
trigger. I'm (ab)using histogram counters for that.

[mvstanton, 2014-07-15 14:39:37]

LGTM, one comment.

https://codereview.chromium.org/394793002/diff/20001/test/cctest/test-seriali...
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/394793002/diff/20001/test/cctest/test-seriali...
test/cctest/test-serialize.cc:696: // We use histogram counters to verify that
compile did not trigger when
Could this "VerifyNoCompilation" stuff be wrapped in something like a scope for
cleaner re-use? I'm thinking it might get used a few more times.

[mvstanton, 2014-07-15 14:54:31]

still lgtm.

https://codereview.chromium.org/394793002/diff/40001/test/cctest/test-seriali...
File test/cctest/test-serialize.cc (right):

https://codereview.chromium.org/394793002/diff/40001/test/cctest/test-seriali...
test/cctest/test-serialize.cc:709: DisallowCompilation
no_compile_expected(isolate);
Elegant!

[vogelheim, 2014-07-15 15:30:09]

lgtm

Neat! That's nicer than the old implementation. :)

[Yang, 2014-07-16 06:59:34]

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