Issue 6597104: Move IncrementalRecordWrite to a stub.

Issue 6597104: Move IncrementalRecordWrite to a stub. (Closed)

Created:
9 years, 9 months ago by Vyacheslav Egorov (Chromium)

Modified:
9 years, 7 months ago

Reviewers:
Erik Corry

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/experimental/gc

Visibility:
Public.

Description

Move IncrementalRecordWrite to a stub. Patch stub to enable/disable it. Don't activate incremental marker for small heaps. BUG= TEST= Committed: http://code.google.com/p/v8/source/detail?r=7030

Patch Set 1 #

Total comments: 19

Created: 9 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+202 lines, -24 lines)			Patch
M	src/code-stubs.h	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/heap.h	View	2 chunks	+3 lines, -3 lines	2 comments	Download
M	src/heap.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	src/ia32/code-stubs-ia32.h	View	1 chunk	+46 lines, -0 lines	0 comments	Download
M	src/ia32/code-stubs-ia32.cc	View	1 chunk	+17 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1 chunk	+2 lines, -6 lines	2 comments	Download
M	src/ia32/macro-assembler-ia32.h	View	2 chunks	+19 lines, -1 line	0 comments	Download
M	src/ia32/macro-assembler-ia32.cc	View	2 chunks	+63 lines, -10 lines	5 comments	Download
M	src/ia32/stub-cache-ia32.cc	View	2 chunks	+3 lines, -2 lines	0 comments	Download
M	src/incremental-marking.h	View	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/incremental-marking.cc	View	4 chunks	+43 lines, -0 lines	10 comments	Download
M	src/mark-compact.cc	View	1 chunk	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Erik Corry

LGTM http://codereview.chromium.org/6597104/diff/1/src/heap.h File src/heap.h (right): http://codereview.chromium.org/6597104/diff/1/src/heap.h#newcode1159 src/heap.h:1159: // Returns the size of object residing in ...

9 years, 9 months ago (2011-03-02 12:34:06 UTC) #2

Erik Corry

LGTM with the comments (esp. the one about XMM registers) addressed.

9 years, 9 months ago (2011-03-02 13:10:38 UTC) #3

Vyacheslav Egorov (Chromium)

9 years, 9 months ago (2011-03-02 15:11:32 UTC) #4

http://codereview.chromium.org/6597104/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6597104/diff/1/src/heap.h#newcode1159
src/heap.h:1159: // Returns the size of object residing in non new spaces.
On 2011/03/02 12:34:06, Erik Corry wrote:
> object -> the objects

Done.

http://codereview.chromium.org/6597104/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6597104/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:1959: object, value, scratch, INLINE_SMI_CHECK,
DESTROY_OBJECT, DESTROY_VALUE, DESTROY_SCRATCH);
On 2011/03/02 12:34:06, Erik Corry wrote:
> Lint.

Done.

http://codereview.chromium.org/6597104/diff/1/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/6597104/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:86: 
On 2011/03/02 12:34:06, Erik Corry wrote:
> Extra blank line.

Done.

http://codereview.chromium.org/6597104/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:115: 
On 2011/03/02 12:34:06, Erik Corry wrote:
> Extra blank line.

Done.

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

http://codereview.chromium.org/6597104/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:130: static const intptr_t kActivationThreshold = 0;
On 2011/03/02 12:34:06, Erik Corry wrote:
> Perhaps worth setting this to some low level so that some debug tests run with
> incremental marking and some without.

For now let's always resort to incremental marking to get test coverage.
I have added todo

http://codereview.chromium.org/6597104/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:138: static void PatchStubs(bool enable) {
On 2011/03/02 12:34:06, Erik Corry wrote:
> This should be called something that reveals what sort of stubs it patches.

Done.

http://codereview.chromium.org/6597104/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:141: 
On 2011/03/02 12:34:06, Erik Corry wrote:
> Blank line.

Done.

http://codereview.chromium.org/6597104/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:152: e = JSGlobalPropertyCell::cast(e)->value();
On 2011/03/02 12:34:06, Erik Corry wrote:
> WTF?

Done.

http://codereview.chromium.org/6597104/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:157: *stub->instruction_start() = enable ? 0x90 :
0xc3;
On 2011/03/02 12:34:06, Erik Corry wrote:
> These should be constants from assembler-ia32.h 
> 
> Also you should assert that the old value was the opposite.

Done.

Expand Messages | Collapse Messages