RFR (M): 8188764: Obsolete AssumeMP and then remove all support for non-MP builds

Sun Sep 23 20:19:27 UTC 2018

Hi David,

Before I forget about it...
In src/hotspot/cpu/x86/jniFastGetField_x86_64.cpp

Note that the is_MP() check was used to conditionally poke in an xor 
trick to create a data dependency to the safepoint counter. You removed 
the not is_MP() path with this patch. However, note that this use of 
forced data dependency to elide the need for an acquire or loadload 
fence is nonsense on x86 hardware, as loadload simply does not reorder. 
If you look at the corresponding SPARC code, you will find that there is 
no such confused xor trick, and rightfully so.

So while said xor trick is simply not needed, it isn't incorrect to do it.

I'll leave it up to you if you want to fix this in this change or not. 
But that xor trick is confused and should IMO be removed. It makes 
readers think it's there for a reason. But the assumption that it is 
needed when is_MP() is wrong.

Thanks,
/Erik

On 2018-09-23 18:26, David Holmes wrote:
> Hi Boris,
>
> Here is latest webrev with the ARM code updated as well.
>
> http://cr.openjdk.java.net/~dholmes/8188764/webrev/
>
> All is_MP removed except where needed for ARMv5 support, MP specific 
> instructions, and controlling of biased locking.
>
> I checked the DMB variants and we only generate for ARMv7 anyway, so 
> no issue there.
>
> Would very much appreciate whatever test builds you can do on 
> different ARM variants Boris!
>
> Thanks,
> David
>
> On 21/09/2018 9:41 AM, David Holmes wrote:
>> Hi Boris,
>>
>> On 21/09/2018 9:34 AM, Boris Ulasevich wrote:
>>> Hi David,
>>>
>>> On 20.09.2018 18:26, David Holmes wrote:
>>>> Hi Boris,
>>>>
>>>> Thanks for jumping on this. I hope you didn't spend too much time 
>>>> on it as I had actually started on the ARM code then decided I 
>>>> didn't need to make the changes, so I'd already gone through pretty 
>>>> much everything that is needed. My concern is with things like the 
>>>> change in
>>>>
>>>> src/hotspot/cpu/arm/assembler_arm_32.hpp
>>>>
>>>> where you removed the is_MP() check but in fact that needs to 
>>>> remain because the instruction pldw is not present on variants of 
>>>> the v7 architecture without the multi-processor extensions:
>>>
>>> Ok. Good point. Sure, my hasty proposal to get rid of is_MP() check 
>>> in arm32 codes was wrong.
>>>
>>> One note. I see there is an ambiguity here: is_MP() is a 
>>> multiprocessor system flag (returns true when _processor_count != 
>>> 1), but multi-processor extensions is an optional ARMv7 feature 
>>> which is not related with processors count, so pldw usage should not 
>>> be controlled by is_MP() flag.
>>
>> is_MP can't return true unless there is more than one processor 
>> (ignoring the pre-initialization case), so if there's more than one 
>> processor there must be a MP supprting architecture.
>>
>>>> http://www.keil.com/support/man/docs/armclang_asm/armclang_asm_pge1425899018492.htm 
>>>>
>>>> That makes me wonder whether any of the memory barrier instructions 
>>>> may also be missing in some v6/v7 variants as well? 
>>>
>>> Nobody complained :) And Reference Manual says DMB/DSB are not 
>>> optional.
>>
>> I need to double-check we don't define variants like "dmb sy" that 
>> are architecture specific.
>>
>>>> Plus I have to account for uniprocessor ARMv5 so need to see 
>>>> whether MacroAssembler::membar, for example, must also retain the 
>>>> is_MP() check.
>>>
>>> Do you want to delete this check? Processor count check seems quite 
>>> natural to see if it has sense to generate memory barriers.
>>
>> That is the general thrust of this change. We assume we always have 
>> MP and so always issue memory barriers.
>>
>>> By the way, MacroAssembler::fast_lock have 
>>> VM_Version::supports_ldrex() check assertion with message 
>>> "unsupported, yet?". Looks like it is not going to work correctly on 
>>> multiprocessor ARMv5.
>>
>> The ARM code does not support multiprocessor ARMv5.
>>
>> Thanks,
>> David
>>
>>>> Also in:
>>>>
>>>> src/hotspot/cpu/arm/stubGenerator_arm.cpp
>>>>
>>>> you can't remove the !is_MP() check and related code as that is 
>>>> needed for ARMv5 (uniprocessor) support.
>>>>
>>>> Similarly in
>>>>
>>>> src/hotspot/cpu/arm/vm_version_arm_32.cpp
>>>>
>>>> you'll probably still want to disable biased-locking for ARMv5.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>> On 20/09/2018 3:31 AM, Boris Ulasevich wrote:
>>>>> Hi David,
>>>>>
>>>>>  > I have not updated the about-to-be-removed Oracle ARM port code.
>>>>>
>>>>> Ok, but we can do the changes in ARM32 codes leaving ARM64 intact.
>>>>> I would propose the following change - I think we can either add 
>>>>> it to your update or to create a separate RFR after your patch and 
>>>>> JEP-340 commit:
>>>>>
>>>>> http://cr.openjdk.java.net/~bulasevich/8188764/webrev.00/
>>>>>
>>>>> regards,
>>>>> Boris
>>>>>
>>>>> On 18.09.2018 08:32, David Holmes wrote:
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8188764
>>>>>> Webrev: http://cr.openjdk.java.net/~dholmes/8188764/webrev/
>>>>>>
>>>>>> As previously discussed in the RFC thread:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-September/034166.html 
>>>>>>
>>>>>>
>>>>>> this change obsoletes the AssumeMP flag and removes the bulk of 
>>>>>> the logic that is conditional on os::is_MP() as follows:
>>>>>>
>>>>>> 1. to gate the introduction of MP-specific features, notably 
>>>>>> memory barriers
>>>>>>
>>>>>> The is_MP check is removed and we will always issue memory barriers
>>>>>> or pre-pend lock prefix etc.
>>>>>>
>>>>>> 2. to align certain patchable code sequences (method entry, call 
>>>>>> sites)
>>>>>>
>>>>>> The is_MP is removed and we always align patchable locations.
>>>>>>
>>>>>> 3. to gate certain optimizations which are questionable on 
>>>>>> uniprocessors
>>>>>> (see quicken_jni_functions)
>>>>>>
>>>>>> These are psuedo-memory-barriers where we manufacture a 
>>>>>> data-dependency
>>>>>> instead of inserting mfence/lfence (x86 example). These are 
>>>>>> treated as
>>>>>> #1 and is_MP is removed.
>>>>>>
>>>>>> 4. to gate optimizations which are desirable on uniprocessors
>>>>>> (mutex/monitor short circuits)
>>>>>>
>>>>>> These are spin controls and so is_MP remains.
>>>>>>
>>>>>> I have not updated the about-to-be-removed Oracle ARM port code.
>>>>>>
>>>>>> Testing:
>>>>>>    - Tiers 1 -3 (mach5)
>>>>>>
>>>>>> Performance run TBD.
>>>>>>
>>>>>> Thanks,
>>>>>> David