Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v4]

[liach]

On Fri, 17 Jun 2022 11:21:49 GMT, Сергей Цыпанов <d...@openjdk.org> wrote:

>> If there are two threads calling `Executable.hasRealParameterData()` under 
>> race and the first one writes into volatile `Executable.parameters` field 
>> (doing _releasing store_) and the second thread reads non-null value from 
>> the same field (doing acquiring read) then it must read exactly the same 
>> value written into `hasRealParameterData` within `privateGetParameters()`. 
>> The reason for this is that we assign `hasRealParameterData` before  
>> _releasing store_.
>> 
>> In the opposite case (_acquiring read_ reads null) the second thread writes 
>> the value itself and returns it from the method so there is no change in 
>> behavior.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8288327: Mark ParameterData.parameters as @Stable and rename real -> isReal

src/java.base/share/classes/java/lang/reflect/Executable.java line 413:

> 411:     }
> 412: 
> 413:     boolean hasRealParameterData() {

Since `privateGetParameters` and `hasRealParameterData` are now both simple 
methods with only one use site, they can be simply inlined in `getParameters()` 
and `getAllGenericParameterTypes()` instead:


    public Parameter[] getParameters() {
        // Need to copy the cached array to prevent users from messing
        // with it.  Since parameters are immutable, we can
        // shallow-copy.
        return parameterData().parameters.clone();
    }
...
            final boolean realParamData = parameterData().isReal();
            final Type[] genericParamTypes = getGenericParameterTypes();
            final Type[] nonGenericParamTypes = getParameterTypes();

src/java.base/share/classes/java/lang/reflect/Executable.java line 419:

> 417:     private ParameterData parameterData() {
> 418:         ParameterData parameterData = this.parameterData;
> 419:         if (parameterData != null){

Suggestion:

        if (parameterData != null) {

src/java.base/share/classes/java/lang/reflect/Executable.java line 777:

> 775:     }
> 776: 
> 777:     record ParameterData(@Stable Parameter[] parameters, boolean isReal) 
> {}

I recommend declaring this right next to `parameterData` field for the ease of 
maintenance, like how `EntrySet` in map implementations are declared right next 
to `entrySet` methods.

-------------

PR: https://git.openjdk.org/jdk/pull/9143