Update should be visible now.
On 01/11/13 11:54, Vitaly Davidovich wrote: > 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 > >> > >
