http://codereview.chromium.org/9017009/diff/1/src/d8.cc File src/d8.cc (right):
http://codereview.chromium.org/9017009/diff/1/src/d8.cc#newcode1087 src/d8.cc:1087: return i::Thread::Options("IsolateThread", 2 << 20); I would prefer 2 * MB instead of 2 << 20. http://codereview.chromium.org/9017009/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/9017009/diff/1/src/heap.cc#newcode629 src/heap.cc:629: if (gc_performed) one_gc_has_been_performed = true; scavenge will not free any space in LO. gc_performed does not imply full_gc_performed. http://codereview.chromium.org/9017009/diff/1/src/heap.cc#newcode5312 src/heap.cc:5312: max_semispace_size_ = SignedRoundUpToPowerOf2(max_semispace_size_); what's the point of these fields being signed? looks ugly. http://codereview.chromium.org/9017009/diff/1/src/platform-macos.cc File src/platform-macos.cc (right): http://codereview.chromium.org/9017009/diff/1/src/platform-macos.cc#newcode734 src/platform-macos.cc:734: static const int kSamplerThreadStackSize = 32 * KB; sometimes you put constants into the class, sometimes outside of it. consistency? http://codereview.chromium.org/9017009/diff/1/src/spaces.cc File src/spaces.cc (right): http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode383 src/spaces.cc:383: intptr_t expand = Min(Max(size(), space_needed), kPageSize - size()); add a comment that you are trying to a least double it in size. http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode412 src/spaces.cc:412: paged_space->AddToFreeLists(old_end, new_area - old_end); it looks a bit spooky it seems we will be wasting region from end_int - end_int % paged_space->ObjectAlignment() to aligned_end_int until next big GC. it's not much, but probably should be noted in comments. http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode509 src/spaces.cc:509: size_t chunk_size = Max(static_cast<intptr_t>(Page::kPageSize), why are we wasting address space? if body size is smaller than kObjectAreaSize (which might happen if we want to allocate some small immovable object). http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode637 src/spaces.cc:637: RoundUp(chunk->size(), Page::kPageSize), I don't like disagreement between chunk size and actually committed memory size. http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode809 src/spaces.cc:809: if (last_page->size() < Page::kPageSize && comment is talking about "current page" but the code deals with the last page. http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode823 src/spaces.cc:823: if (identity() == OLD_POINTER_SPACE) initial = initial * 8; This looks very magical. I highly recommend to propagate information about the requester with specific needs (e.g. by introducing ReserveLinearAllocationRegion) to here and make all magic conditional. http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode833 src/spaces.cc:833: SignedRoundUpToPowerOf2( SignedRoundUpToPowerOf2 does not look right. Should we finally make all positive size values unsigned? http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode1917 src/spaces.cc:1917: if (large_list_ != NULL && The same piece of (complicated) code is repeated two times. Can you factor it out into a separate function? http://codereview.chromium.org/9017009/diff/1/src/spaces.cc#newcode2264 src/spaces.cc:2264: // The AddToFreeLists call above will reduce the size of the space in the Allocate is increasing size in the accounting stats. Who is going to do that instead of it? http://codereview.chromium.org/9017009/diff/1/src/utils.h File src/utils.h (right): http://codereview.chromium.org/9017009/diff/1/src/utils.h#newcode175 src/utils.h:175: uintptr_t x = static_cast<uintptr_t>(x_argument); inline int SignedRoundUpToPowerOf2(int_type x_argument) { return static_cast<int>(RoundUpToPowerOf2(static_cast<uintptr_t>(x))); } http://codereview.chromium.org/9017009/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
