Review request JDK-8004729: Parameter Reflection API
Joe Darcy
joe.darcy at oracle.com
Fri Jan 11 18:27:59 UTC 2013
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
>> >>
>>
>>
More information about the core-libs-dev
mailing list