Hello Eric,
In Executable,
283 // TODO: This may eventually need to be guarded by security
284 // mechanisms similar to those in Field, Method, etc.
If not done so already, please file a bug to note this item for
follow-up work.
Extra space:
316
317 parameters = tmp;
318
319 }
Parameter.java
41 * @author Eric McCorkle
We generally don't use author tags in new code under /src.
For consistency with existing code
(http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1f9c19741285), please add an
"@jls 13.1 The Form of a Binary" tag to the isSynthethic method.
The @since tags on getAnnotation, getAnnotations, getDeclaredAnnotation,
etc. are redundant with the @since on the Parameter type itself and
should be removed.
Executable.c
Please verify the "Copyright (c) 1994, 2010" range on this file is
correct and that an copyright end range of "2013" is applied as appropriate.
Otherwise looks fine.
Thanks,
-Joe
On 1/22/2013 10:54 AM, Eric McCorkle wrote:
> Are there any additional comments? I'd like to get this pushed today.
>> On 01/17/13 16:54, Eric McCorkle wrote:
>> After (lengthy) examination, it seems that properly addressing the
>> synthesized parameters issue will require more changes to javac. In
>> light of this, I think that the synthesized parameter logic should go in
>> a follow-up enhancement. I have created JDK-8006345 to track this issue:
>>>>http://bugs.sun.com/view_bug.do?bug_id=8006345>>>> Therefore, I've rolled back the changes I was making which would have
>> synthesized parameters in the reflection API itself, and updated the
>> webrev. Please examine for any additional concerns.
>>>>>> On 01/11/13 13:27, Joe Darcy wrote:
>>> Hi Eric,
>>>>>> Taking another look at the code, some extra logic / checking is needed
>>> in cases where the number of source parameters (non-synthetic and
>>> non-synthesized) disagrees with the number of actual parameters at a
>>> class file level.
>>>>>> For example, if the single source parameter of an inner class
>>> constructor is annotated, the annotation should be associated with the
>>> *second* parameter since the first class file parameter is a synthesized
>>> constructor added by the compiler. I think generally annotations should
>>> not be associated with synthesized or synthetic parameters.
>>>>>> -Joe
>>>>>> On 1/11/2013 9:03 AM, Eric McCorkle wrote:
>>>> Update should be visible now.
>>>>>>>> On 01/11/13 11:54, Vitaly Davidovich wrote:
>>>>> Yes that's exactly what I'm looking for as well.
>>>>>>>>>> Sent from my phone
>>>>>>>>>> On Jan 11, 2013 11:25 AM, "Peter Levart" <peter.levart at gmail.com>>>>> <mailto:peter.levart at gmail.com>> wrote:
>>>>>>>>>> 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>
>>>>> <mailto: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>>>>> <http://cr.openjdk.java.net/~coleenp/JDK-8004729>
>>>>> >>>>>
>>>>> >>>>> The feature request is here:
>>>>> >>>>>
>>>>>http://bugs.sun.com/view_bug.__do?bug_id=8004729>>>>> <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>>>>> <http://cr.openjdk.java.net/~abuckley/8misc.pdf>
>>>>> >>>>>
>>>>> >>>>>
>>>>> >>>>> Thanks,
>>>>> >>>>> Eric
>>>>> >>
>>>>>>>>>>