Review request JDK-8004729: Parameter Reflection API

[Vitaly Davidovich]

Yup, avoiding multiple read/write ops on the volatile field is just for
perf - I saw the null guard there; sorry, should've been clearer.

Thanks

Sent from my phone
On Jan 10, 2013 9:47 PM, "Eric McCorkle" <eric.mccorkle at oracle.com> wrote:

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