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.lev...@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.mccor...@oracle.com
>>>> <mailto:eric.mccorkle@oracle.**com <eric.mccor...@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
>>>> >>
>>>>
>>>>
>
