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

[Frederic Parain]

Bertrand,

Thank you for your review. A few comments inlined below.

On 11/23/2017 04:48 AM, bdelsart work wrote:
> Hi Fred,
> 
> One small detail (no worth a rereview), this is no longer necessary in 
> valueKlass.cpp;
> 
> 39 #if INCLUDE_ALL_GCS
> 40 #include "gc/g1/g1SATBCardTableModRefBS.hpp"
> 41 #endif // INCLUDE_ALL_GCS

Removed.

> Remaining looks good. Maybe suboptimal but safer for now
> 
> I for instance still have strong doubts about the need to enqueue the 
> mirror for G1 in VTBuffer::allocate_value (since the VTBuffers currently 
> act exactly like the thread stacks and are parsed only during safepoint).

I missed this one. This code is unnecessary for the same reasons as the
other GC barriers I've already removed. I've removed it and re-ran the
tests without issues.

Updated webrev:

http://cr.openjdk.java.net/~fparain/8190430/webrev.03/index.html

> [ I also had some doubts  about the need in vwithfield to systematically 
> copy in the heap a vtBuffer allocated value 'voop' when stored in a non 
> flattened field in a 'new_value' since voop is always older than 
> new_value. However, this is really a design choice. Allowing a non 
> flattened field to point to an older value in a vtBuffer would require 
> to be more careful for instance when walking the value or copying it on 
> frame return and may not be worth the added complexity (particularly for 
> the copy on frame return). Hence, the proposed design where a non 
> flattened field always points towards the heap may indeed be better ]

The problem is that writing a value to a field is equivalent to
publishing it, it is not thread-local anymore because any thread
can access it. We want to keep the management of the TLVB local
to its thread, so to prevent threads to refer to values buffered
in other threads' TLVBs, we have to make this copy then writing
to a field or an array.

Best regards,

Fred


> 
> --
> Freelance - Bertrand Delsart Software Solutions
> Remote Research, Development and Troubleshooting
> JVM, Real-Time and Concurrency expert
> http://www.bdelsart.com <http://www.bdelsart.com/>
> 
> 
> 2017-11-22 20:22 GMT+01:00 Frederic Parain <frederic.parain at oracle.com 
> <mailto:frederic.parain at oracle.com>>:
> 
>     Here’s an updated webrev with unnecessary GC barriers removed,
>     and an assertion added to check the consistency of the mark bits
>     of buffered values.
> 
>     http://cr.openjdk.java.net/~fparain/8190430/webrev.02/index.html
>     <http://cr.openjdk.java.net/~fparain/8190430/webrev.02/index.html>
> 
>     Fred
> 
> 
>      > On Nov 8, 2017, at 14:39, Frederic Parain
>     <frederic.parain at oracle.com <mailto:frederic.parain at oracle.com>> wrote:
>      >
>      > 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/
>     <http://cr.openjdk.java.net/~fparain/8190430/webrev.01/>
>      >
>      > Fred
>      >
>      >
>      >> On Oct 31, 2017, at 13:21, Frederic Parain
>     <frederic.parain at oracle.com <mailto: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/
>     <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
>      >
> 
>