Chromium Code Reviews

Issue 7302003: Support slots recording for compaction during incremental marking. (Closed)

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

Description

Support slots recording for compaction during incremental marking. Currently we still rely on a single big slots buffer which is not filtered or canonicalized and can grow limitlessly. R=erik.corry@gmail.com Committed: http://code.google.com/p/v8/source/detail?r=8559

Patch Set 1 #

Total comments: 31

Patch Set 2 : fix review remarks, fix bug #

Patch Set 3 : fix presubmit, remove last debug check #

Unified diffs Side-by-side diffs Stats (+496 lines, -238 lines)
M src/assembler.h View 1 chunk +2 lines, -0 lines 0 comments
M src/assembler.cc View 1 chunk +8 lines, -0 lines 0 comments
M src/heap.cc View 1 chunk +17 lines, -7 lines 0 comments
M src/ia32/assembler-ia32-inl.h View 3 chunks +7 lines, -3 lines 0 comments
M src/ia32/code-stubs-ia32.h View 2 chunks +53 lines, -18 lines 0 comments
M src/ia32/code-stubs-ia32.cc View 6 chunks +86 lines, -31 lines 0 comments
M src/ia32/deoptimizer-ia32.cc View 1 chunk +4 lines, -2 lines 0 comments
M src/ia32/full-codegen-ia32.cc View 1 chunk +1 line, -1 line 0 comments
M src/ia32/macro-assembler-ia32.h View 2 chunks +1 line, -6 lines 0 comments
M src/ia32/macro-assembler-ia32.cc View 4 chunks +6 lines, -44 lines 0 comments
M src/incremental-marking.h View 2 chunks +6 lines, -1 line 0 comments
M src/incremental-marking.cc View 8 chunks +38 lines, -13 lines 0 comments
M src/incremental-marking-inl.h View 3 chunks +17 lines, -1 line 0 comments
M src/mark-compact.h View 3 chunks +7 lines, -1 line 0 comments
M src/mark-compact.cc View 13 chunks +73 lines, -35 lines 0 comments
M src/objects-inl.h View 6 chunks +23 lines, -14 lines 0 comments
M src/serialize.cc View 1 chunk +6 lines, -0 lines 0 comments
M src/spaces.h View 7 chunks +31 lines, -9 lines 0 comments
M src/spaces.cc View 9 chunks +110 lines, -52 lines 0 comments

Messages

Total messages: 3 (0 generated)
Vyacheslav Egorov (Chromium)
9 years, 5 months ago (2011-07-01 19:31:33 UTC) #1
Erik Corry
LGTM http://codereview.chromium.org/7302003/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7302003/diff/1/src/heap.cc#newcode4320 src/heap.cc:4320: mark_compact_collector()->RecordSlot(slot, slot, object); Does this happen? The pointer ...
9 years, 5 months ago (2011-07-04 11:04:10 UTC) #2
Vyacheslav Egorov (Chromium)
9 years, 4 months ago (2011-08-05 12:50:28 UTC) #3 
http://codereview.chromium.org/7302003/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7302003/diff/1/src/heap.cc#newcode4320
src/heap.cc:4320: mark_compact_collector()->RecordSlot(slot, slot, object);
On 2011/07/04 11:04:11, Erik Corry wrote:
> Does this happen?  The pointer can't suddenly start pointing into the
evacuation
> candidate unless we are allocating in the evacuation candidate while promoting
> from new space.  It seems wrong to allocate in an evacuation candidate once we
> have decided it is an evacuation candidate.

This else if belongs to the first if. So yes this can happen. Added a comment.

http://codereview.chromium.org/7302003/diff/1/src/ia32/assembler-ia32-inl.h
File src/ia32/assembler-ia32-inl.h (right):

http://codereview.chromium.org/7302003/diff/1/src/ia32/assembler-ia32-inl.h#n...
src/ia32/assembler-ia32-inl.h:97: code, NULL, HeapObject::cast(target_code));
On 2011/07/04 11:04:11, Erik Corry wrote:
> Is this 'NULL' OK because we cannot compact code space?  A comment would be
> good.

Done.

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6241: // The first instruction is generated as a
label so as to get the offset
On 2011/07/04 11:04:11, Erik Corry wrote:
> first instruction is -> first two instructions are
> as a label -> with labels

Done.

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6322: switch (evacuation_state) {
On 2011/07/04 11:04:11, Erik Corry wrote:
> This switch introduces a lot of boilerplate compared with the alternative:
> 
> if (evacuation_stat == kWithEvacuationCandidates) hh
>   __ mov...
> } else {
>   ASSERT(evacutaion_state == kWithoutEvacuationCandidates);
>   __ mov...
>   __ mov...
> }
> 
> 7 lines instead of 10.

Done.

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6337: switch (evacuation_state) {
On 2011/07/04 11:04:11, Erik Corry wrote:
> And here.

Done.

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.h#newc...
src/ia32/code-stubs-ia32.h:550: static const byte kFiveByteNopInstruction =
0x3d;  // Cmpl al, #imm32.
On 2011/07/04 11:04:11, Erik Corry wrote:
> You are sure it is al and not eax?

Done.

http://codereview.chromium.org/7302003/diff/1/src/ia32/code-stubs-ia32.h#newc...
src/ia32/code-stubs-ia32.h:554: ASSERT(!compaction || incremental);
On 2011/07/04 11:04:11, Erik Corry wrote:
> It would be very good to assert that the old opcode was one of the ones we
> expect to see.

Done.

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking-inl.h
File src/incremental-marking-inl.h (right):

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking-inl.h#n...
src/incremental-marking-inl.h:56: // Object is not going to be rescaned we need
to record slot.
On 2011/07/04 11:04:11, Erik Corry wrote:
> rescaned -> rescanned,
> slot -> the slot

Done.

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:370: true,
On 2011/07/04 11:04:11, Erik Corry wrote:
> This constant deserves a comment or an enum type.

Done.

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:370: true,
On 2011/07/04 11:04:11, Erik Corry wrote:
> This constant deserves a comment or an enum type.

Done.

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:491: false,
On 2011/07/04 11:04:11, Erik Corry wrote:
> These constants deserve comments or enum types.

Done.

http://codereview.chromium.org/7302003/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:509: false,
On 2011/07/04 11:04:11, Erik Corry wrote:
> & here

Done.

http://codereview.chromium.org/7302003/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7302003/diff/1/src/mark-compact.cc#newcode395
src/mark-compact.cc:395: static void ClearEvacuationCandidates(PagedSpace*
space) {
On 2011/07/04 11:04:11, Erik Corry wrote:
> Commented code

Done.

http://codereview.chromium.org/7302003/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7302003/diff/1/src/objects-inl.h#newcode1121
src/objects-inl.h:1121:
value->GetHeap()->incremental_marking()->RecordWrite(this, NULL, value);
On 2011/07/04 11:04:11, Erik Corry wrote:
> This is because we don't compact the map space, right?  Comment?

Done.

http://codereview.chromium.org/7302003/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7302003/diff/1/src/spaces.cc#newcode1746
src/spaces.cc:1746: Page::FromAddress(node->address())->IsEvacuationCandidate())
{
On 2011/07/04 11:04:11, Erik Corry wrote:
> It would be nicer to not put the free list entries in the list in the first
> place.

Evacuation decision is made late.

http://codereview.chromium.org/7302003/diff/1/src/spaces.cc#newcode1808
src/spaces.cc:1808: 
On 2011/07/04 11:04:11, Erik Corry wrote:
> Missing blank line

Done.

Powered by Google App Engine