Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

The webrev has been refreshed with the solution I describe below
implemented.  Please make additional comments.

On 01/10/13 17:29, Eric McCorkle wrote:
> Good catch there.  I made the field volatile, and I also did the same
> with the cache fields in Parameter.
> 
> It is possible with what exists that you could wind up with multiple
> copies of identical parameter objects in existence.  It goes something
> like this
> 
> thread A sees Executable.parameters is null, goes into the VM to get them
> thread B sees Executable.parameters is null, goes into the VM to get them
> thread A stores to Executable.parameters
> thread B stores to Executable.parameters
> 
> Since Parameters is immutable (except for its caches, which will always
> end up containing the same things), this *should* have no visible
> effects, unless someone does == instead of .equals.
> 
> This can be avoided by doing a CAS, which is more expensive execution-wise.
> 
> My vote is to *not* do a CAS, and accept that (in extremely rare cases,
> even as far as concurrency-related anomalies go), you may end up with
> duplicates, and document that very well.
> 
> Thoughts?
> 
> On 01/10/13 16:10, Peter Levart wrote:
>> Hello Eric,
>>
>> I have another one. Although not very likely, the reference to the same
>> Method/Constructor can be shared among multiple threads. The publication
>> of a parameters array should therefore be performed via a volatile write
>> / volatile read, otherwise it can happen that some thread sees
>> half-initialized array content. The 'parameters' field in Executable
>> should be declared as volatile and there should be a single read from it
>> and a single write to it in the privateGetParameters() method (you need
>> a local variable to hold intermediate states)...
>>
>> Regards, Peter
>>
>> On 01/10/2013 09:42 PM, Eric McCorkle wrote:
>>> Thanks to all for initial reviews; however, it appears that the version
>>> you saw was somewhat stale.  I've applied your comments (and some
>>> changes that I'd made since the version that was posted).
>>>
>>> Please take a second look.
>>>
>>> Thanks,
>>> Eric
>>>
>>>
>>> On 01/10/13 04:19, Peter Levart wrote:
>>>> Hello Eric,
>>>>
>>>> You must have missed my comment from the previous webrev:
>>>>
>>>>  292     private Parameter[] privateGetParameters() {
>>>>  293         if (null != parameters)
>>>>  294             return parameters.get();
>>>>
>>>> If/when the 'parameters' SoftReference is cleared, the method will be
>>>> returning null forever after...
>>>>
>>>> You should also retrieve the referent and check for it's presence before
>>>> returning it:
>>>>
>>>> Parameter[] res;
>>>> if (parameters != null && (res = parameters.get()) != null)
>>>>     return res;
>>>> ...
>>>> ...
>>>>
>>>> Regards, Peter
>>>>
>>>> On 01/09/2013 10:55 PM, Eric McCorkle wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the core reflection API implementation of parameter
>>>>> reflection.  This is the final component of method parameter reflection.
>>>>>   This was posted for review before, then delayed until the check-in for
>>>>> JDK-8004728 (hotspot support for parameter reflection), which occurred
>>>>> yesterday.
>>>>>
>>>>> Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, *not*
>>>>> jdk8/tl; therefore, it may be a while before the changeset makes its way
>>>>> into jdk8/tl.
>>>>>
>>>>> Also note: since the check-in of JDK-8004727 (javac support for
>>>>> parameter reflection), there has been a failure in the tests for
>>>>> Pack200.  This is being addressed in a fix contributed by Kumar, which I
>>>>> believe has also been posted for review.
>>>>>
>>>>> The open webrev is here:
>>>>> http://cr.openjdk.java.net/~coleenp/JDK-8004729
>>>>>
>>>>> The feature request is here:
>>>>> http://bugs.sun.com/view_bug.do?bug_id=8004729
>>>>>
>>>>> The latest version of the spec can be found here:
>>>>> http://cr.openjdk.java.net/~abuckley/8misc.pdf
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Eric
>>