Review request JDK-8004729: Parameter Reflection API

[Vitaly Davidovich]

Hi Eric,

Parameter.equals() doesn't need null check - instanceof covers that already.

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.

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).

Thanks

Sent from my phone
On Jan 10, 2013 6:05 PM, "Eric McCorkle" <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
> >>
>