LGTM
http://codereview.chromium.org/6542047/diff/1/src/SConscript File src/SConscript (right): http://codereview.chromium.org/6542047/diff/1/src/SConscript#newcode86 src/SConscript:86: incremental-marking.cc c is before s in the latin alphabet. http://codereview.chromium.org/6542047/diff/1/src/arm/deoptimizer-arm.cc File src/arm/deoptimizer-arm.cc (right): http://codereview.chromium.org/6542047/diff/1/src/arm/deoptimizer-arm.cc#newcode124 src/arm/deoptimizer-arm.cc:124: void Deoptimizer::PatchStackCheckCodeAt(Code* code, Should we add a TODO to make use of this argument? http://codereview.chromium.org/6542047/diff/1/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/6542047/diff/1/src/assembler.h#newcode274 src/assembler.h:274: INLINE(void set_target_address(Address target, Code* code)); Could you use GetCodeFromTargetAddress instead of adding an argument? Actually it seems that they are not the same object. Perhaps a better name than 'code' can be found. http://codereview.chromium.org/6542047/diff/1/src/compiler-intrinsics.h File src/compiler-intrinsics.h (right): http://codereview.chromium.org/6542047/diff/1/src/compiler-intrinsics.h#newcode1 src/compiler-intrinsics.h:1: // Copyright 2010 the V8 project authors. All rights reserved. It seems to me that this should either be unchanged or 2011. http://codereview.chromium.org/6542047/diff/1/src/debug.cc File src/debug.cc (right): http://codereview.chromium.org/6542047/diff/1/src/debug.cc#newcode376 src/debug.cc:376: Handle<Code> code(Code::GetCodeFromTargetAddress(target)); Perhaps rename this variable to something more meaningful, then you can replace this->code() with just code() below. http://codereview.chromium.org/6542047/diff/1/src/deoptimizer.h File src/deoptimizer.h (right): http://codereview.chromium.org/6542047/diff/1/src/deoptimizer.h#newcode142 src/deoptimizer.h:142: static void PatchStackCheckCodeAt(Code* code, Call this variable unoptimized_code? http://codereview.chromium.org/6542047/diff/1/src/heap-inl.h File src/heap-inl.h (right): http://codereview.chromium.org/6542047/diff/1/src/heap-inl.h#newcode286 src/heap-inl.h:286: for (int i = 0; i < len; i++) { I'm an idiot! http://codereview.chromium.org/6542047/diff/1/src/heap-inl.h#newcode287 src/heap-inl.h:287: StoreBuffer::Mark(address + start + i*kPointerSize); Spaces around * http://codereview.chromium.org/6542047/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/6542047/diff/1/src/heap.cc#newcode476 src/heap.cc:476: PrintF("[IncremenalMarker] SCAVENGE -> MARK-SWEEEP\n"); menal -> mental SWEEEP -> SWEEP http://codereview.chromium.org/6542047/diff/1/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/6542047/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode1961 src/ia32/lithium-codegen-ia32.cc:1961: __ IncrementalMarkingRecordWrite(object, value, scratch); This looks rather horrible in terms of some performance issues. We should probably experiment with rescanning the cell space at the end of the incremental marking. http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.cc File src/incremental-marking.cc (right): http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.cc#newcode1 src/incremental-marking.cc:1: // Copyright 2010 the V8 project authors. All rights reserved. 2011 http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h File src/incremental-marking.h (right): http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode1 src/incremental-marking.h:1: // Copyright 2010 the V8 project authors. All rights reserved. 2011 http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode48 src/incremental-marking.h:48: static void Start() { I think it would be cleaner to move largish functions that are not inline/not called very often into the .cc file. This includes Start() and Hurry(). http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode89 src/incremental-marking.h:89: state_ = STOPPED; Fits on one line. http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode129 src/incremental-marking.h:129: static const intptr_t kFactor = 8; Perhaps call this kAllocationMarkingFactor. http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode212 src/incremental-marking.h:212: return Marking::IsMarked(obj->address()); Assert that the other bit is not marked? http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode221 src/incremental-marking.h:221: // Grey markbits: 01 Probably should be 11 when we handle overflow. http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode223 src/incremental-marking.h:223: return Marking::IsMarked(obj->address() + kPointerSize); Assert that the other bit is not marked? http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode255 src/incremental-marking.h:255: return "???"; UNREACHABLE()? http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode303 src/incremental-marking.h:303: WhiteToGrey(object); This pushes the object onto the stack. When it is popped the map will be marked. http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode305 src/incremental-marking.h:305: if (IsWhite(map)) WhiteToGrey(map); Here we also mark the map. Aren't we doing it twice? http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newcode321 src/incremental-marking.h:321: // We are sweeping code and map spaces precisely so clearing is not required. lint? http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc File src/mark-compact.cc (right): http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode105 src/mark-compact.cc:105: 2 blank lines here and several places. http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode155 src/mark-compact.cc:155: // if (IsCompacting()) tracer_->set_is_compacting(); Commented code. http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode232 src/mark-compact.cc:232: // on top of it to see whether they are equal to old_start. Or perhaps we should compact the marking stack by removing pointers to filler objects in this case. http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode892 src/mark-compact.cc:892: UNREACHABLE(); TODO? http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode893 src/mark-compact.cc:893: #if 0 Commented code. http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode1175 src/mark-compact.cc:1175: /*if (FLAG_collect_maps && Commented code. TODO? http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode1515 src/mark-compact.cc:1515: // FlushCode::ProcessCandidates(); Commented code and missing TODO' http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode2006 src/mark-compact.cc:2006: object->Size() == 4 || kPointerSize? http://codereview.chromium.org/6542047/diff/1/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/6542047/diff/1/src/objects-inl.h#newcode84 src/objects-inl.h:84: WRITE_BARRIER(this, offset, value); \ Indentation of backslashes is now wrong. http://codereview.chromium.org/6542047/diff/1/src/x64/deoptimizer-x64.cc File src/x64/deoptimizer-x64.cc (right): http://codereview.chromium.org/6542047/diff/1/src/x64/deoptimizer-x64.cc#newcode203 src/x64/deoptimizer-x64.cc:203: void Deoptimizer::PatchStackCheckCodeAt(Code* code, Add TODO? http://codereview.chromium.org/6542047/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
