Review request JDK-8004729: Parameter Reflection API
Joe Darcy
joe.darcy at oracle.com
Sat Jan 12 18:36:57 UTC 2013
PS The link to the blog entry I meant to include:
https://blogs.oracle.com/darcy/entry/nested_inner_member_and_top
-Joe
On 1/12/2013 10:36 AM, Joe Darcy wrote:
> Hi Eric,
>
> On 1/11/2013 9:14 PM, Eric McCorkle wrote:
>> I got halfway through implementing a change that would synthesize
>> Parameter's for the static links (this for the inner classes), when it
>> occurred to me: is there really a case for allowing reflection on those
>> parameters.
>>
>> Put another way,
>>
>> public class Foo {
>> public class Bar {
>> int baz(int qux) { }
>> }
>> }
>>
>> Should baz.getParameters() return just qux, or should it expose
>> Foo.this? On second thought, I can't think of a good reason why you
>> *want* to reflect on Foo.this.
>
> You need to reflect on that parameter so you can call the constructor
> properly using reflection :-)
>
>>
>> Also, the logic is much simpler. You just have to figure out how deep
>> you are in inner classes, store that information somewhere, and offset
>> by that much every time a Parameter calls back to its Executable to get
>> information. The logic for synthesizing the this parameters is much
>> more complex.
>>
>> Thoughts?
>
> We may need some more help from javac to mark parameters as
> synthesized or synthetic, which can be done as follow-up work. For
> inner classes that are members of types, the outer this parameters are
> required to be a certain way by the platform specification to make
> linkage work. *However*, local and anonymous classes only have to
> obey a compiler's contract with itself and are not specified. In
> particular, not all such inner classes constructors even have an outer
> this parameter. For example, with javac the constructor of an
> anonymous class in a static initializer will *not* have an other this
> parameter.
>
> -Joe
>
>>
>> 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