Review request JDK-8004729: Parameter Reflection API

[Eric McCorkle]

On 01/10/13 19:50, Vitaly Davidovich wrote:
> Hi Eric,
> 
> Parameter.equals() doesn't need null check - instanceof covers that already.
> 

Removed.

> Maybe this has been mentioned already, but personally I'm not a fan of
> null checks such as "if (null == x)" - I prefer the null on the right
> hand side, but that's just stylistic.

Changed.

> 
> Perhaps I'm looking at a stale webrev but
> Executable.privateGetParameters() reads and writes from/to the volatile
> field more than once.  I think Peter already mentioned that it should
> use one read into a local and one write to publish the final version to
> the field (it can return the temp as well).
> 

You weren't.  From a pure correctness standpoint, there is nothing wrong
with what is there.  getParameters0 is a constant function, and
parameters is writable only if null.  Hence, we only every see one
nontrivial write to it.

But you are right, it should probably be reduced to a single write, for
performance reasons (to avoid unnecessary memory barriers).  Therefore,
I changed it.

However, I won't be able to refresh the webrev until tomorrow.

> Thanks
> 
> Sent from my phone
> 
> On Jan 10, 2013 6:05 PM, "Eric McCorkle" <eric.mccorkle at oracle.com
> <mailto:eric.mccorkle at oracle.com>> wrote:
> 
>     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
>     >>
>