RFR: 8230203: Replace markWord enums with uintptr_t constants

[Erik Osterlund]

Hi,

>> On 27 Aug 2019, at 22:21, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>> 
>> On 2019-08-27 16:37, Doerr, Martin wrote:
>> Hi Stefan,
>> 
>>>> I think it would be better to use C++ 2011 Strongly-typed Enums.
>>> Could you elaborate why it would be better to use them in this case?
>> IMHO enums are better to read and edit. I don't like "static const uintptr_t" replicated 100 times.
> 
> OK. Point taken.
> 
>> In addition, static consts usually spam the binaries with stuff which is never needed while enums are only for the compiler.
> 
> I thought that wasn't a problem unless you, for example, tried to take/use the address of the constant.

According to the C++03 standard, storage for static consts must be allocated always. But AFAIK, no compiler actually does that unless it is ODR-used. Subsequent standard updates removed that annoying statement, and changed it to basically say only ODR-used constants need storage.

So I think in practice we get the wanted behaviour already, and after C++14, that will also be well specified, withot requiring us to change any code.

> Are you so opposed to this change that you think we should not do it? I'd also like to hear feedback regarding this from other reviewers.

I think going ahead with your suggested changes is fine.

Thanks,
/Erik

> Thanks,
> StefanK
>> 
>> I don't understand what keeps us from targeting JEP 347 to jdk14.
>> 
>> Best regards,
>> Martin
>> 
>> 
>>> -----Original Message-----
>>> From: Stefan Karlsson <stefan.karlsson at oracle.com>
>>> Sent: Dienstag, 27. August 2019 13:08
>>> To: Doerr, Martin <martin.doerr at sap.com>; David Holmes
>>> <david.holmes at oracle.com>; hotspot-dev <hotspot-
>>> dev at openjdk.java.net>; Kim Barrett <kim.barrett at oracle.com>
>>> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
>>> constants
>>> 
>>> Hi Martin,
>>> 
>>>> On 2019-08-27 10:22, Doerr, Martin wrote:
>>>> Hi,
>>>> 
>>>> I think it would be better to use C++ 2011 Strongly-typed Enums.
>>> Could you elaborate why it would be better to use them in this case?
>>> 
>>> AFAICT these enums where only enums because that used to be the way
>>> constants were written in HotSpot. There are some comments around this in:
>>> https://bugs.openjdk.java.net/browse/JDK-8153116
>>> 
>>>   Can we still not use them?
>>>> AFAIK all compilers which are used for jdk/jdk support that.
>>> I don't think we can go there until we get:
>>> https://openjdk.java.net/jeps/347 - JEP 347: Adopt C++14 Language
>>> Features in HotSpot
>>> 
>>> Thanks,
>>> StefanK
>>> 
>>>> Best regards,
>>>> Martin
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf
>>> Of
>>>>> David Holmes
>>>>> Sent: Dienstag, 27. August 2019 10:02
>>>>> To: Stefan Karlsson <stefan.karlsson at oracle.com>; hotspot-dev
>>> <hotspot-
>>>>> dev at openjdk.java.net>
>>>>> Subject: Re: RFR: 8230203: Replace markWord enums with uintptr_t
>>>>> constants
>>>>> 
>>>>> Hi Stefan,
>>>>> 
>>>>>> On 27/08/2019 5:32 pm, Stefan Karlsson wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> Please review this patch to convert the enum constants in markWord
>>> into
>>>>>> static const unintptr_t constants.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~stefank/8230203/webrev.01/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8230203
>>>>> Overall this seems like a good cleanup, but the introduced "int" casts
>>>>> now jump out at me. In:
>>>>> 
>>>>>    src/hotspot/cpu/aarch64/aarch64.ad
>>>>>    src/hotspot/cpu/arm/macroAssembler_arm.cpp
>>>>>    src/hotspot/cpu/ppc/macroAssembler_ppc.cpp
>>>>> 
>>>>> it would seem to me that all the "add -(int)markWord::monitor_value"
>>>>> would be cleaner/clearer as: "sub markWord::monitor_value". But that is
>>>>> a bit out of scope for your change - it just jumped out at me. I expect
>>>>> add is more efficient than sub.
>>>>> 
>>>>> In  src/hotspot/share/opto/macro.cpp I don't understand why this int
>>>>> cast is needed:
>>>>> 
>>>>>         Node* not_biased_ctrl =  opt_bits_test(ctrl, region, 3, x_node,
>>>>> !
>>>>> (~(int)markWord::age_mask_in_place), 0);
>>>>> 
>>>>> I'm unclear if this is really
>>>>> 
>>>>> ~(int)markWord::age_mask_in_place
>>>>> 
>>>>> or
>>>>> 
>>>>> (int)(~markWord::age_mask_in_place)
>>>>> 
>>>>> ? If the latter then okay. If the former then I don't grok it. :)
>>>>> 
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>> 
>>>>>> This is a follow-up RFE from the review of JDK-8229258, where a number
>>>>>> of unnecessary casts were pointed out.
>>>>>> 
>>>>>> There was also a recent bug in this area, JDK-8221725, where Thomas
>>>>>> points out the problem of relying on the bit width of the enum values.
>>>>>> 
>>>>>> This patch:
>>>>>> 1) changes all constants to have the same type as the markWord::_value
>>>>>> field.
>>>>>> 2) Removes unnecessary uintptr_t casts
>>>>>> 3) Removes uintx casts, because they are typedefed to be uintptr_t
>>>>>> 4) Adds explicit casts to (int) when values are negated.
>>>>>> 5) Try to only use uintptr_t inside markWord.
>>>>>> 6) Leaves the use of intptr_t for parameters and return types of the
>>>>>> different hash functions.
>>>>>> 
>>>>>> Tested with tier1-3
>>>>>> 
>>>>>> Thanks,
>>>>>> StefanK
>