On 01/10/2013 11:29 PM, 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?
We can not guarantee the singularity of a Parameter instance (like for
example Class instance) because of various reasons, among them:
- the Method/Constructor instances that are available via public API are
allways copied and caching of Parameter instances is performed on each
individual copied Method/Constructor instance. I already had a patch
which moved caching of annotations, for example, to the "root" instance,
but it has been postponed because of big anticipated annotations changes
comming in. Maybe we should revisit the same also for Parameter
instances when time comes...
- even if caching of Parameters/annotations is performed on "root"
instances, the root instances can change over time because they are
cached in j.l.Class via a SoftReference which can be cleared and new
"root" instances can be created for the same methods/constructors.
So as currently stands, the effort to do CAS for Parameters is useless.
Regards, Peter
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
The feature request is here:
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
Thanks,
Eric
