Add map-based read barrier to WeakCell

Add map-based read barrier to WeakCell

R=hpayer@chromium.org, ulan@chromium.org
BUG=

[Erik Corry Chromium.org, 2015-02-03 16:12:55]

This is a slightly different approach to
https://codereview.chromium.org/892843002/
Instead of mistagging, we change the map.  This makes it easier to ignore the
read barrier in cases where we are just doing a compare and don't want to
trigger it.

It's only IA32, as a proof of concept.

[Erik Corry Chromium.org, 2015-02-03 17:42:53]

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6220: static HObjectAccess ForWeakCellValue() {
I suspect some or perhaps all of the uses of this are for checks and should not
trigger the read barrier.

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
File src/ic/ia32/ic-compiler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
Map::WeakCellForMap(transitioned_maps->at(i));
I think perhaps here we should trigger the read barrier on the first cell (yeah
there are two variables called cell in this function!) to show that it is in use
and should not be cleared on GC. What do you think?

[ulan, 2015-02-04 09:35:04]

Looks good. Updating the map could race with the sweeper thread. Shouldn't the
barrier check the state of incremental marker?

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6220: static HObjectAccess ForWeakCellValue() {
On 2015/02/03 17:42:53, Erik Corry Chromium.org wrote:
> I suspect some or perhaps all of the uses of this are for checks and should
not
> trigger the read barrier.

Yesterday I landed a change in StoreGlobal hydrogen stub that loads and uses
weak value.

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:2995: __ mov(result, FieldOperand(object,
HeapObject::kMapOffset));
__ mov(FieldOperand(object, HeapObject::kMapOffset), result);

https://codereview.chromium.org/893073006/diff/1/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:2432: mov(scratch, FieldOperand(value,
HeapObject::kMapOffset));
mov(FieldOperand(value, HeapObject::kMapOffset), scratch); ?

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
File src/ic/ia32/ic-compiler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
Map::WeakCellForMap(transitioned_maps->at(i));
On 2015/02/03 17:42:53, Erik Corry Chromium.org wrote:
> I think perhaps here we should trigger the read barrier on the first cell
(yeah
> there are two variables called cell in this function!) to show that it is in
use
> and should not be cleared on GC. What do you think?

Do you mean read barrier on receiver_map? I think we don't need it. If the
compare succeeds than we already have the receiver map in register, so it must
be alive. If the compare fails, then we have no information and we didn't "use"
the map.

[Erik Corry, 2015-02-04 10:49:41]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2015-02-04 10:49:42]

Thanks for review.

The two different WeakCell maps are identical from the point of view of the
sweeper, so I think the data race makes no difference.  If we have to check the
state of the sweeper, then that would make this approach much less attractive.

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6220: static HObjectAccess ForWeakCellValue() {
On 2015/02/04 09:35:04, ulan wrote:
> On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > I suspect some or perhaps all of the uses of this are for checks and should
> not
> > trigger the read barrier.
> 
> Yesterday I landed a change in StoreGlobal hydrogen stub that loads and uses
> weak value.

OK, we probably need both.

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:2995: __ mov(result, FieldOperand(object,
HeapObject::kMapOffset));
On 2015/02/04 09:35:04, ulan wrote:
> __ mov(FieldOperand(object, HeapObject::kMapOffset), result);

This is intended to be a store, not a load.

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
File src/ic/ia32/ic-compiler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
Map::WeakCellForMap(transitioned_maps->at(i));
On 2015/02/04 09:35:04, ulan wrote:
> On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > I think perhaps here we should trigger the read barrier on the first cell
> (yeah
> > there are two variables called cell in this function!) to show that it is in
> use
> > and should not be cleared on GC. What do you think?
> 
> Do you mean read barrier on receiver_map? I think we don't need it. If the
> compare succeeds than we already have the receiver map in register, so it must
> be alive. If the compare fails, then we have no information and we didn't
"use"
> the map.

I agree we don't need it for correctness.  The question is whether it could be
useful for tracking whether the map was recently used, so that we can extend its
life in GC even if there are no live objects using it at the next GC.  But I'm
not exactly sure what's going on here: Does this count as a recent use that
should prevent weak map clearing at the next GC?

[ulan, 2015-02-04 11:08:10]

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:2995: __ mov(result, FieldOperand(object,
HeapObject::kMapOffset));
On 2015/02/04 10:49:42, Erik Corry wrote:
> On 2015/02/04 09:35:04, ulan wrote:
> > __ mov(FieldOperand(object, HeapObject::kMapOffset), result);
> 
> This is intended to be a store, not a load.

Yep. The destination of the mov is the first argument :)
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ia32/assemb...

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
File src/ic/ia32/ic-compiler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
Map::WeakCellForMap(transitioned_maps->at(i));
On 2015/02/04 10:49:42, Erik Corry wrote:
> On 2015/02/04 09:35:04, ulan wrote:
> > On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > > I think perhaps here we should trigger the read barrier on the first cell
> > (yeah
> > > there are two variables called cell in this function!) to show that it is
in
> > use
> > > and should not be cleared on GC. What do you think?
> > 
> > Do you mean read barrier on receiver_map? I think we don't need it. If the
> > compare succeeds than we already have the receiver map in register, so it
must
> > be alive. If the compare fails, then we have no information and we didn't
> "use"
> > the map.
> 
> I agree we don't need it for correctness.  The question is whether it could be
> useful for tracking whether the map was recently used, so that we can extend
its
> life in GC even if there are no live objects using it at the next GC.  But I'm
> not exactly sure what's going on here: Does this count as a recent use that
> should prevent weak map clearing at the next GC?

The transitioned_map[i] has a back pointer to the current map (which is equal to
the receiver_map[i] since the map check succeeded). So I think the read barrier
for transitioned_map[i] would keep also receiver_map[i] alive.

[Erik Corry, 2015-02-04 12:15:44]

On 2015/02/04 11:08:10, ulan wrote:
>
https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
> File src/ia32/lithium-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
> src/ia32/lithium-codegen-ia32.cc:2995: __ mov(result, FieldOperand(object,
> HeapObject::kMapOffset));
> On 2015/02/04 10:49:42, Erik Corry wrote:
> > On 2015/02/04 09:35:04, ulan wrote:
> > > __ mov(FieldOperand(object, HeapObject::kMapOffset), result);
> > 
> > This is intended to be a store, not a load.
> 
> Yep. The destination of the mov is the first argument :)

Doh!

>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/ia32/assemb...
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
> File src/ic/ia32/ic-compiler-ia32.cc (right):
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
> src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
> Map::WeakCellForMap(transitioned_maps->at(i));
> On 2015/02/04 10:49:42, Erik Corry wrote:
> > On 2015/02/04 09:35:04, ulan wrote:
> > > On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > > > I think perhaps here we should trigger the read barrier on the first
cell
> > > (yeah
> > > > there are two variables called cell in this function!) to show that it
is
> in
> > > use
> > > > and should not be cleared on GC. What do you think?
> > > 
> > > Do you mean read barrier on receiver_map? I think we don't need it. If the
> > > compare succeeds than we already have the receiver map in register, so it
> must
> > > be alive. If the compare fails, then we have no information and we didn't
> > "use"
> > > the map.
> > 
> > I agree we don't need it for correctness.  The question is whether it could
be
> > useful for tracking whether the map was recently used, so that we can extend
> its
> > life in GC even if there are no live objects using it at the next GC.  But
I'm
> > not exactly sure what's going on here: Does this count as a recent use that
> > should prevent weak map clearing at the next GC?
> 
> The transitioned_map[i] has a back pointer to the current map (which is equal
to
> the receiver_map[i] since the map check succeeded). So I think the read
barrier
> for transitioned_map[i] would keep also receiver_map[i] alive.

The back pointers are sometimes cleared, though.

I think ideally all the transition pointers (forward and back) would be weak,
and using them like this would mark them used.  On every GC they would be zapped
(or aged and conditionally zapped) if they were unused.  I'm not even sure we
need the back pointers in that case.  I think they are there to stop
intermediate maps that are part of heavily used transition chains from being
collected, but having use-detection would achieve that more directly, and would
track what we really need to know: "Was the transition chain used?" rather than
"Is there still a long lived object that maybe used the transition a long time
ago?".

The receiver map will not necessarily keep anything alive since that object
could be long dead by the next full GC.

[Hannes Payer (out of office), 2015-02-04 12:25:01]

On 2015/02/04 10:49:42, Erik Corry wrote:
> Thanks for review.
> 
> The two different WeakCell maps are identical from the point of view of the
> sweeper, so I think the data race makes no difference.  If we have to check
the
> state of the sweeper, then that would make this approach much less attractive.
> 
> https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h#...
> src/hydrogen-instructions.h:6220: static HObjectAccess ForWeakCellValue() {
> On 2015/02/04 09:35:04, ulan wrote:
> > On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > > I suspect some or perhaps all of the uses of this are for checks and
should
> > not
> > > trigger the read barrier.
> > 
> > Yesterday I landed a change in StoreGlobal hydrogen stub that loads and uses
> > weak value.
> 
> OK, we probably need both.
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
> File src/ia32/lithium-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ia32/lithium-codegen-ia3...
> src/ia32/lithium-codegen-ia32.cc:2995: __ mov(result, FieldOperand(object,
> HeapObject::kMapOffset));
> On 2015/02/04 09:35:04, ulan wrote:
> > __ mov(FieldOperand(object, HeapObject::kMapOffset), result);
> 
> This is intended to be a store, not a load.
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32.cc
> File src/ic/ia32/ic-compiler-ia32.cc (right):
> 
>
https://codereview.chromium.org/893073006/diff/1/src/ic/ia32/ic-compiler-ia32...
> src/ic/ia32/ic-compiler-ia32.cc:117: Handle<WeakCell> cell =
> Map::WeakCellForMap(transitioned_maps->at(i));
> On 2015/02/04 09:35:04, ulan wrote:
> > On 2015/02/03 17:42:53, Erik Corry http://Chromium.org wrote:
> > > I think perhaps here we should trigger the read barrier on the first cell
> > (yeah
> > > there are two variables called cell in this function!) to show that it is
in
> > use
> > > and should not be cleared on GC. What do you think?
> > 
> > Do you mean read barrier on receiver_map? I think we don't need it. If the
> > compare succeeds than we already have the receiver map in register, so it
must
> > be alive. If the compare fails, then we have no information and we didn't
> "use"
> > the map.
> 
> I agree we don't need it for correctness.  The question is whether it could be
> useful for tracking whether the map was recently used, so that we can extend
its
> life in GC even if there are no live objects using it at the next GC.  But I'm
> not exactly sure what's going on here: Does this count as a recent use that
> should prevent weak map clearing at the next GC?

It is not an issue for the sweeper thread, since the size of the different
WeakCells is the same.

[Hannes Payer (out of office), 2015-02-04 12:45:59]

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/893073006/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6404: ReadBarrierField::encode(read_barrier ? 1 : 0)
|
I like that.

https://codereview.chromium.org/893073006/diff/1/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/893073006/diff/1/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:2429: // indicates the cell has been read from.
This seems to be more expensive than just clearing a bit. The advantage is, that
we do not slow down the case when we can go directly to the value (omit read
barrier). I guess we have to measure the performance impact.