This CL seems to work really hard to duplicate functionality that already
exists. If you look at MemoryAllocator::AllocateChunk, it already tries to put
all EXECUTABLE allocates in the CodeRange if it exists. Because of that, the
functionality you want is reduced to an ordering problem. If you can ensure that
the CodeRange exists and is initialized before you request the space for the
deopt tables, then you can use MemoryAllocator::AllocateChunk to give you the
memory you need. You will probably need to pass through a flag that prevents
committing the executable pages and do that later, but that shouldn't be too
hard. With this approach, a very large amount of the code in this CL is
unnecessary.

I also feel very strongly that we should allocate the deopt entry tables in the same way on all platforms, there should be nothing specific about x64. Please
make this CL cross-platform.


https://codereview.chromium.org/11566011/diff/6001/src/deoptimizer.h
File src/deoptimizer.h (right):

https://codereview.chromium.org/11566011/diff/6001/src/deoptimizer.h#newcode106
src/deoptimizer.h:106: Address eager_deoptimization_entry_start_;
Is there any reason why this shouldn't work identically on all
platforms, taking the deopt tables from the CodeSpace? If not, please
make this cross platform.

https://codereview.chromium.org/11566011/diff/6001/src/spaces.cc
File src/spaces.cc (right):

https://codereview.chromium.org/11566011/diff/6001/src/spaces.cc#newcode212
src/spaces.cc:212: bool committed) {
nit: s/committed/commit

https://codereview.chromium.org/11566011/diff/6001/src/spaces.cc#newcode300
src/spaces.cc:300: // ASSERT(size_ == 0);
Why this change?

https://codereview.chromium.org/11566011/diff/6001/src/spaces.cc#newcode506
src/spaces.cc:506: MemoryChunk*
MemoryAllocator::CommitChunkInCodeRange(Address start,
Yikes! This is copy-pasted from MemoryAllocator::AllocateChunk. At the
very least, you should refactor to share functionality, but please see
my overall comments about this approach.

https://codereview.chromium.org/11566011/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to