RFR(M): 8190430 [MVT] Fixing GC support for Thread-local value buffers

[bdelsart work]

Hi Fred,

A few other comments

- interpreterRuntime.cpp:268 (and probably others)

In the long term (possibly a future cleanup), it would be better not to add
UseG1GC tests and explicit barriers like this one all over the non GC code.
This should use an oop_store method compatible with any GC or an API
defined in barrierSet.hpp. I suppose you cannot use the current oop_store
method because it does not like the fact that the address where the oop is
stored in not in the heap. If you do not want to extend oop_store and
related methods so that they work when the location is not in the heap
(this would be simpler but may have a performance impact), you may need to
write non-heap variants of oop_store, obj_put_field, ... or to expose
methods like BarrierSet::write_ref_field_in_non_heap_buffer[_pre] or
VTBuffer::write_ref_field[_pre]

However, this may be a non-issue because my other concern is that I am not
sure of whether you really need to apply the barriers when writing oops in
a vtBuffer. If the vtBuffers are parsed while parsing the Thread frames
(during the stop the word phase), this should not be required. Are we
expecting the vtBuffer space to be so big that we need to parse them
concurrently ? In fact, I have some doubts about the fact that you can
indeed safely parse the vtBuffers when the mutators are running and
popping/reusing vtBuffers. Hence, I really think a lot of this marking code
is useless.

- Similar issue about whether we really need to enqueue values in
ValueKlass::value_store
when the destination is in a vtBuffer. This is needed only if the parsing
of the vtBuffers is not done while parsing the frames. I did not check
whether this is the case... but I doubt it.

One of the biggest advantage of using non-heap values should be to avoid
all the barrier overhead.

- Finally, I do not really like the UseG1GC check and
the G1SATBCardTableModRefBS::enqueue call in VTBuffer::allocate_value but
it looks like this is already done that way in a lot of places which should
be G1 agnostic.

I recommend at least to add the missing #if INCLUDE_ALL_GCS check. In the
long term, the GC team should consider cleaning up all the
G1SATBCardTableModRefBS::enqueue calls, using a generic GC API which would
work for other concurrent GCs (this is outside the scope of the valhalla
repositories).

Now, I also have some doubts wbout whether the enqueuing is needed here but
I am less sure that for the previous ones. This may have to be double
checked with the GC team since the klass will be discovered when ârsing the
stack frames, at a stop the world phase.

- Remaining looks good (except for the markbits stuff reported in my
previous mail)

Regards,

Bertrand



--
Freelance - Bertrand Delsart Software Solutions
Remote Research, Development and Troubleshooting
JVM, Real-Time and Concurrency expert
http://www.bdelsart.com


2017-11-17 17:43 GMT+01:00 bdelsart work <bdelsart.work at gmail.com>:

> Hi Fred,
>
> I started to look at the webrev and will try to investigate more deeply
> but I did notice something strange in dealiser marking. Here is a quick
> feedback in case this is currently leading to issues in more complex tests.
>
> It looks like you set set the new mark with just an '|' on the markbits in
> BufferedValuesDealiaser::oops_do. If I understand correctly, the expected
> marks are switched regularly and hence the initial state of the VTBuffer
> mask part may not always be 0. If this is the case, the proposed code may
> not always set the mark to what you expected.
>
> You should probably clear the VTBuffer::mark_mask part from value->mark() before
> the | in
>
> +  intptr_t new_mark_word = ((intptr_t) (oopDesc*)(value->mark()))+              | (intptr_t)_current_mark;
>
> Alternatively, the marks could be chosen so that a simple XOR works (for
> instance by using only one bit for this marking)
>
> To double check, you can easily add an assert at the end of
> BufferedValuesDealiaser::oops_do to check that indeed your filtering
> condition is now true, e.g. that (new_mark_word & VTBuffer::mark_mask) ==
> _current_mark
>
> Regards
>
> Bertrand
>
> --
> Freelance - Bertrand Delsart Software Solutions
> Remote Research, Development and Troubleshooting
> JVM, Real-Time and Concurrency expert
> http://www.bdelsart.com
>
>
> 2017-11-08 20:39 GMT+01:00 Frederic Parain <frederic.parain at oracle.com>:
>
>> Updated webrev where the memory allocation for the TLVB is changed from
>> on-demand mmap() calls to a reserve/lazy commits mechanism.
>>
>> http://cr.openjdk.java.net/~fparain/8190430/webrev.01/
>>
>> Fred
>>
>>
>> > On Oct 31, 2017, at 13:21, Frederic Parain <frederic.parain at oracle.com>
>> wrote:
>> >
>> > Please review this changeset fixing the GC support and other
>> > issues with the Thread-Local Value Buffer:
>> >
>> > http://cr.openjdk.java.net/~fparain/8190430/webrev.00/
>> >
>> > This changeset re-activates the TLVB by default.
>> > The VTBuffer test has been fixed to generate value types
>> > with object references.
>> >
>> > All hotspot_valhalla tests pass.
>> >
>> > Thank you,
>> >
>> > Fred
>>
>>
>