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#newcode300 src/spaces.cc:300: // ASSERT(size_ == 0); On 2012/12/24 14:46:46, haitao.feng wrote:
On 2012/12/20 10:18:35, danno wrote: > Why this change?
"make x64.debug.check" has some errors. When I debugged one case, I
found out
size_ is equal to size_executable_. Could you talk a little on the
comment
"TODO(gc) this will be true again when we fix FreeMemory" here and "//
TODO(gc)
make code_range part of memory allocator?" several lines below?
The point of this comment is that pages allocated directly CodeRange are managed by the code range, not by the MemoryAllocator. This suggests that any allocations, including code allocations, probably shouldn't adjust either size_ or executable_size_ on the MemoryAllocator, and neither should FreeMemory calls on them either. https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.cc#newcode50 src/deoptimizer.cc:50: eager_deoptimization_entry_start_ = allocator->ReserveChunk(deopt_table_size, Please don't store into a member variable and change after the fact. Set a local variable and store the member variable only once with the tweaked address, i.e.: Address chunk = allocator->ReserveChunk(...); eager_deoptimization_entry_start_ = chunk + MemoryAllocator::CodePageAreaStartOffset(); https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.cc#newcode77 src/deoptimizer.cc:77: delete eager_deoptimization_entry_code_; These must be destructed _after_ the memory chunks, don't they, since they are initialized before? https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.cc#newcode1607 src/deoptimizer.cc:1607: MemoryChunk** chunk = type == EAGER Can you please not use the conditional operator here to be tricky with the assignment address? Just store the results of CommitChunk into chunk which is just a MemoryChunk* and use an if statement afterwards to assign to either data->eager_deoptimization_chunk_ or data->lazy_deoptimization_chunk_. https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.cc#newcode1610 src/deoptimizer.cc:1610: if (*chunk == NULL) { I don't think this is right. If the deopt table grows (which it does), this routine gets called multiple times. In that case, you need to make sure that the entire grown table is committed. Here you only commit once with the initial table size. Or am I missing something? https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.h File src/deoptimizer.h (right): https://codereview.chromium.org/11566011/diff/11001/src/deoptimizer.h#newcode117 src/deoptimizer.h:117: VirtualMemory* lazy_deoptimization_entry_code_; Rename this and the one above to XXX_memory_ rather than XXX_code_ https://codereview.chromium.org/11566011/diff/11001/src/platform.h File src/platform.h (right): https://codereview.chromium.org/11566011/diff/11001/src/platform.h#newcode387 src/platform.h:387: void Set(void* address, size_t size) { I don't think you need this, see the comment in spaces.cc https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode290 src/spaces.cc:290: // ASSERT(size_ == 0); This is _not_ safe to remove. Either you are leaking somewhere, or the logic to track size_/executable_size_ is wrong. If size_ is executable_size_ when you tear down, then you didn't release something with FreeMemory, or (more likely) FreeMemory/ReserveAlignedMemory needs to be adjusted to properly to manage size_/executable_size_ uniformly in the case that the memory comes from the code range (either always account for the memory in the MemoryAllocator's statistics or never). https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode350 src/spaces.cc:350: VirtualMemory reservation; Remove. https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode361 src/spaces.cc:361: reservation.Set(static_cast<void*>(base), reserved); This isn't correct. The VirtualMemory from the code range is managed by the CodeRange itself. By adding the Set method, you've made it possible to double-manage a VirtualMemory block. I think you should just create an empty VirtualMemory, i.e.: VirtualMemory empty; controller->TakeControl(empty); The calling code needs to be able to deal with the fact that it's not responsible for the memory management of a request. https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode369 src/spaces.cc:369: reservation.TakeControl(&temp); Replace with: controller->TakeControl(temp); https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode370 src/spaces.cc:370: size_ += reservation.size(); temp.size() https://codereview.chromium.org/11566011/diff/11001/src/spaces.cc#newcode376 src/spaces.cc:376: controller->TakeControl(&reservation); Remove https://codereview.chromium.org/11566011/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
