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!
> 

Reply via email to