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