RFR (S): 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName()

[Vitaly Davidovich]

Why not add the required code when/if it's actually needed? It's not like
the pattern is tricky :) your call of course, just looks odd as-is.

Sent from my phone
On Sep 26, 2013 8:15 PM, "Christian Thalinger" <
christian.thalinger at oracle.com> wrote:

>
> On Sep 26, 2013, at 5:11 PM, Vitaly Davidovich <vitalyd at 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.thalinger at oracle.com> wrote:
>
>>
>> On Sep 26, 2013, at 2:28 PM, John Rose <john.r.rose at oracle.com> wrote:
>>
>> > On Sep 26, 2013, at 1:13 PM, Christian Thalinger <
>> christian.thalinger at oracle.com> wrote:
>> >
>> >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger <
>> christian.thalinger at oracle.com> wrote:
>> >>
>> >>>
>> >>> On Sep 26, 2013, at 1:22 AM, Peter Levart <peter.levart at 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!
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130926/9fb66c4a/attachment-0001.html