Yup, avoiding multiple read/write ops on the volatile field is just for perf - I saw the null guard there; sorry, should've been clearer.
Thanks Sent from my phone On Jan 10, 2013 9:47 PM, "Eric McCorkle" <eric.mccor...@oracle.com> 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>> 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/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 > > >> > > >
