RFR: JDK-8200623: Primitive heap access for interpreter BarrierSetAssembler/x86

[Roman Kennke]

Am 05.06.2018 um 17:33 schrieb Erik Österlund:
> Hi Roman,
> 
> On 2018-06-05 16:07, Roman Kennke wrote:
>> Hi Erik,
>>
>>>> JDK-8199417 added better modularization for interpreter barriers.
>>>> Shenandoah and possibly future GCs also need barriers for primitive
>>>> access.
>>>>
>>>> Some notes on implementation:
>>>> - float/double/long access produced some headaches for the following
>>>> reasons:
>>>>
>>>>     - float and double would either take XMMRegister which is not
>>>> compatible with Register
>>>>     - or load-from/store-to the floating point stack (see
>>>> MacroAssembler::load/store_float/double)
>>>>     - long access on x86_32 would load-into/store-from 2 registers, or
>>>> else use a trick via the floating point stack to do atomic access
>>>>
>>>> None of this seemed easy/nice to do with the API. I helped myself by
>>>> accepting noreg as dst/src argument, which means the corresponding tos
>>>> (i.e. ltos, ftos, dtos) and the BSA would then access from/to
>>>> xmm0/float-stack in case of float/double or the double-reg/float-stack
>>>> in case of long/32bit, which is all that we ever need.
>>> It is indeed a bit painful that in hotspot, XMMRegister is not a
>>> Register (unlike the Graal implementation). And I think I agree that if
>>> it is indeed only ever needed by ToS, then this is the preferable
>>> solution to having two almost identicaly APIs - one for integral types
>>> and one for floating point types. It beats me though, that in this patch
>>> you do not address the jni fast get field optimization on x86. It is
>>> seemingly missing barriers now. Should probably make sure that one fits
>>> in as well. Fortunately, I think it should work out pretty well.
>> As mentioned in the review thread for JDK-8203172, we in Shenandoah land
>> decided to disable JNI fastgetfield stuff for now. I am not sure whether
>> or not we want to go through access_* anyway? It's probably more
>> consistent if we do. If we decide that we do, I'll add it to this patch,
>> if we don't, I'll rip it out of JDK-8203172 :-)
> 
> Okay so let's not deal with JNI fast get field in either of those these
> two patches, and leave that exercise to future adventurers that feel
> brave enough to change that code. If you decide later to add
> modularization for that to enable this fantastic performance
> optimization on Shenandoah, then perhaps we can have a new patch with
> the appropriate code (possibly the speculative PC range filter I
> proposed) then in the new modularization.
> 
> In that case, it looks reasonable the way it is now. Perhaps there
> should be a T_ADDRESS case for stores for consistency though? You can
> now load a T_ADDRESS but not store it, which is a bit surprising I
> suppose. Otherwise it looks good.
> 

Ok I've added it:

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8200623/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8200623/webrev.01/

Good now?

Thanks, Roman