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#newcode426
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#newcode408
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.
https://codereview.chromium.org/373713006/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
