Review request JDK-8004729: Parameter Reflection API
Eric McCorkle
eric.mccorkle at oracle.com
Fri Jan 11 17:03:34 UTC 2013
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