Remove slots that point to unboxed doubles from the StoreBuffer/SlotsBuffer.

Remove slots that point to unboxed doubles from the StoreBuffer/SlotsBuffer.

The problem is that tagged slot could become a double slot after migrating of an object to another map with "shifted" fields (for example as a result of generalizing immutable data property to a data field).
This CL also adds useful machinery that helps triggering incremental write barriers.

BUG=chromium:454297
LOG=Y

Committed: https://crrev.com/9633ebabd405c264d33f603f8798c31f59418dcd
Cr-Commit-Position: refs/heads/master@{#27054}

[Igor Sheludko, 2015-02-27 10:59:38]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-02-27 10:59:46]

Patchset #1 (id:20001) has been deleted

[Igor Sheludko, 2015-02-27 11:00:49]

Patchset #1 (id:40001) has been deleted

[Igor Sheludko, 2015-02-27 15:39:22]

Patchset #2 (id:80001) has been deleted

[Igor Sheludko, 2015-02-27 15:40:39]

Patchset #2 (id:90009) has been deleted

[Igor Sheludko, 2015-02-27 21:58:16]

Patchset #2 (id:110001) has been deleted

[Igor Sheludko, 2015-02-27 22:02:38]

ishell@chromium.org changed reviewers:
+ hpayer@chromium.org, verwaest@chromium.org

[Igor Sheludko, 2015-02-27 22:02:39]

PTAL

[Hannes Payer (out of office), 2015-03-03 09:48:53]

https://codereview.chromium.org/957273002/diff/130001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4292: void SlotsBuffer::RemoveSlot(SlotsBuffer* buffer,
ObjectSlot slot_to_remove) {
I think it would be more efficient to just clear the slot instead of moving
everything to the left.

https://codereview.chromium.org/957273002/diff/130001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/spaces.h#newco...
src/heap/spaces.h:389: // FLAG_stress_compaction and
FLAG_manual_evacuation_candidates_selection
I don't think we need the FLAG_stress_compaction dependency.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:250: // Returns number of slots stored in the slots
buffer.
store buffer

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:251: int StoreBuffer::SizeForTesting() {
Why is it called SizeForTesting? Why don't call it NumberOfRecordedSlots or
something like that.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:259: void StoreBuffer::RemoveSlots(Address
start_address, Address end_address) {
How often do you think is this function called. If it is called *many* times, we
will copy many times the store buffer entries. Wouldn't it be more efficient to
invalidate the store buffer entries?

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.h...
src/heap/store-buffer.h:109: // Returns number of slots stored in the slots
buffer.
store buffer

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc#newcode2120
src/objects.cc:2120: // both StoreBuffer or respective SlotsBuffer.
"or respective" => "and"

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc#newcode2126
src/objects.cc:2126: Address end_address = obj_address +
old_map->instance_size();
Why are you removing all the pointers?

[Igor Sheludko, 2015-03-04 14:49:43]

Patchset #3 (id:150001) has been deleted

[Igor Sheludko, 2015-03-04 14:49:50]

Patchset #3 (id:170001) has been deleted

[Igor Sheludko, 2015-03-04 14:54:20]

Addressed comments.

https://codereview.chromium.org/957273002/diff/130001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4292: void SlotsBuffer::RemoveSlot(SlotsBuffer* buffer,
ObjectSlot slot_to_remove) {
On 2015/03/03 09:48:53, Hannes Payer wrote:
> I think it would be more efficient to just clear the slot instead of moving
> everything to the left.

Done.

https://codereview.chromium.org/957273002/diff/130001/src/heap/spaces.h
File src/heap/spaces.h (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/spaces.h#newco...
src/heap/spaces.h:389: // FLAG_stress_compaction and
FLAG_manual_evacuation_candidates_selection
On 2015/03/03 09:48:53, Hannes Payer wrote:
> I don't think we need the FLAG_stress_compaction dependency.

I would prefer to leave this dependency in order to reduce the number of
unnecessary checks in MarkCompactCollector::CollectEvacuationCandidates() for
production code.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:250: // Returns number of slots stored in the slots
buffer.
On 2015/03/03 09:48:53, Hannes Payer wrote:
> store buffer

Done.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:251: int StoreBuffer::SizeForTesting() {
On 2015/03/03 09:48:53, Hannes Payer wrote:
> Why is it called SizeForTesting? Why don't call it NumberOfRecordedSlots or
> something like that.

Actually it doesn't make sense. Removed.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:259: void StoreBuffer::RemoveSlots(Address
start_address, Address end_address) {
On 2015/03/03 09:48:53, Hannes Payer wrote:
> How often do you think is this function called. If it is called *many* times,
we
> will copy many times the store buffer entries. Wouldn't it be more efficient
to
> invalidate the store buffer entries?

Good point.

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.h
File src/heap/store-buffer.h (right):

https://codereview.chromium.org/957273002/diff/130001/src/heap/store-buffer.h...
src/heap/store-buffer.h:109: // Returns number of slots stored in the slots
buffer.
On 2015/03/03 09:48:53, Hannes Payer wrote:
> store buffer

Done.

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc#newcode2120
src/objects.cc:2120: // both StoreBuffer or respective SlotsBuffer.
On 2015/03/03 09:48:53, Hannes Payer wrote:
> "or respective" => "and"

Done.

https://codereview.chromium.org/957273002/diff/130001/src/objects.cc#newcode2126
src/objects.cc:2126: Address end_address = obj_address +
old_map->instance_size();
On 2015/03/03 09:48:53, Hannes Payer wrote:
> Why are you removing all the pointers?

Because in the next loop all the values will be written again and correct slots
will be added to respective store/slots buffer.

[Hannes Payer (out of office), 2015-03-05 21:34:43]

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:267: const Address kRemovedSlot = NULL;
The NULL value is not much fun. Maybe we should point it to an address in old
space that does not move. Then we do not have to guard everywhere against NULL
pointer.

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:287: old_top_ = new_top;
Why do you set old_top_ here, the old top should remain unchanged because upper
!= old_top_

[Igor Sheludko, 2015-03-06 09:05:52]

Patchset #4 (id:210001) has been deleted

[Igor Sheludko, 2015-03-06 09:06:59]

Addressed comments

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.cc
File src/heap/store-buffer.cc (right):

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:267: const Address kRemovedSlot = NULL;
On 2015/03/05 21:34:43, Hannes Payer wrote:
> The NULL value is not much fun. Maybe we should point it to an address in old
> space that does not move. Then we do not have to guard everywhere against NULL
> pointer.

Good point!

https://codereview.chromium.org/957273002/diff/190001/src/heap/store-buffer.c...
src/heap/store-buffer.cc:287: old_top_ = new_top;
On 2015/03/05 21:34:43, Hannes Payer wrote:
> Why do you set old_top_ here, the old top should remain unchanged because
upper
> != old_top_

Sorry, I didn't get the question. Added more comments to sources.

[Hannes Payer (out of office), 2015-03-06 21:33:59]

LGTM. I am looking forward to have a new store buffer implementation that avoids
complicated code like that.

[Igor Sheludko, 2015-03-06 22:39:24]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-03-06 22:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957273002/230001

[commit-bot: I haz the power, 2015-03-06 23:55:05]

Committed patchset #4 (id:230001)

[commit-bot: I haz the power, 2015-03-06 23:55:28]

Patchset 4 (id:??) landed as
https://crrev.com/9633ebabd405c264d33f603f8798c31f59418dcd
Cr-Commit-Position: refs/heads/master@{#27054}

[Igor Sheludko, 2015-03-09 10:01:46]

A revert of this CL (patchset #4 id:230001) has been created in
https://codereview.chromium.org/986283002/ by ishell@chromium.org.

The reason for reverting is: It caused a lot of Canary crashes..