Review request JDK-8004729: Parameter Reflection API
Eric McCorkle
eric.mccorkle at oracle.com
Thu Jan 17 21:54:43 UTC 2013
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
>>> >>
>>>
>>>
>
More information about the core-libs-dev
mailing list