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#newcode408
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.
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.
