Combine the incremental-marking write barrier and the remembered-set...

Combine the incremental-marking write barrier and the remembered-set
write barrier into one stub.

Committed: http://code.google.com/p/v8/source/detail?r=7514

[Vyacheslav Egorov (Chromium), 2011-04-06 11:46:40]

LGTM with comments addressed

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6476: // the object we are writing into in the
'address' register.  We insert a
we have the object in the object register.
I guess we have something different in the address register. Like slot offset.

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/codegen-ia32.cc#newcod...
src/ia32/codegen-ia32.cc:9798: __ RememberedSetHelper(receiver.reg(),
this place should have an incremental write barrier. why did you remove it?

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2056: __ RecordWrite(object, address, value,
OMIT_REMEMBERED_SET, kSaveFPRegs);
mixing two constant naming styles in a single call. maybe we should choose one?

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:67: preserve[object.code()] = true;
Is there any particular reason for modes removal? This will probably make call
prologue bigger in some cases.

http://codereview.chromium.org/6794052/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:176: if (emit_remembered_set ==
OMIT_REMEMBERED_SET &&
Maybe add an assertion that during snapshot generation we don't omit any write
barriers.

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:1502: EMIT_REMEMBERED_SET, kDontSaveFPRegs,
OMIT_SMI_CHECK);
strange formatting

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:1545: // write too.
this was not a record write for hole, it's record write for a value we push on
line 1535

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

http://codereview.chromium.org/6794052/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:172: CodeStub::IncrementalMarkingRecordWrite) {
maybe change keyname as well?

[Erik Corry, 2011-04-06 12:31:28]

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6476: // the object we are writing into in the
'address' register.  We insert a
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> we have the object in the object register.
> I guess we have something different in the address register. Like slot offset.

Done.

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/codegen-ia32.cc#newcod...
src/ia32/codegen-ia32.cc:9798: __ RememberedSetHelper(receiver.reg(),
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> this place should have an incremental write barrier. why did you remove it?

This whole file is on death row.  Comment added.  You will note that I did not
remove it in this change.

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:2056: __ RecordWrite(object, address, value,
OMIT_REMEMBERED_SET, kSaveFPRegs);
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> mixing two constant naming styles in a single call. maybe we should choose
one?

Yes.  Postponed.

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

http://codereview.chromium.org/6794052/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:67: preserve[object.code()] = true;
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> Is there any particular reason for modes removal? This will probably make call
> prologue bigger in some cases.

We can't splat the registers if they are going to be used later for the
remembered set write barrier.  This will be further rationalized in the next
change.

http://codereview.chromium.org/6794052/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:176: if (emit_remembered_set ==
OMIT_REMEMBERED_SET &&
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> Maybe add an assertion that during snapshot generation we don't omit any write
> barriers.

We omit the remembered set when writing into global variable cells.  These are
always scanned.  We can do this when generating snapshots too.

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:1502: EMIT_REMEMBERED_SET, kDontSaveFPRegs,
OMIT_SMI_CHECK);
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> strange formatting

Done.

http://codereview.chromium.org/6794052/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:1545: // write too.
On 2011/04/06 11:46:40, Vyacheslav Egorov wrote:
> this was not a record write for hole, it's record write for a value we push on
> line 1535 

Oops.  Nice catch.  Fixed.  And comment added.  This a tricky bit of code.