Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(80)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6542047: Basic implementation of incremental marking. (Closed)

Created:
9 years, 10 months ago by Vyacheslav Egorov (Chromium)
Modified:
9 years, 7 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

Basic implementation of incremental marking. Based on classic tri-color abstraction (white --- not seen, grey --- seen and in the marking stack, black --- seen and scanned) with a write barrier that transfers black objects to grey when we assign to white object into field. Currently write-barrier is out-lined (generated code just calls C function) so write-intensive applications are expected to show significant degradation. Committed: http://code.google.com/p/v8/source/detail?r=6916

Patch Set 1 #

Total comments: 57
Unified diffs Side-by-side diffs Delta from patch set Stats (+778 lines, -109 lines) Patch
M src/SConscript View 1 chunk +1 line, -0 lines 2 comments Download
M src/arm/deoptimizer-arm.cc View 1 chunk +2 lines, -1 line 2 comments Download
M src/assembler.h View 2 chunks +4 lines, -3 lines 2 comments Download
M src/assembler.cc View 2 chunks +6 lines, -0 lines 0 comments Download
M src/builtins.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M src/compiler-intrinsics.h View 1 chunk +1 line, -1 line 2 comments Download
M src/debug.cc View 4 chunks +5 lines, -5 lines 2 comments Download
M src/deoptimizer.h View 1 chunk +2 lines, -1 line 2 comments Download
M src/deoptimizer.cc View 1 chunk +4 lines, -1 line 0 comments Download
M src/execution.h View 2 chunks +4 lines, -1 line 0 comments Download
M src/execution.cc View 5 chunks +24 lines, -3 lines 0 comments Download
M src/flag-definitions.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/heap.h View 1 chunk +18 lines, -0 lines 0 comments Download
M src/heap.cc View 4 chunks +18 lines, -1 line 2 comments Download
M src/heap-inl.h View 1 chunk +2 lines, -2 lines 2 comments Download
M src/ia32/assembler-ia32-inl.h View 3 chunks +11 lines, -3 lines 0 comments Download
M src/ia32/deoptimizer-ia32.cc View 3 chunks +4 lines, -1 line 0 comments Download
M src/ia32/lithium-codegen-ia32.cc View 1 chunk +13 lines, -3 lines 2 comments Download
M src/ia32/lithium-ia32.h View 1 chunk +11 lines, -2 lines 0 comments Download
M src/ia32/lithium-ia32.cc View 1 chunk +3 lines, -1 line 0 comments Download
M src/ia32/macro-assembler-ia32.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/ia32/macro-assembler-ia32.cc View 2 chunks +23 lines, -1 line 0 comments Download
M src/ia32/stub-cache-ia32.cc View 3 chunks +13 lines, -6 lines 0 comments Download
M src/ic-inl.h View 1 chunk +1 line, -0 lines 0 comments Download
A src/incremental-marking.h View 1 chunk +352 lines, -0 lines 19 comments Download
A src/incremental-marking.cc View 1 chunk +43 lines, -0 lines 1 comment Download
M src/lithium-allocator.cc View 1 chunk +7 lines, -0 lines 0 comments Download
M src/liveedit.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M src/mark-compact.h View 2 chunks +4 lines, -10 lines 0 comments Download
M src/mark-compact.cc View 14 chunks +137 lines, -21 lines 16 comments Download
M src/objects.cc View 2 chunks +5 lines, -5 lines 0 comments Download
M src/objects-inl.h View 22 chunks +23 lines, -30 lines 2 comments Download
M src/serialize.cc View 1 chunk +5 lines, -2 lines 0 comments Download
M src/spaces.cc View 4 chunks +5 lines, -0 lines 0 comments Download
M src/spaces-inl.h View 2 chunks +12 lines, -2 lines 0 comments Download
M src/x64/deoptimizer-x64.cc View 1 chunk +2 lines, -1 line 1 comment Download

Messages

Total messages: 3 (0 generated)
Vyacheslav Egorov (Chromium)
9 years, 10 months ago (2011-02-21 16:16:54 UTC) #1
Erik Corry
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 ...
9 years, 10 months ago (2011-02-22 12:27:19 UTC) #2
Vyacheslav Egorov (Chromium)
9 years, 10 months ago (2011-02-23 14:31:46 UTC) #3 
Thanks. Committing.

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
On 2011/02/22 12:27:19, Erik Corry wrote:
> c is before s in the latin alphabet.

Done.

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#newc...
src/arm/deoptimizer-arm.cc:124: void Deoptimizer::PatchStackCheckCodeAt(Code*
code,
On 2011/02/22 12:27:19, Erik Corry wrote:
> Should we add a TODO to make use of this argument?

Done.

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));
On 2011/02/22 12:27:19, Erik Corry wrote:
> 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.

Done.

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.
On 2011/02/22 12:27:19, Erik Corry wrote:
> It seems to me that this should either be unchanged or 2011.

Done.

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));
On 2011/02/22 12:27:19, Erik Corry wrote:
> Perhaps rename this variable to something more meaningful, then you can
replace
> this->code() with just code() below.

Done.

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,
On 2011/02/22 12:27:19, Erik Corry wrote:
> Call this variable unoptimized_code?

Done.

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");
On 2011/02/22 12:27:19, Erik Corry wrote:
> menal -> mental
> SWEEEP -> SWEEP

Done.

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.c...
src/ia32/lithium-codegen-ia32.cc:1961: __ IncrementalMarkingRecordWrite(object,
value, scratch);
On 2011/02/22 12:27:19, Erik Corry wrote:
> 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.

Yes.

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#newco...
src/incremental-marking.h:48: static void Start() {
On 2011/02/22 12:27:19, Erik Corry wrote:
> 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().

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:89: state_ = STOPPED;
On 2011/02/22 12:27:19, Erik Corry wrote:
> Fits on one line.

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:129: static const intptr_t kFactor = 8;
On 2011/02/22 12:27:19, Erik Corry wrote:
> Perhaps call this kAllocationMarkingFactor.

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:221: // Grey markbits: 01
On 2011/02/22 12:27:19, Erik Corry wrote:
> Probably should be 11 when we handle overflow.

Yes. Will investigate when we start handling overflow.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:255: return "???";
On 2011/02/22 12:27:19, Erik Corry wrote:
> UNREACHABLE()?

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:303: WhiteToGrey(object);
On 2011/02/22 12:27:19, Erik Corry wrote:
> This pushes the object onto the stack.  When it is popped the map will be
> marked.

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:305: if (IsWhite(map)) WhiteToGrey(map);
On 2011/02/22 12:27:19, Erik Corry wrote:
> Here we also mark the map.  Aren't we doing it twice?

Done.

http://codereview.chromium.org/6542047/diff/1/src/incremental-marking.h#newco...
src/incremental-marking.h:321: // We are sweeping code and map spaces precisely
so clearing is not required.
On 2011/02/22 12:27:19, Erik Corry wrote:
> lint?

Done.

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: 
On 2011/02/22 12:27:19, Erik Corry wrote:
> 2 blank lines here and several places.

Done.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode155
src/mark-compact.cc:155: // if (IsCompacting()) tracer_->set_is_compacting();
On 2011/02/22 12:27:19, Erik Corry wrote:
> Commented code.

Done.

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.
On 2011/02/22 12:27:19, Erik Corry wrote:
> Or perhaps we should compact the marking stack by removing pointers to filler
> objects in this case.

We will think about it.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode892
src/mark-compact.cc:892: UNREACHABLE();
On 2011/02/22 12:27:19, Erik Corry wrote:
> TODO?

Done.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode893
src/mark-compact.cc:893: #if 0
On 2011/02/22 12:27:19, Erik Corry wrote:
> Commented code.

Done.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode1175
src/mark-compact.cc:1175: /*if (FLAG_collect_maps &&
On 2011/02/22 12:27:19, Erik Corry wrote:
> Commented code.  TODO?

Done.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode1515
src/mark-compact.cc:1515: // FlushCode::ProcessCandidates();
On 2011/02/22 12:27:19, Erik Corry wrote:
> Commented code and missing TODO'

Done.

http://codereview.chromium.org/6542047/diff/1/src/mark-compact.cc#newcode2006
src/mark-compact.cc:2006: object->Size() == 4 ||
On 2011/02/22 12:27:19, Erik Corry wrote:
> kPointerSize?

Done.

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);                      \
On 2011/02/22 12:27:19, Erik Corry wrote:
> Indentation of backslashes is now wrong.

Done.

Powered by Google App Engine
This is Rietveld 408576698