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