Chris, Since you touched getName(), maybe get rid of that superfluous null check after expandFromVM? The code can just fall through.
Vitaly Sent from my phone On Sep 26, 2013 5:47 PM, "Christian Thalinger" < christian.thalin...@oracle.com> wrote: > > On Sep 26, 2013, at 2:28 PM, John Rose <john.r.r...@oracle.com> wrote: > > > On Sep 26, 2013, at 1:13 PM, Christian Thalinger < > christian.thalin...@oracle.com> wrote: > > > >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger < > christian.thalin...@oracle.com> wrote: > >> > >>> > >>> On Sep 26, 2013, at 1:22 AM, Peter Levart <peter.lev...@gmail.com> > wrote: > >>> > >>>> On 09/26/2013 01:27 AM, Christian Thalinger wrote: > >>>>> http://cr.openjdk.java.net/~twisti/8019192/webrev/ > >>>>> > >>>>> 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName() > >>>>> Reviewed-by: > >>>>> > >>>>> This is a race in MemberName's name and type getters. > >>>>> > >>>>> MemberName's type field is of type Object so it can hold different > objects when it gets filled in from the VM. These types include String and > Object[]. On the first invocation the current type if it's not MethodType > gets converted to a MethodType. > >>>>> > >>>>> There is a tiny window where some instanceof check have already been > done on one thread and then another thread stores a MethodType. The > following checkcast then fails. > >>>>> > >>>>> The fix is to make name and type volatile and do the conversion in a > synchronized block. This is okay because it's only done once. > >>>>> > >>>>> src/share/classes/java/lang/invoke/MemberName.java > >>>>> > >>>> > >>>> Hi Christian, > >>>> > >>>> Wouldn't it be cleaner that instead of just casting and catching > ClassCastException, the volatile field 'type' was 1st copied to a local > variable and then an instanceof check + casting and returning performed on > the local variable. This would avoid throwing ClassCastException even if it > is performed only once per MemberName… > >>> > >>> Not sure it would be cleaner; depends on the definition of "cleaner". > I had similar code as you describe before but I changed it to catch the > exception. If people have a strong opinion here I can change it back. > >> > >> Here are the two different versions: > >> > >> http://cr.openjdk.java.net/~twisti/8019192/webrev.00/ > >> http://cr.openjdk.java.net/~twisti/8019192/webrev.01/ > > > > I think the second version is better. The first uses exception > processing for normal (though rare) control flow, which is questionable > style. > > > > As we discussed offline, the fields do *not* need to be volatile, since > the code in the synch. block will see an updated state without volatiles, > and races outside the synch block are benign, leading to the same end-state. > > > > Hoisting the field into a local is the best practice here.(***) That's > the hand we are dealt with the Java memory model. > > > > Reviewed, with those adjustments. > > …and there is the according webrev: > > http://cr.openjdk.java.net/~twisti/8019192/webrev.02/ > > > > > — John > > > > (***) P.S. This tickles one of my pet peeves. A hoisted field value > should have the same name (either approximately or exactly) as the field, > as long as the difference is merely the sampling of a value which is not > expected to change. (As opposed to a clever saving of a field that is > destined to be updated, etc.) > > > > IDEs and purists may disagree with this, but it always sets my teeth on > edge to see name changes that obscure consistency of values for the sake of > some less interesting consistency (scope rules or rare state changes). > > The problem I have with this is that some people might think the local > variable is the instance field. IDEs help here because they usually > highlight local variables and instance fields in different colors. How > about this: > > http://cr.openjdk.java.net/~twisti/8019192/webrev.03/ > > > > > Is there at least some standard naming pattern for these things? > "typeSnapshot" doesn't do it for me. "typeVal" is quieter; I prefer > quieter. Elsewhere in the same code I have defiantly used plain "type" > with a shout-out to hostile IDEs, as with MethodHandle.bindTo or > MethodHandle.asCollector. Looking for a compromise... > > > > (I also firmly believe that constructor parameter names are best named > with the corresponding field names, even though quibblers of various > species, IDE/human, note that there are distinctions to be made between > them. It is for the sake of this opinion of mine that blank finals can be > initialized with the form 'this.foo = foo', which some dislike.) > > I concur. This is what I'm doing. > > > > > But it's fine; push it! > >
