On Sep 26, 2013, at 5:11 PM, Vitaly Davidovich <vita...@gmail.com> wrote:
> Chris, > > Since you touched getName(), maybe get rid of that superfluous null check > after expandFromVM? The code can just fall through. > > Was thinking about removing it and already had it but backed of because I wanted to keep the pattern if someone adds code to that method in the future. The compiler will optimize it anyway. > 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! >
