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

[Erik Österlund]

Hi Roman,

Looks good.

Thanks,
/Erik

On 2018-06-05 18:29, Roman Kennke wrote:
> 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
>