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

[Frederic Parain]

Bertrand,

You are not the only one who spotted the issue about these barriers.
We had a metting the GC team last Wednesday about them, and we
agreed that these barriers were not needed.
I’ve already removed them, and re-ran the tests without noticing any
failure.

I’ll post a new webrev soon.

Thank you,

Fred


> On Nov 17, 2017, at 14:07, bdelsart work <bdelsart.work at gmail.com> wrote:
> 
> 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
> 
> 
>