Review request JDK-8004729: Parameter Reflection API

[Peter Levart]

On 01/11/2013 04:54 PM, Eric McCorkle wrote:
> The webrev has been updated again.
>
> The multiple writes to parameters have been removed, and the tests have
> been expanded to look at inner classes, and to test modifiers.
>
> Please look over it again.

Hello Eric,

You still have 2 reads of volatile even in fast path. I would do it this 
way:


private Parameter[] privateGetParameters() {
     Parameter[] tmp = parameters; // one and only read
     if (tmp != null)
         return tmp;

     // Otherwise, go to the JVM to get them
     tmp = getParameters0();

     // If we get back nothing, then synthesize parameters
     if (tmp == null) {
         final int num = getParameterCount();
         tmp = new Parameter[num];
         for (int i = 0; i < num; i++)
         // TODO: is there a way to synthetically derive the
         // modifiers?  Probably not in the general case, since
         // we'd have no way of knowing about them, but there
         // may be specific cases.
         tmp[i] = new Parameter("arg" + i, 0, this, i);
             // This avoids possible races from seeing a
             // half-initialized parameters cache.
     }

     parameters = tmp;

     return tmp;
}


Regards, Peter

>
> Test-wise, I've got a clean run on JPRT (there were some failures in
> lambda stuff, but I've been seeing that for some time now).
>
> On 01/10/13 21:47, Eric McCorkle 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
>>>      >>
>>>