Review request JDK-8004729: Parameter Reflection API (original) (raw)

Vitaly Davidovich vitalyd at gmail.com
Fri Jan 11 00:50:37 UTC 2013

Hi Eric,

Parameter.equals() doesn't need null check - instanceof covers that already.

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.

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).

Thanks

Sent from my phone On Jan 10, 2013 6:05 PM, "Eric McCorkle" <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 >>>>> >>>>> The feature request is here: >>>>> http://bugs.sun.com/viewbug.do?bugid=8004729 >>>>> >>>>> The latest version of the spec can be found here: >>>>> http://cr.openjdk.java.net/~abuckley/8misc.pdf >>>>> >>>>> >>>>> Thanks, >>>>> Eric >>

More information about the core-libs-dev mailing list