Re: [10] RFR: 8182487: Add Unsafe.objectFieldOffset(Class, String)

[Christian Thalinger]

> On Jun 20, 2017, at 3:14 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> as a startup optimization, we can avoid a number of reflective operations on
> variouscore classes by adding a specialized objectFieldOffset method taking
> Class and String rather than Field:
> 
> Webrev: https://bugs.openjdk.java.net/browse/JDK-8182487
> Hotspot: http://cr.openjdk.java.net/~redestad/8182487/hotspot.00 
> 

While you are cleaning this up, could we remove that code:

+static inline void throw_new(JNIEnv *env, const char *ename) {
+  char buf[100];
+
+  jio_snprintf(buf, 100, "%s%s", "java/lang/", ename);

and instead pass the whole java/lang/* string?

> JDK: http://cr.openjdk.java.net/~redestad/8182487/jdk.00
> 
> On startup tests this reduces executed instructions by ~1-2M, depending on
> how many of the touched classes are loaded.
> 
> Since all uses of this would throw an Error, InternalError or
> ExceptionInInitializerError if the field was missing - effectively
> aborting VM execution - it felt reasonable to simplify the code to
> consistently throw InternalError and remove a number of distracting
> try-catch blocks.
> 
> Thanks!
> 
> /Claes

Re: [9] RFR: 8152207: Perform array bound checks while getting a length of bytecode instructions

[Christian Thalinger]

> On May 4, 2016, at 1:48 PM, Artem Smotrakov  
> wrote:
> 
> Hello,
> 
> Please review two small patches for jdk and hotspot repos which add array 
> bound checks to functions which return a length of bytecode instruction.
> 
> Please see details in https://bugs.openjdk.java.net/browse/JDK-8152207
> 
> http://cr.openjdk.java.net/~asmotrak/8152207/jdk/webrev.00/
> http://cr.openjdk.java.net/~asmotrak/8152207/hotspot/webrev.00/

  static boolis_defined (int  code){ return 0 <= code && code < 
number_of_codes && flags(code, false) != 0; }
+  static int length_for (Code code){ return 0 <= code && code 
< number_of_codes ? _lengths[code] & 0xF : -1; }
+  static int wide_length_for(Code code){ return 0 <= code && code 
< number_of_codes ? _lengths[code]  >> 4 : -1; }
You should factor the bound check into a separate method.

> 
> Artem

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 1:22 PM, Aleksey Shipilev  
> wrote:
> 
> On 02/20/2016 01:40 AM, Christian Thalinger wrote:
>>> On Feb 19, 2016, at 9:03 AM, John Rose  wrote:
>>> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
>>> mailto:christian.thalin...@oracle.com>> 
>>> wrote:
>>>> Why don’t you change the values to:
>>>> 
>>>>   static final byte LATIN1 = 1;
>>>>   static final byte UTF16  = 2;
> 
> We've been over this during Compact Strings development. The idea that
> John has below is related to our actual insights leading to 0/1 instead
> of 1/2. The best thing you can do with 1/2 is, apparently:
> 
>  int length() {
> return value.length >> (coder - 1);
>  }
> 
>  char charAt(int idx) {
> return getCharAt(idx * coder);// variant 1
> return getCharAt(idx << (coder - 1)); // variant 2
>  }
> 
> ...and you are better off not doing excess "-1" or
> non-strength-reducible multiplications in a very hot paths in String.
> 
> Anyhow, that ship had sailed, and any change in coder definitions would
> require to respin an awful lot of Compact String testing, and probably
> revalidating lots of premises in the code. This is good as a thought
> experiment, but not practical at this point in JDK 9 development.
> 
> 
>> But if coder is stable for both values the compiler can constant fold the 
>> if-statement for the shift value:
>> 
>>  int len = val.length >> (coder == LATIN1 ? 0 : 1);
>> 
>> That should produce the same code and we would avoid:
>> 
>> 143  * Constant-folding this field is handled internally in VM.
> 
> The constant-folding story is not the only story you should care about.
> 
> For most Strings, we do not know either String.value or String.coder
> statically. This particular constant-folding bit affects String
> concatenation with constants, but the solution to it cannot possibly
> regress an overwhelming case of non-constant Strings. Changing the coder
> designations *would* affect non-constant Strings.
> 
> I would guess that the comment on "coder" field throws a reader into
> thinking that @Stable is the answer to constant folding woes. But VM
> already trusts final fields in String (e.g. String.value.arraylength is
> folded, see JDK-8149813) -- as the part of TrustStaticNonFinalFields
> machinery, which hopefully some day would get exposed to other fields.
> Frozen arrays would hit the final (pun intended) nail into String.value
> folding. @Stable hack is doing that today; but that's a hack, for a very
> performance-sensitive corner in JDK.
> 
> Hopefully the rewritten comments spell it better:

That comment is better.

>  http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.02/
>  http://cr.openjdk.java.net/~shade/8150180/webrev.hs.02/
> 
> Cheers,
> -Aleksey
> 
> 

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 9:03 AM, John Rose  wrote:
> 
> On Feb 19, 2016, at 9:57 AM, Christian Thalinger 
> mailto:christian.thalin...@oracle.com>> 
> wrote:
>> 
>> Why don’t you change the values to:
>> 
>>static final byte LATIN1 = 1;
>>static final byte UTF16  = 2;
> 
> 
> Not sure what you are asking, but here's my take on why 0,1 is the (slight) 
> winner.
> The values 0,1 are the log2 of the array element sizes 1,2, so they can be 
> used with shift instructions.
> With 1,2 they would have to be used with multiply instructions, or else 
> tweaked back to shift inputs.
> Most loops will speculate on the scale factor, but those loops which must 
> work with a non-constant scale will be slightly cleaner with 0,1.

I see what you are saying:

  int len = val.length >> coder;  // assume LATIN1=0/UTF16=1;

But if coder is stable for both values the compiler can constant fold the 
if-statement for the shift value:

  int len = val.length >> (coder == LATIN1 ? 0 : 1);

That should produce the same code and we would avoid:

143  * Constant-folding this field is handled internally in VM.

Re: RFR (S) 8150180: String.value contents should be trusted

[Christian Thalinger]

> On Feb 19, 2016, at 4:05 AM, Peter Levart  wrote:
> 
> Hi,
> 
> Just a stupid question.
> 
> Your comment in String:
> 
> 140  * @implNote Note this field is not {@link Stable}, because we want
> 141  * LATIN1 (0) coder to fold too. {@link Stable} would not allow that,
> 142  * as it reserves the default value as "not initialized" value.
> 143  * Constant-folding this field is handled internally in VM.
> 144  */
> 145 private final byte coder;

Why don’t you change the values to:

static final byte LATIN1 = 1;
static final byte UTF16  = 2;

?

> 
> 
> Couldn't @Stable final instance fields constant-fold the default value too in 
> general? I mean can't it be assumed that the final field has been set before 
> JIT kicks in?
> 
> Regards, Peter
> 
> On 02/19/2016 12:55 PM, Aleksey Shipilev wrote:
>> Hi,
>> 
>> Please review a simple performance improvement in Strings:
>>   https://bugs.openjdk.java.net/browse/JDK-8150180
>> 
>> In short, we want VM to trust constant String contents, so that
>> "Foo".charAt(0) is folded. As far as current Hotspot goes, this is only
>> achievable with @Stable -- we need to trust the array contents:
>>   http://cr.openjdk.java.net/~shade/8150180/webrev.jdk.01/
>> 
>> This, however, steps into the compiler bug caused by StringUTF16.getChar
>> intrinsic producing a mismatched load, that is then folded incorrectly.
>> So we instead rely on Java level folding in cases like these:
>>   http://cr.openjdk.java.net/~shade/8150180/webrev.hs.01/
>> 
>> ...and it works:
>>   http://cr.openjdk.java.net/~shade/8150180/notes.txt
>> 
>> While VM change looks like a workaround, Vladimir I. and me concluded
>> that @Stable folding code should just reject folding mismatched loads to
>> begin with. So, getChar intrinsic change is the actual fix. Vladimir I.
>> ought to add more assertions in @Stable folding code separately:
>>   https://bugs.openjdk.java.net/browse/JDK-8150186
>> 
>> Since this issue might trigger compiler bugs, I would like to push
>> through hs-comp to pass compiler nightlies.
>> 
>> Cheers,
>> -Aleksey
>> 
> 

Re: JDK 9 RFR to remove jdk/internal/jimage/JImageReadTest.java from the problem list

[Christian Thalinger]

> On Jan 9, 2016, at 11:23 PM, Alan Bateman  wrote:
> 
> 
> 
> On 09/01/2016 19:35, James Laskey wrote:
>> Alan questioned why this is showing up after the sjavac changes. There was 
>> concern there was some lurking issue.
>> 
> No issue removing the test from the exclude list, that should have been done 
> as part of JDK-8146712.
> 
> I think we need to establish whether the META-INF/jvmci.providers 
> configuration files are meant to be in the run-time image. I assume they are 
> but they have not been present until the recent changes (looks like the 
> sjavac change). Their presence confuses the test, a minor oversight in the 
> test that is fixed now. The issue is of course nothing to do with the jimage 
> code. If you look at a build from a few days ago then the 
> META-INF/jvmci.providers aren't in the exploded build, pull in the sjavac 
> change and they are in the exploded build. Maybe the changes to the 
> exclude/include patterns causes this but needs digging into.

The @ServiceProvider mechanism will be moved from the JVMCI to the JVMCI 
compiler to make the JVMCI simpler:

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/d1326c9f3cfb
http://hg.openjdk.java.net/graal/graal-compiler/rev/56359eb3abfa

It’s a pending change that hasn’t reached hs-comp yet but should in the next 
weeks.  Do whatever you need to do to make the tests work.

> 
> -Alan

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Christian Thalinger]

Looks good.  Thanks.

> On Dec 16, 2015, at 1:13 AM, Konstantin Shefov  
> wrote:
> 
> Christian
> 
> I have fixed the enum so it uses "ENUMENTRY(int)" format now and does linear 
> search.
> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04/
> 
> -Konstantin
> 
> On 12/15/2015 08:36 PM, Christian Thalinger wrote:
>> 
>>> On Dec 14, 2015, at 11:11 PM, Konstantin Shefov 
>>> mailto:konstantin.she...@oracle.com>> wrote:
>>> 
>>> Hi Christian
>>> 
>>> Thanks for reviewing, I have changed indents as you asked:
>>> 
>>> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03 
>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.03>
>> 
>> Thanks.  I’m still not comfortable with the enum.  It would be great if we 
>> could get the values from the VM like in JVMCI:
>> 
>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101
>> 
>> but that would be overkill here.  But I would like to see the enum entries 
>> take the integer values as arguments, like here:
>> 
>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27
>> 
>> and either do a simple linear search to find the entry or build up a table 
>> like the HotSpotConstantPool example above.
>> 
>>> 
>>> -Konstantin
>>> 
>>> On 12/15/2015 06:23 AM, Christian Thalinger wrote:
>>>> 
>>>>> On Dec 11, 2015, at 1:54 AM, Konstantin Shefov 
>>>>> mailto:konstantin.she...@oracle.com>> 
>>>>> wrote:
>>>>> 
>>>>> Hello
>>>>> 
>>>>> Please review the new version on the patch.
>>>>> 
>>>>> New webrev:
>>>>> Webrev hotspot: 
>>>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02 
>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.02>
>>>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 
>>>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.02>
>>>> 
>>>> These newlines look ridiculous especially when it’s not even needed:
>>>> 
>>>> +  // Returns a class reference index for a method or a field.
>>>> +  public int  getClassRefIndexAt
>>>> + (int index) { return 
>>>> getClassRefIndexAt0 (constantPoolOop, index); }
>>>> 
>>>> Either fix the indent or just add them like regular methods should look 
>>>> like.
>>>> 
>>>>> 
>>>>> What has been changed:
>>>>> 1. Added tests for the new added methods.
>>>>> 2. Removed CP tag codes 100 - 105 from being passed to java and left only 
>>>>> codes from the open JVM spec 
>>>>> (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140).
>>>>> 
>>>>> Thanks
>>>>> -Konstantin
>>>>> 
>>>>> On 11/27/2015 07:48 PM, Konstantin Shefov wrote:
>>>>>> Coleen,
>>>>>> Thanks for review
>>>>>> 
>>>>>> On 11/24/2015 07:33 PM, Coleen Phillimore wrote:
>>>>>>> 
>>>>>>> I have a couple preliminary comments.  First, there are no tests added 
>>>>>>> with all this new functionality.  Tests should be added with the 
>>>>>>> functionality changeset, not promised afterward.
>>>>>> I will add tests.
>>>>>>> Secondly, I do not like that JDK code knows the implementation details 
>>>>>>> of the JVM's constant pool tags.  These should be only for internal use.
>>>>>> The package "sun.reflect" is for internal use only, so it shouldn’t be a 
>>>>>> problem.
>>>>>>> My third comment is that I haven't looked carefully at this constant 
>>>>>>> pool implementation but it seems very unsafe if the class is redefined 
>>>>>>> and relies on an implementation detail in the JVM that can change.   I 
>>>>>>> will have more comments once I look more at the jvmti specification.
>>>>>>> 
>>>>>>> thanks,
>>>>>>> Coleen
>>>&

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Christian Thalinger]

> On Dec 14, 2015, at 11:11 PM, Konstantin Shefov 
>  wrote:
> 
> Hi Christian
> 
> Thanks for reviewing, I have changed indents as you asked:
> 
> http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03 
> <http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03>

Thanks.  I’m still not comfortable with the enum.  It would be great if we 
could get the values from the VM like in JVMCI:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101
 
<http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101>

but that would be overkill here.  But I would like to see the enum entries take 
the integer values as arguments, like here:

http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27
 
<http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27>

and either do a simple linear search to find the entry or build up a table like 
the HotSpotConstantPool example above.

> 
> -Konstantin
> 
> On 12/15/2015 06:23 AM, Christian Thalinger wrote:
>> 
>>> On Dec 11, 2015, at 1:54 AM, Konstantin Shefov 
>>> mailto:konstantin.she...@oracle.com>> wrote:
>>> 
>>> Hello
>>> 
>>> Please review the new version on the patch.
>>> 
>>> New webrev:
>>> Webrev hotspot: 
>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02 
>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.02>
>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 
>>> <http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.02>
>> 
>> These newlines look ridiculous especially when it’s not even needed:
>> 
>> +  // Returns a class reference index for a method or a field.
>> +  public int  getClassRefIndexAt
>> + (int index) { return 
>> getClassRefIndexAt0 (constantPoolOop, index); }
>> 
>> Either fix the indent or just add them like regular methods should look like.
>> 
>>> 
>>> What has been changed:
>>> 1. Added tests for the new added methods.
>>> 2. Removed CP tag codes 100 - 105 from being passed to java and left only 
>>> codes from the open JVM spec ( 
>>> <https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140>https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140
>>>  
>>> <https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140>).
>>> 
>>> Thanks
>>> -Konstantin
>>> 
>>> On 11/27/2015 07:48 PM, Konstantin Shefov wrote:
>>>> Coleen,
>>>> Thanks for review
>>>> 
>>>> On 11/24/2015 07:33 PM, Coleen Phillimore wrote:
>>>>> 
>>>>> I have a couple preliminary comments.  First, there are no tests added 
>>>>> with all this new functionality.  Tests should be added with the 
>>>>> functionality changeset, not promised afterward. 
>>>> I will add tests.
>>>>> Secondly, I do not like that JDK code knows the implementation details of 
>>>>> the JVM's constant pool tags.  These should be only for internal use.
>>>> The package "sun.reflect" is for internal use only, so it shouldn’t be a 
>>>> problem.
>>>>> My third comment is that I haven't looked carefully at this constant pool 
>>>>> implementation but it seems very unsafe if the class is redefined and 
>>>>> relies on an implementation detail in the JVM that can change.   I will 
>>>>> have more comments once I look more at the jvmti specification.
>>>>> 
>>>>> thanks,
>>>>> Coleen
>>>>> 
>>>>> On 11/24/15 9:48 AM, Konstantin Shefov wrote:
>>>>>> Hello
>>>>>> 
>>>>>> Please, review modified webrev.
>>>>>> 
>>>>>> I have added methods
>>>>>> getNameAndTypeRefIndexAt(int index) - to get name and type entry index 
>>>>>> from a method, field or indy entry index;
>>>>>> getClassRefIndexAt(int index) - to get class entry index from a method 
>>>>>> or field entry index;
>>>>>> 
>

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Christian Thalinger]

> On Dec 11, 2015, at 1:54 AM, Konstantin Shefov  
> wrote:
> 
> Hello
> 
> Please review the new version on the patch.
> 
> New webrev:
> Webrev hotspot: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02
> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 
> <http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02>

These newlines look ridiculous especially when it’s not even needed:

+  // Returns a class reference index for a method or a field.
+  public int  getClassRefIndexAt
+ (int index) { return getClassRefIndexAt0 
(constantPoolOop, index); }

Either fix the indent or just add them like regular methods should look like.

> 
> What has been changed:
> 1. Added tests for the new added methods.
> 2. Removed CP tag codes 100 - 105 from being passed to java and left only 
> codes from the open JVM spec 
> (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140).
> 
> Thanks
> -Konstantin
> 
> On 11/27/2015 07:48 PM, Konstantin Shefov wrote:
>> Coleen,
>> Thanks for review
>> 
>> On 11/24/2015 07:33 PM, Coleen Phillimore wrote:
>>> 
>>> I have a couple preliminary comments.  First, there are no tests added with 
>>> all this new functionality.  Tests should be added with the functionality 
>>> changeset, not promised afterward. 
>> I will add tests.
>>> Secondly, I do not like that JDK code knows the implementation details of 
>>> the JVM's constant pool tags.  These should be only for internal use.
>> The package "sun.reflect" is for internal use only, so it shouldn’t be a 
>> problem.
>>> My third comment is that I haven't looked carefully at this constant pool 
>>> implementation but it seems very unsafe if the class is redefined and 
>>> relies on an implementation detail in the JVM that can change.   I will 
>>> have more comments once I look more at the jvmti specification.
>>> 
>>> thanks,
>>> Coleen
>>> 
>>> On 11/24/15 9:48 AM, Konstantin Shefov wrote:
>>>> Hello
>>>> 
>>>> Please, review modified webrev.
>>>> 
>>>> I have added methods
>>>> getNameAndTypeRefIndexAt(int index) - to get name and type entry index 
>>>> from a method, field or indy entry index;
>>>> getClassRefIndexAt(int index) - to get class entry index from a method or 
>>>> field entry index;
>>>> 
>>>> I removed previously added method
>>>> getInvokedynamicRefInfoAt(int index) - as it is no more needed now.
>>>> 
>>>> New webrev:
>>>> Webrev hotspot: 
>>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.01
>>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.01
>>>> 
>>>> Thanks
>>>> -Konstantin
>>>> 
>>>> On 11/18/2015 02:11 PM, Konstantin Shefov wrote:
>>>>> Remi,
>>>>> 
>>>>> Thanks for reviewing. Your suggestion sounds sensibly.
>>>>> May be it also has sense to make a method 
>>>>> "getMethodRefNameAndTypeIndex(int index)" to acquire name-and-type entry 
>>>>> index for methods also.
>>>>> 
>>>>> -Konstantin
>>>>> 
>>>>> On 11/18/2015 12:04 AM, Remi Forax wrote:
>>>>>> Hi Konstantin,
>>>>>> Technically, getInvokedynamicRefInfoAt should be 
>>>>>> getNameAndTypeOfInvokedynamicRefInfoAt because it only extract the 
>>>>>> nameAndType value of the InvokeDynamicRef.
>>>>>> 
>>>>>> In term of API, I think it's better to decompose the API, i.e. to have a 
>>>>>> method
>>>>>>   int getInvokedynamicRefNameAndTypeIndex(int index)
>>>>>> that returns the nameAndType index and to reuse 
>>>>>> getNameAndTypeRefInfoAt(index) to get the corresponding array of Strings.
>>>>>> 
>>>>>> cheers,
>>>>>> Rémi
>>>>>> 
>>>>>> - Mail original -
>>>>>>> De: "Christian Thalinger" 
>>>>>>> À: "Konstantin Shefov" 
>>>>>>> Cc: "hotspot-dev developers" , 
>>>>>>> core-libs-dev@openjdk.java.net
>>>>>>> Envoyé: Lundi 16 Novembre 2015 23:41:45
>>>>>>> Objet: Re: RFR [9] 8141615: Add new public methods to 

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Christian Thalinger]

Kind of related but not really, I’ve filed a bug against ConstantPool to add 
JSR 292 “support”:

JDK-8077229: sun.reflect.ConstantPool should support class and method constant 
pools
https://bugs.openjdk.java.net/browse/JDK-8077229 
<https://bugs.openjdk.java.net/browse/JDK-8077229>

> On Nov 17, 2015, at 11:04 AM, Remi Forax  wrote:
> 
> Hi Konstantin,
> Technically, getInvokedynamicRefInfoAt should be 
> getNameAndTypeOfInvokedynamicRefInfoAt because it only extract the 
> nameAndType value of the InvokeDynamicRef.
> 
> In term of API, I think it's better to decompose the API, i.e. to have a 
> method
>  int getInvokedynamicRefNameAndTypeIndex(int index)
> that returns the nameAndType index and to reuse 
> getNameAndTypeRefInfoAt(index) to get the corresponding array of Strings. 
> 
> cheers,
> Rémi
> 
> - Mail original -
>> De: "Christian Thalinger" 
>> À: "Konstantin Shefov" 
>> Cc: "hotspot-dev developers" , 
>> core-libs-dev@openjdk.java.net
>> Envoyé: Lundi 16 Novembre 2015 23:41:45
>> Objet: Re: RFR [9] 8141615: Add new public methods to
>> sun.reflect.ConstantPool
>> 
>> [CC'ing core-libs-dev]
>> 
>>> On Nov 13, 2015, at 4:55 AM, Konstantin Shefov
>>>  wrote:
>>> 
>>> Hello
>>> 
>>> Please review an enhancement to add three new methods to
>>> sun.reflect.ConstantPool class.
>>> The following methods are suggested:
>>> 
>>> public String[] getNameAndTypeRefInfoAt(int index) - returns string
>>> representation of name and type from a NameAndType constant pool entry
>>> with the specified index
>>> 
>>> public String[] getInvokedynamicRefInfoAt(int index) - returns string
>>> representation of name and type from an InvokeDynamic constant pool entry
>>> with the specified index
>>> 
>>> public Tag getTagAt(int index) - returns a Tag enum instance of a constant
>>> pool entry with the specified index
>>> 
>>> These three methods could be used for testing API working with constant
>>> pool, e.g. JVMCI.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8141615
>>> Webrev hotspot:
>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00
>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00
>>> 
>>> Thanks
>>> -Konstantin
>> 
>> 

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Christian Thalinger]

[CC'ing core-libs-dev]

> On Nov 13, 2015, at 4:55 AM, Konstantin Shefov  
> wrote:
> 
> Hello
> 
> Please review an enhancement to add three new methods to 
> sun.reflect.ConstantPool class.
> The following methods are suggested:
> 
> public String[] getNameAndTypeRefInfoAt(int index) - returns string 
> representation of name and type from a NameAndType constant pool entry with 
> the specified index
> 
> public String[] getInvokedynamicRefInfoAt(int index) - returns string 
> representation of name and type from an InvokeDynamic constant pool entry 
> with the specified index
> 
> public Tag getTagAt(int index) - returns a Tag enum instance of a constant 
> pool entry with the specified index
> 
> These three methods could be used for testing API working with constant pool, 
> e.g. JVMCI.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8141615
> Webrev hotspot: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00
> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00
> 
> Thanks
> -Konstantin

Re: 8139891: Prepare Unsafe for true encapsulation

[Christian Thalinger]

> On Oct 22, 2015, at 6:31 AM, Paul Sandoz  wrote:
> 
> Hi Chris,
> 
> This looks like a good first step. I am sure hotspot devs will have more 
> concrete review comments, but i like the way the method aliasing (copying the 
> same technique StrictMath -> Math) avoids any changes to the intrinsics, thus 
> the changes are much more localized.

Yes, me too.  I was worried about a lot of changes in compiler code so I’m 
really happy to see this is not the case.

> 
> It may be worth having some fast debugging output for the aliasing in case 
> internal refactoring messes things up.
> 
> Paul.
> 
>> On 22 Oct 2015, at 07:28, Chris Hegarty  wrote:
>> 
>> As part of the preparation for JEP 260 [1], Unsafe needs to be separable
>> from the base module [2], so it can be exported by a new, yet to be
>> defined, jdk.unsupported module, and have a separate evolution policy
>> that is not exposed. That is, the JDK needs an internal Unsafe that can
>> be evolved and added to in future releases, while maintaining the
>> existing Unsafe API that developers are using.
>> 
>> Many uses of Unsafe are for performance reasons. Any changes made
>> should preserve the current performance characteristics. To achieve this
>> the existing Unsafe intrinsic candidate methods should remain intrinsic
>> candidate methods. The VM can provide aliases for these intrinsics so
>> they are common to both the internal and sun.misc versions of Unsafe.
>> 
>> The JDK implementation will be updated to use the new internal version
>> of Unsafe.
>> 
>> For the purposes of making progress, I think this work can be split into
>> several steps:
>> 
>> 1) Create the new internal Unsafe class and provide intrinsic support
>>for both.
>> 2) Update existing, and possibly add new, tests to use the new
>>internal Unsafe. Some tests may need to be duplicated, or reworked,
>>to test both versions of Unsafe.
>> 3) Update the Unsafe usages in the JDK so that there is no longer any
>>dependency on sun.misc.Unsafe.
>> 
>> To this end I've put together a webrev containing the changes required
>> for step 1. It contains
>>  - the intrinsic aliasing,
>>  - the new internal Unsafe ( copy of sun.misc.Unsafe ),
>>  - reverts sun.misc.Unsafe to, almost, its 1.8 API,
>>  - minimal test and implementation updates, since there some usages
>>of unaligned access that is new in the 1.9.
>> 
>> http://cr.openjdk.java.net/~chegar/unsafe_phase1/
>> 
>> All JPRT hotspot and core libraries tests pass.
>> 
>> I have most of the work for steps 2 and 3 done, but some discussion and
>> clean up is required. It also increase the level of difficult to review
>> the changes in phase 1, which is mostly what I'd like to get agreement
>> on first.
>> 
>> -Chris.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8132928
>> [2] https://bugs.openjdk.java.net/browse/JDK-8139891
> 

Re: RFR: 8136556 - Add the ability to perform static builds of MacOSX x64 binaries

[Christian Thalinger]

> On Oct 19, 2015, at 8:09 AM, Bob Vandette  wrote:
> 
> Here’s the updated set of webrev’s that address two issues:
> 
> 1. Move jni_util.h out of jawt.h
> 2. Christians concern over using a single variable name for Makefile and 
> C/C++ logic.

Thanks.  Looks good to me.

> 
> http://cr.openjdk.java.net/~bobv/8136556/webrev.01 
> 
> http://cr.openjdk.java.net/~bobv/8136556/hotspot/webrev.01 
> 
> http://cr.openjdk.java.net/~bobv/8136556/jdk/webrev.01 
> 
> 
> Bob.
> 
>> On Oct 16, 2015, at 2:28 AM, Alan Bateman > > wrote:
>> 
>> On 15/10/2015 19:07, Bob Vandette wrote:
>>> Please review this JDK 9 enhancement which allows a completely static build 
>>> of the JDK for MacOSX x64 platforms.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8136556 
>>>  
>>> >> >
>>> 
>>> The change involves:
>>> 
>>> 1. Producing “.a” archives for each native libraries.
>>> 2. Ensuring that all symbols across the JDK native libraries are unique.
>>> 3. Changing the JNI_OnLoad and JNI_OnUnload (and the Agent equivalents) to 
>>> have the each library name appended per
>>>the JNI specification.
>>> 4. Modifications to the launcher and launcher Makefiles to allow them to be 
>>> linked with the java.base and jdk.jdwp.agent libraries
>>>and function.
>>> 
>>> http://cr.openjdk.java.net/~bobv/8136556/webrev.00/ 
>>>  
>>> >> >
>>> http://cr.openjdk.java.net/~bobv/8136556/hotspot/webrev.00/ 
>>>  
>>> >> >
>>> http://cr.openjdk.java.net/~bobv/8136556/jdk/webrev.00/ 
>>>  
>>> >> >
>>> 
>>> Note: This change does not link every possible static library with the 
>>> launchers.  It is currently limited to
>>> the java.base and jdk.jdwp.agent libraries in order to allow for the TCK 
>>> validation of the base module only.
>>> 
>> I've skimmed through the patches and the DEF_* macros look okay. The only 
>> one that doesn't look right is jawt.h/jawt.c. As jawt.h is shipped by the 
>> JDK then I think the include of jni_util.h needs to move from jawt.h to 
>> jawt.c.
>> 
>> If I read the changes correctly then not loading the 
>> JavaRuntimeSupport.framework on Mac means the locale might not be right, is 
>> that correct? Brent might remember this issue as we've often pondered the 
>> implications of this disappearing in an Mac update.
>> 
>> Will there be continuous or at least regular builds setup so that build 
>> breakages will be reported in a timely manner? It would be easy to change 
>> something that breaks the static library build.
>> 
>> -Alan
> 

Re: RFR: 8136556 - Add the ability to perform static builds of MacOSX x64 binaries

[Christian Thalinger]

Copy-pasting the comment I made earlier (internally):

>> make/bsd/makefiles/gcc.make:
>> 
>> + ifeq ($(BUILD_STATIC),true)
>> + CXXFLAGS += -DSTATIC_BUILD
>> + CFLAGS += -DSTATIC_BUILD
>> 
>> Can we use the same name everywhere?
> 
> We were trying to differentiate Makefile options from compile time 
> conditionals.
> In one case it’s set to true and the other it’s defined.
> 
> Are there no other cases where this is done?

I’m not sure but looking at make/excludeSrc.make all of them use the same name.

> On Oct 15, 2015, at 8:10 AM, Bob Vandette  wrote:
> 
> Please review this JDK 9 enhancement which allows a completely static build 
> of the JDK for MacOSX x64 platforms.
> 
> https://bugs.openjdk.java.net/browse/JDK-8136556 
> 
> 
> The change involves:
> 
> 1. Producing “.a” archives for each native libraries.
> 2. Ensuring that all symbols across the JDK native libraries are unique.
> 3. Changing the JNI_OnLoad and JNI_OnUnload (and the Agent equivalents) to 
> have the each library name appended per
>   the JNI specification.
> 4. Modifications to the launcher and launcher Makefiles to allow them to be 
> linked with the java.base and jdk.jdwp.agent libraries
>   and function.
> 
> http://cr.openjdk.java.net/~bobv/8136556/webrev.00/ 
> 
> http://cr.openjdk.java.net/~bobv/8136556/hotspot/webrev.00/ 
> 
> http://cr.openjdk.java.net/~bobv/8136556/jdk/webrev.00/ 
> 
> 
> Note: This change does not link every possible static library with the 
> launchers.  It is currently limited to
> the java.base and jdk.jdwp.agent libraries in order to allow for the TCK 
> validation of the base module only.
> 
> 
> Bob.

Re: RFR - 8072450: 9-dev build failed on elinux-i586 and rlinux-i586

[Christian Thalinger]

> On Feb 4, 2015, at 9:44 AM, Daniel Fuchs  wrote:
> 
> On 04/02/15 17:21, Christian Thalinger wrote:
>>> 
>>> -const jlong MAX_DIFF_SECS = 0x01;   //  2^32
>>> +const jlong MAX_DIFF_SECS = 0x01LL; //  2^32
>> 
>> Don’t use LL directly; we have a compiler-dependent macro for this:
>> 
>> CONST64
>> 
> 
> Hi Christian,
> 
> I have pushed the change as it was reviewed by David, Coleen, & Staffan.
> 
> I have logged https://bugs.openjdk.java.net/browse/JDK-8072482
> (assigned to myself) as a followup bug.

That’s fine, thanks.  Maybe you can fix a couple other places where LL-usage 
crept in too.

> 
> best regards,
> 
> -- daniel

Re: RFR - 8072450: 9-dev build failed on elinux-i586 and rlinux-i586

[Christian Thalinger]

> On Feb 4, 2015, at 3:31 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a fix for:
> 
> 8072450: 9-dev build failed on elinux-i586 and rlinux-i586
> 
> My fix for JDK-8068730 which was integrated from hs-rt into jdk9-dev
> yesterday is causing a build failure in the jdk9/dev nightly on two platforms.
> It appears that these platforms have a slightly older version
> of the compiler - which chokes on jvm.cpp with the following error:
> 
> hotspot/src/share/vm/prims/jvm.cpp:307: error: integer constant is too large 
> for ���long��� type
> 
> Adding a LL suffix should solve the issue:
> 
> ##
> 
> diff --git a/src/share/vm/prims/jvm.cpp b/src/share/vm/prims/jvm.cpp
> --- a/src/share/vm/prims/jvm.cpp
> +++ b/src/share/vm/prims/jvm.cpp
> @@ -304,7 +304,7 @@
> // java.lang.System, but we choose to keep it here so that it stays next
> // to JVM_CurrentTimeMillis and JVM_NanoTime
> 
> -const jlong MAX_DIFF_SECS = 0x01;   //  2^32
> +const jlong MAX_DIFF_SECS = 0x01LL; //  2^32

Don’t use LL directly; we have a compiler-dependent macro for this:

CONST64

> const jlong MIN_DIFF_SECS = -MAX_DIFF_SECS; // -2^32
> 
> JVM_LEAF(jlong, JVM_GetNanoTimeAdjustment(JNIEnv *env, jclass ignored, jlong 
> offset_secs))
> 
> ##
> 
> ( or if you prefer here is a webrev:
>  http://cr.openjdk.java.net/~dfuchs/webrev_8072450/webrev.00 )
> 
> best regards,
> 
> -- daniel
> 

Re: [9] RFR(L) 8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames

[Christian Thalinger]

> On Nov 3, 2014, at 12:41 PM, David Chase  wrote:
> 
> 
> On 2014-11-03, at 3:09 PM, Peter Levart  wrote:
> 
>> Hi David,
>> 
>> I was thinking about the fact that java.lang.invoke code is leaking into 
>> java.lang.Class. Perhaps, If you don't mind rewriting the code, a better 
>> code structure would be, if j.l.Class changes only consisted of adding a 
>> simple:
>> …
> 
>> This way all the worries about ordering of writes into array and/or size are 
>> gone. The array is still used to quickly search for an element, but VM only 
>> scans the linked-list.
>> 
>> What do you think of this?
> 
> I’m not sure.  I know Coleen Ph would like to see that happen.
> 
> A couple of people have vague plans to move more of the MemberName resolution 
> into core libs.
> (Years ago I worked on a VM where *all* of this occurred in Java, but some of 
> it was ahead of time.)
> 
> I heard mention of “we want to put more stuff in there” but I got the 
> impression that already happened
> (there’s reflection data, for example) so I’m not sure that makes sense.
> 
> There’s also a proposal from people in the runtime to just use a jmethodid, 
> take the hit of an extra indirection,
> and no need to for this worrisome jvm/java concurrency.
> 
> And if we instead wrote a hash table that only grew, and never relocated 
> elements, we could
> (I think) allow non-synchronized O(1) probes of the table from the Java side, 
> synchronized
> O(1) insertions from the Java side, and because nothing moves, a smaller 
> dance with the
> VM.  I’m rather tempted to look into this — given the amount of work it would 
> take to do the
> benchmarking to see if (a) jmethodid would have acceptable performance or (b) 
> the existing
> costs are too high, I could instead just write fast code and be done.

…but you still have to do the benchmarking.  Let’s not forget that there was a 
performance regression with the first C++ implementation of this.

> 
> And another way to view this is that we’re now quibbling about performance, 
> when we still
> have an existing correctness problem that this patch solves, so maybe we 
> should just get this
> done and then file an RFE.
> 
> David
> 

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Christian Thalinger]

On Jun 30, 2014, at 5:50 PM, Coleen Phillimore  
wrote:

> 
> On 6/30/14, 3:50 PM, Christian Thalinger wrote:
>>  private Class(ClassLoader loader) {
>>  // Initialize final field for classLoader.  The initialization 
>> value of non-null
>>  // prevents future JIT optimizations from assuming this final field 
>> is null.
>>  classLoader = loader;
>> +componentType = null;
>>  }
>> 
>> Are we worried about the same optimization?
> 
> Hi,  I've decided to make them consistent and add another parameter to the 
> Class constructor.
> 
> http://cr.openjdk.java.net/~coleenp/8047737_jdk_2/

Thanks.

> 
> Thanks,
> Coleen
>> 
>> +  compute_optional_offset(_component_mirror_offset,
>> + klass_oop, vmSymbols::componentType_name(),
>> + vmSymbols::class_signature());
>> 
>> Is there a followup cleanup to make it non-optional?  Or, are you waiting 
>> for JPRT to be able to push hotspot and jdk changes together?
>> 
>> On Jun 30, 2014, at 5:42 AM, Coleen Phillimore > <mailto:coleen.phillim...@oracle.com>> wrote:
>> 
>>> 
>>> On 6/30/14, 1:55 AM, David Holmes wrote:
>>>> Hi Coleen,
>>>> 
>>>> Your webrev links are to internal locations.
>>> 
>>> Sorry, I cut/pasted the wrong links.  They are:
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8047737_jdk/ 
>>> <http://cr.openjdk.java.net/%7Ecoleenp/8047737_jdk/>
>>> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
>>> 
>>> and the full version
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
>>> 
>>> Thank you for pointing this out David.
>>> 
>>> Coleen
>>> 
>>>> 
>>>> David
>>>> 
>>>> On 28/06/2014 5:24 AM, Coleen Phillimore wrote:
>>>>> Summary: Add field in java.lang.Class for componentType to simplify oop
>>>>> processing and intrinsics in JVM
>>>>> 
>>>>> This is part of ongoing work to clean up oop pointers in the metadata
>>>>> and simplify the interface between the JDK j.l.C and the JVM. There's a
>>>>> performance benefit at the end of all of this if we can remove all oop
>>>>> pointers from metadata.   mirror in Klass is the only one left after
>>>>> this full change.
>>>>> 
>>>>> See bug https://bugs.openjdk.java.net/browse/JDK-8047737
>>>>> 
>>>>> There are a couple steps to this change because Hotspot testing is done
>>>>> with promoted JDKs.  The first step is this webrev:
>>>>> 
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/
>>>>> 
>>>>> When the JDK is promoted, the code to remove
>>>>> ArrayKlass::_component_mirror will be changed under a new bug id.
>>>>> 
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full
>>>>> 
>>>>> Finally, a compatibility request and licensee notification will occur to
>>>>> remove the function JVM_GetComponentType.
>>>>> 
>>>>> Performance testing was done that shows no difference in performance.
>>>>> The isArray() call is a compiler intrinsic which is now called instead
>>>>> of getComponentType, which was recognized as a compiler intrinsic.
>>>>> 
>>>>> JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
>>>>> tests were performed on both the change requested (1st one) and the full
>>>>> change.
>>>>> 
>>>>> hotspot NSK tests were run on the hotspot-only change with a promoted JDK.
>>>>> 
>>>>> Please send your comments.
>>>>> 
>>>>> Thanks,
>>>>> Coleen
>>> 
>> 
> 

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Christian Thalinger]

On Jun 30, 2014, at 1:06 PM, Coleen Phillimore  
wrote:

> 
> On 6/30/14, 3:50 PM, Christian Thalinger wrote:
>>  private Class(ClassLoader loader) {
>>  // Initialize final field for classLoader.  The initialization 
>> value of non-null
>>  // prevents future JIT optimizations from assuming this final field 
>> is null.
>>  classLoader = loader;
>> +componentType = null;
>>  }
>> 
>> Are we worried about the same optimization?
> 
> I don't know if I was justified in worrying about the optimization in the 
> first place.   Since getComponentType() is conditional, I wasn't worried.
> 
> But it should be consistent.  Maybe I should revert the classLoader 
> constructor change, now that I fixed all the tests not to care.   What do 
> people think?
>> 
>> +  compute_optional_offset(_component_mirror_offset,
>> + klass_oop, vmSymbols::componentType_name(),
>> + vmSymbols::class_signature());
>> 
>> Is there a followup cleanup to make it non-optional?  Or, are you waiting 
>> for JPRT to be able to push hotspot and jdk changes together?
> 
> Yes, please look at the _full webrev.  That has the non-optional changes in 
> it and the follow on changes to remove getComponentType as an intrinsic in C2 
> (will file new RFE).  I really would like a compiler person to comment on it.

Sorry, I missed that.  Yes, the compiler changes look good.

> 
> Thanks so much,
> Coleen
> 
>> 
>> On Jun 30, 2014, at 5:42 AM, Coleen Phillimore 
>>  wrote:
>> 
>>> 
>>> On 6/30/14, 1:55 AM, David Holmes wrote:
>>>> Hi Coleen,
>>>> 
>>>> Your webrev links are to internal locations.
>>> 
>>> Sorry, I cut/pasted the wrong links.  They are:
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8047737_jdk/
>>> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
>>> 
>>> and the full version
>>> 
>>> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
>>> 
>>> Thank you for pointing this out David.
>>> 
>>> Coleen
>>> 
>>>> 
>>>> David
>>>> 
>>>> On 28/06/2014 5:24 AM, Coleen Phillimore wrote:
>>>>> Summary: Add field in java.lang.Class for componentType to simplify oop
>>>>> processing and intrinsics in JVM
>>>>> 
>>>>> This is part of ongoing work to clean up oop pointers in the metadata
>>>>> and simplify the interface between the JDK j.l.C and the JVM. There's a
>>>>> performance benefit at the end of all of this if we can remove all oop
>>>>> pointers from metadata.   mirror in Klass is the only one left after
>>>>> this full change.
>>>>> 
>>>>> See bug https://bugs.openjdk.java.net/browse/JDK-8047737
>>>>> 
>>>>> There are a couple steps to this change because Hotspot testing is done
>>>>> with promoted JDKs.  The first step is this webrev:
>>>>> 
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/
>>>>> 
>>>>> When the JDK is promoted, the code to remove
>>>>> ArrayKlass::_component_mirror will be changed under a new bug id.
>>>>> 
>>>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full
>>>>> 
>>>>> Finally, a compatibility request and licensee notification will occur to
>>>>> remove the function JVM_GetComponentType.
>>>>> 
>>>>> Performance testing was done that shows no difference in performance.
>>>>> The isArray() call is a compiler intrinsic which is now called instead
>>>>> of getComponentType, which was recognized as a compiler intrinsic.
>>>>> 
>>>>> JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
>>>>> tests were performed on both the change requested (1st one) and the full
>>>>> change.
>>>>> 
>>>>> hotspot NSK tests were run on the hotspot-only change with a promoted JDK.
>>>>> 
>>>>> Please send your comments.
>>>>> 
>>>>> Thanks,
>>>>> Coleen
>>> 
>> 
> 

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Christian Thalinger]

 private Class(ClassLoader loader) {
 // Initialize final field for classLoader.  The initialization value 
of non-null
 // prevents future JIT optimizations from assuming this final field is 
null.
 classLoader = loader;
+componentType = null;
 }

Are we worried about the same optimization?

+  compute_optional_offset(_component_mirror_offset,
+ klass_oop, vmSymbols::componentType_name(),
+ vmSymbols::class_signature());

Is there a followup cleanup to make it non-optional?  Or, are you waiting for 
JPRT to be able to push hotspot and jdk changes together?

On Jun 30, 2014, at 5:42 AM, Coleen Phillimore  
wrote:

> 
> On 6/30/14, 1:55 AM, David Holmes wrote:
>> Hi Coleen,
>> 
>> Your webrev links are to internal locations.
> 
> Sorry, I cut/pasted the wrong links.  They are:
> 
> http://cr.openjdk.java.net/~coleenp/8047737_jdk/
> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
> 
> and the full version
> 
> http://cr.openjdk.java.net/~coleenp/8047737_hotspot/
> 
> Thank you for pointing this out David.
> 
> Coleen
> 
>> 
>> David
>> 
>> On 28/06/2014 5:24 AM, Coleen Phillimore wrote:
>>> Summary: Add field in java.lang.Class for componentType to simplify oop
>>> processing and intrinsics in JVM
>>> 
>>> This is part of ongoing work to clean up oop pointers in the metadata
>>> and simplify the interface between the JDK j.l.C and the JVM. There's a
>>> performance benefit at the end of all of this if we can remove all oop
>>> pointers from metadata.   mirror in Klass is the only one left after
>>> this full change.
>>> 
>>> See bug https://bugs.openjdk.java.net/browse/JDK-8047737
>>> 
>>> There are a couple steps to this change because Hotspot testing is done
>>> with promoted JDKs.  The first step is this webrev:
>>> 
>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/
>>> 
>>> When the JDK is promoted, the code to remove
>>> ArrayKlass::_component_mirror will be changed under a new bug id.
>>> 
>>> http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full
>>> 
>>> Finally, a compatibility request and licensee notification will occur to
>>> remove the function JVM_GetComponentType.
>>> 
>>> Performance testing was done that shows no difference in performance.
>>> The isArray() call is a compiler intrinsic which is now called instead
>>> of getComponentType, which was recognized as a compiler intrinsic.
>>> 
>>> JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
>>> tests were performed on both the change requested (1st one) and the full
>>> change.
>>> 
>>> hotspot NSK tests were run on the hotspot-only change with a promoted JDK.
>>> 
>>> Please send your comments.
>>> 
>>> Thanks,
>>> Coleen
> 

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Christian Thalinger]

On Jun 2, 2014, at 6:29 AM, Vladimir Ivanov  
wrote:

> Tobias, I'll take care of it.

Are you also taking care of the backports?

> 
> Best regards,
> Vladimir Ivanov
> 
> On 6/2/14 10:04 AM, Tobias Hartmann wrote:
>> 
>> On 28.05.2014 12:48, Vladimir Ivanov wrote:
>>> Looks good.
>>> 
>>> It should be safe to sync on MTF instance since it's not accessible
>>> outside (MTF and MT.form() are package-private).
>>> 
>>> Best regards,
>>> Vladimir Ivanov
>> 
>> Thank you, Vladimir.
>> 
>> Could someone please push the patch?
>> 
>> Thanks,
>> Tobias
>> 
>>> 
>>> On 5/28/14 1:49 PM, Tobias Hartmann wrote:
 Hi,
 
 thanks everyone for the feedback!
 
 @Remi: I agree with Paul. This is not a problem because if the normal
 read sees an outdated null value, a new LambdaForm is created and
 setCachedLambdaForm(...) is executed. This will guarantee that the
 non-null value is seen and used. The unnecessary creation of a new
 LamdaForm is not a problem either.
 
 @John: I added the code that you suggested to simulate CAS. Please find
 the new webrev at:
 
 http://cr.openjdk.java.net/~anoll/8005873/webrev.02/
 
 Sorry for the delay, I was on vacation.
 
 Thanks,
 Tobias
 
 On 19.05.2014 20:31, John Rose wrote:
> On May 16, 2014, at 4:56 AM, Tobias Hartmann
>  wrote:
> 
>> Is it sufficient then to use synchronized (lambdaForms) { ... } in
>> setCachedLambdaForm(..) and a normal read in cachedLambdaForm(..)?
> Yes, that is how I see it.  The fast path is a racy non-volatile read
> of a safely-published structure.
> 
> (If safe publication via arrays were broken, java.lang.String would be
> broken.  But the JMM is carefully designed to support safe publication
> of array elements, and through array elements.)
> 
> — John
 
>> 

Re: RFR (XS): 8032606: ClassValue.ClassValueMap.type is unused

[Christian Thalinger]

Thanks, John and Vladimir.

On May 15, 2014, at 2:46 PM, Vladimir Ivanov  
wrote:

> Looks good.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 5/15/14 9:48 PM, Christian Thalinger wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8032606
>> http://cr.openjdk.java.net/~twisti/8032606/webrev.00
>> 
>> 8032606: ClassValue.ClassValueMap.type is unused
>> Reviewed-by:
>> 

RFR (XS): 8032606: ClassValue.ClassValueMap.type is unused

[Christian Thalinger]

https://bugs.openjdk.java.net/browse/JDK-8032606
http://cr.openjdk.java.net/~twisti/8032606/webrev.00

8032606: ClassValue.ClassValueMap.type is unused
Reviewed-by:

Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache

[Christian Thalinger]

On May 14, 2014, at 3:47 AM, Vladimir Ivanov  
wrote:

> Tobias, I agree with your evaluation.
> 
> My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT 
> doesn't see what is stored in the array.

Now that is really unfortunate.

> Maybe use a lock instead?
> 
> Best regards,
> Vladimir Ivanov
> 
> On 5/14/14 2:33 PM, Tobias Hartmann wrote:
>> Hi Remi,
>> 
>> sorry, I accidentally reverted that part.. Here is the correct webrev:
>> 
>> http://cr.openjdk.java.net/~anoll/8005873/webrev.01/
>> 
>> Thanks,
>> Tobias
>> 
>> On 14.05.2014 12:04, Remi Forax wrote:
>>> your patch doesn't work !
>>> 
>>> the array is still allocated as an classical array in the constructor.
>>> 
>>> cheers,
>>> Remi
>>> 
>>> Envoyé avec AquaMail pour Android
>>> http://www.aqua-mail.com
>>> 
>>> 
>>> Le 14 mai 2014 11:04:41 Tobias Hartmann  a
>>> écrit :
>>> 
>>>> Hi,
>>>> 
>>>> please review the following patch for bug 8005873.
>>>> 
>>>> *Problem*
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8005873
>>>> 
>>>> The test creates multiple threads (~500) that repeatedly execute
>>>> invokeExact on a method handle referencing a static method. The call
>>>> to invokeExact is performed through an optimized inline cache
>>>> (CompiledStaticCall) that points to the LambdaForm generated for the
>>>> method handle. The IC is first set to interpreted code by
>>>> CompiledStaticCall::set_to_interpreted(...) but soon updated to refer
>>>> to the compiled version of the lambda form (-Xcomp).
>>>> 
>>>> Because tiered compilation is enabled, the VM decides to compile the
>>>> LambdaForm at a different level and sets the old version to
>>>> not-entrant. The next call through the IC therefore triggers a
>>>> re-resolving of the call site finally performing a Java upcall to
>>>> java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call
>>>> sequence [1]). A *new* LambdaForm is returned and
>>>> CompiledStaticCall::set_to_interpreted(...) is executed again to
>>>> update the IC. The assert is hit because the callee differs.
>>>> 
>>>> The problem is that there are multiple LambdaForms created for the
>>>> same invokeExact instruction. Caching in the Java runtime code should
>>>> guarantee that only one LambdaForm is created and reused.
>>>> Instrumenting Invokers::invokeHandleForm(...) shows that almost all
>>>> requests result in a cache miss and return a new LambdaForm.
>>>> 
>>>> This behaviour is caused by a race condition in
>>>> Invokers::invokeHandleForm(...). Threads may initially see a cache
>>>> miss when invoking MethodTypeForm::cachedLambdaForm(...), then create
>>>> a new LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to
>>>> update the cache without checking it again. In a high concurrency
>>>> setting, this leads to multiple LambdaForms being created for the
>>>> same invokeExact instruction because the cache entry is overwritten
>>>> by multiple threads.
>>>> 
>>>> *Solution*
>>>> Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/
>>>> 
>>>> An AtomicReferenceArray is used to cache the LambdaForms and
>>>> .get(...) and .compareAndSet(...) are used to retrieve and update the
>>>> cache entries. This allows only one thread to set the LambdaForm that
>>>> is then being used by all instances of the invokeExact.
>>>> 
>>>> *Testing*
>>>> - Failing test (vm/mlvm/meth/stress/jni/nativeAndMH)
>>>> - Nashorn + Octane
>>>> - JPRT
>>>> 
>>>> Many thanks to Christian Thalinger and Vladimir Ivanov!
>>>> 
>>>> Best,
>>>> Tobias
>>>> 
>>>> [1] Call sequence of reresolving the IC target
>>>> SharedRuntime::handle_wrong_method(...)
>>>> -> SharedRuntime::reresolve_call_site(...)
>>>> -> SharedRuntime::find_callee_method(...)
>>>> -> SharedRuntime::find_callee_info_helper(...)
>>>> -> LinkResolver::resolve_invoke(...)
>>>> -> LinkResolver::resolve_invokehandle(...)
>>>> -> LinkResolver::resolve_invokehandle(...)
>>>> -> LinkResolver::lookup_polymorphic_method(...)
>>>> -> SystemDictionary::find_method_handle_invoker(...)
>>>> -> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...)
>>>> -> Invokers::methodHandleInvokeLinkerMethod(...)
>>>> -> Invokers::invokeHandleForm(...)
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> Backport?
>>> 
>>> 
>> 

Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types

[Christian Thalinger]

On Apr 3, 2014, at 9:44 PM, John Rose  wrote:

> On Apr 3, 2014, at 6:33 PM, Christian Thalinger 
>  wrote:
> 
>> Of course they are popular because these are the type names.  There is no 
>> type L; it’s an object.  I don’t understand why we have to use different 
>> names just because they are used in other namespaces.  This is not a C 
>> define.
> 
> They stand for JVM signatures as well as basic types.  The letters are 
> signature letters.  Can we move on from this?

Sure.  Push it.

> 
> — John
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types

[Christian Thalinger]

On Mar 26, 2014, at 8:01 AM, Vladimir Ivanov  
wrote:

> Here's a version with the new naming scheme:
> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03.naming
> 
> I like existing naming scheme and OBJECT/VOID/LONG/etc names are quite 
> popular(e.g. Wrapper & ASM (Opcodes) use them).

Of course they are popular because these are the type names.  There is no type 
L; it’s an object.  I don’t understand why we have to use different names just 
because they are used in other namespaces.  This is not a C define.

> So, I'm in favor of leaving it as is.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 3/26/14 12:24 AM, Christian Thalinger wrote:
>> + enum BasicType {
>> + L_TYPE('L', Object.class, Wrapper.OBJECT),  // all reference types
>> + I_TYPE('I', int.class,Wrapper.INT),
>> + J_TYPE('J', long.class,   Wrapper.LONG),
>> + F_TYPE('F', float.class,  Wrapper.FLOAT),
>> + D_TYPE('D', double.class, Wrapper.DOUBLE),  // all primitive types
>> + V_TYPE('V', void.class,   Wrapper.VOID);// not valid in all 
>> contexts
>> 
>> I would suggest to not name them X_TYPE but give them meaningful names like 
>> Int, Float, Void.  The enum BasicType already infers that it’s a type.  If 
>> you think it’s not clear that it’s a type just use BasicType.Double in that 
>> places.
>> 
>> On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov  
>> wrote:
>> 
>>> Updated version:
>>> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/
>>> 
>>> - changed the way how arrays of types are created:
>>>static final BasicType[] ALL_TYPES = BasicType.values();
>>>static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, 
>>> ALL_TYPES.length-1);
>>> 
>>> - added a test for BasicType (LambdaFormTest.testBasicType).
>>> 
>>> Best regards,
>>> Vladimir Ivanov
>>> 
>>> On 3/22/14 2:08 AM, Vladimir Ivanov wrote:
>>>> John, thanks for the feedback.
>>>> 
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02
>>>> 
>>>> Also moved LambdaForm.testShortenSignature() into a stand-alone unit test.
>>>> 
>>>> Best regards,
>>>> Vladimir Ivanov
>>>> 
>>>> On 3/21/14 10:54 PM, John Rose wrote:
>>>>> On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov
>>>>> mailto:vladimir.x.iva...@oracle.com>>
>>>>> wrote:
>>>>> 
>>>>>> Thanks for the feedback.
>>>>>> 
>>>>>> What do you think about the following:
>>>>>> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/
>>>>> 
>>>>> That looks nice.  Strong typing; who woulda' thunk it.  :-)
>>>>> 
>>>>> The uses of ".ordinal()" are the extra cost relative to using just small
>>>>> integers.  They seem totally reasonable in the code.
>>>>> 
>>>>> I suggest either wrapping "ordinal" as something like "id" or else
>>>>> changing temp names like "id" to "ord", to reinforce the use of a common
>>>>> name.
>>>>> 
>>>>> Don't make the enum class public.  And especially don't make the mutable
>>>>> arrays public:
>>>>> 
>>>>> +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE,
>>>>> J_TYPE, F_TYPE, D_TYPE, V_TYPE };
>>>>> +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE,
>>>>> J_TYPE, F_TYPE, D_TYPE };
>>>>> 
>>>>> — John
>>>>> 
>>>>> P.S.  That would only be safe if we had (what we don't yet) a notion of
>>>>> frozen arrays like:
>>>>> 
>>>>> +public static final BasicType final[] ALL_TYPES = { L_TYPE,
>>>>> I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE };
>>>>> +public static final BasicType final[] ARG_TYPES = { L_TYPE,
>>>>> I_TYPE, J_TYPE, F_TYPE, D_TYPE };
>>>>> 
>>>>> 
>>>>> 
>>>>> ___
>>>>> mlvm-dev mailing list
>>>>> mlvm-...@openjdk.java.net
>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>>> 
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
>> ___
>> mlvm-dev mailing list
>> mlvm-...@openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(S) : 8038186 : [TESTBUG] improvements of test j.l.i.MethodHandles

[Christian Thalinger]

On Mar 24, 2014, at 1:33 PM, Igor Ignatyev  wrote:

> Hi all,
> 
> Please review the patch:
> 
> Problems:
> - MethodHandlesTest::testCatchException() doesn't provide enough testing of 
> j.l.i.MethodHandles::catchException().
> - MethodHandlesTest contains more than 3k lines, an auxiliary code together 
> w/ a test code, many methods aren't connected w/ each other, etc. All this 
> leads to that it is very very hard to understand, maintain.

Yes, it is.

> 
> Fix:
> - the auxiliary code was moved to testlibray
> - the test code was moved to a separate subpackage

This is very nice.  Are there plans to do this for the other existing tests as 
well?

> - random signature is used for target and handler
> - added scenarios w/ different return types
> 
> webrev: http://cr.openjdk.java.net/~iignatyev/8038186/webrev.01/
> jbs: https://bugs.openjdk.java.net/browse/JDK-8038186
> -- 
> Igor
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types

[Christian Thalinger]

+ enum BasicType {
+ L_TYPE('L', Object.class, Wrapper.OBJECT),  // all reference types
+ I_TYPE('I', int.class,Wrapper.INT),
+ J_TYPE('J', long.class,   Wrapper.LONG),
+ F_TYPE('F', float.class,  Wrapper.FLOAT),
+ D_TYPE('D', double.class, Wrapper.DOUBLE),  // all primitive types
+ V_TYPE('V', void.class,   Wrapper.VOID);// not valid in all 
contexts

I would suggest to not name them X_TYPE but give them meaningful names like 
Int, Float, Void.  The enum BasicType already infers that it’s a type.  If you 
think it’s not clear that it’s a type just use BasicType.Double in that places.

On Mar 24, 2014, at 9:29 AM, Vladimir Ivanov  
wrote:

> Updated version:
> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.03/
> 
> - changed the way how arrays of types are created:
>static final BasicType[] ALL_TYPES = BasicType.values();
>static final BasicType[] ARG_TYPES = Arrays.copyOf(ALL_TYPES, 
> ALL_TYPES.length-1);
> 
> - added a test for BasicType (LambdaFormTest.testBasicType).
> 
> Best regards,
> Vladimir Ivanov
> 
> On 3/22/14 2:08 AM, Vladimir Ivanov wrote:
>> John, thanks for the feedback.
>> 
>> Updated webrev:
>> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.02
>> 
>> Also moved LambdaForm.testShortenSignature() into a stand-alone unit test.
>> 
>> Best regards,
>> Vladimir Ivanov
>> 
>> On 3/21/14 10:54 PM, John Rose wrote:
>>> On Mar 21, 2014, at 8:49 AM, Vladimir Ivanov
>>> mailto:vladimir.x.iva...@oracle.com>>
>>> wrote:
>>> 
 Thanks for the feedback.
 
 What do you think about the following:
 http://cr.openjdk.java.net/~vlivanov/8037210/webrev.01/
>>> 
>>> That looks nice.  Strong typing; who woulda' thunk it.  :-)
>>> 
>>> The uses of ".ordinal()" are the extra cost relative to using just small
>>> integers.  They seem totally reasonable in the code.
>>> 
>>> I suggest either wrapping "ordinal" as something like "id" or else
>>> changing temp names like "id" to "ord", to reinforce the use of a common
>>> name.
>>> 
>>> Don't make the enum class public.  And especially don't make the mutable
>>> arrays public:
>>> 
>>> +public static final BasicType[] ALL_TYPES = { L_TYPE, I_TYPE,
>>> J_TYPE, F_TYPE, D_TYPE, V_TYPE };
>>> +public static final BasicType[] ARG_TYPES = { L_TYPE, I_TYPE,
>>> J_TYPE, F_TYPE, D_TYPE };
>>> 
>>> — John
>>> 
>>> P.S.  That would only be safe if we had (what we don't yet) a notion of
>>> frozen arrays like:
>>> 
>>> +public static final BasicType final[] ALL_TYPES = { L_TYPE,
>>> I_TYPE, J_TYPE, F_TYPE, D_TYPE, V_TYPE };
>>> +public static final BasicType final[] ARG_TYPES = { L_TYPE,
>>> I_TYPE, J_TYPE, F_TYPE, D_TYPE };
>>> 
>>> 
>>> 
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types

[Christian Thalinger]

On Mar 18, 2014, at 2:35 PM, John Rose  wrote:

> On Mar 18, 2014, at 1:36 PM, Christian Thalinger 
>  wrote:
> 
>> Why are we not using an Enum instead of an "untyped" byte?
> 
> Byte is moderately typed, in the sense (which I rely on during development) 
> that you can't assign an int or char to a byte w/o a cast.
> That's why it is not just a plain int.
> 
> But those values (L_TYPE etc.) are used a lot as numbers, and specifically as 
> low-level array indexes, and also comparisons (x < V_TYPE).
> 
> To turn them into enums, we'd have to add lots of calls to '.ordinal()' to 
> turn them right back to numbers.  That dilutes (completely IMO) the value 
> they have as enums to raise the level of the code.

But without being strongly typed we get bugs like this one:
 @Override
-MethodHandle bindArgument(int pos, char basicType, Object value) {
+MethodHandle bindArgument(int pos, byte basicType, Object value) {
 // If the member needs dispatching, do so.
 if (pos == 0 && basicType == 'L') {
I’m just saying that for the sake of maintainability and correctness an Enum 
would be better.

> 
> — John
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (L) 8037210: Get rid of char-based descriptions 'J' of basic types

[Christian Thalinger]

On Mar 14, 2014, at 4:28 AM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8037210/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8037210
> 953 lines changed: 425 ins; 217 del; 311 mod
> 
> This is a massive cleanup of JSR292 code to replace char-based description of 
> basic types by numeric constants.

Why are we not using an Enum instead of an "untyped" byte?

> 
> Contributed-by: john.r.r...@oracle.com
> 
> Testing: jdk/java/{lang/invoke,util}, vm.mlvm.testlist, nashorn, jruby
> Flags: -ea -esa -Xverify:all -D...COMPILE_THRESHOLD={0,30}
> 
> Best regards,
> Vladimir Ivanov
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: [9] RFR (S): 8036117: MethodHandles.catchException doesn't handle VarargsCollector right (8034120 failed)

[Christian Thalinger]

Even better.

On Mar 10, 2014, at 3:21 PM, Vladimir Ivanov  
wrote:

> Chris, thanks for the review.
> 
> John suggested an elegant way to fix the problem - use asFixedArity.
> 
> Updated fix:
> http://cr.openjdk.java.net/~vlivanov/8036117/webrev.01/
> 
> Best regards,
> Vladimir Ivanov
> 
> On 3/8/14 4:51 AM, Christian Thalinger wrote:
>> Seems good to me.  I’d like to have another name for this method:
>> 
>> + private static Object invokeCustom(MethodHandle target, Object... 
>> args) throws Throwable {
>> 
>> On Mar 4, 2014, at 12:00 PM, Vladimir Ivanov  
>> wrote:
>> 
>>> http://cr.openjdk.java.net/~vlivanov/8036117/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8036117
>>> 84 lines changed: 74 ins; 3 del; 7 mod
>>> 
>>> I have to revert a cleanup I did for 8027827.
>>> MethodHandle.invokeWithArguments (and generic invocation) has unpleasant
>>> peculiarity in behavior when used with VarargsCollector. So,
>>> unfortunately, invokeWithArguments is not an option there.
>>> 
>>> Looking at the API (excerpts from javadoc [1] [2]), the following
>>> condition doesn't hold in that case:
>>>   "trailing parameter type of the caller is a reference type identical
>>> to or assignable to the trailing parameter type of the adapter".
>>> 
>>> Example:
>>>   target.invokeWithArguments((Object[])args)
>>>   =>
>>>   target.invoke((Object)o1,(Object)o2,(Object)o3)
>>>   =/>
>>>   target.invokeExact((Object)o1, (Object)o2, (Object[])o3)
>>> 
>>> because Object !<: Object[].
>>> 
>>> The fix is to skip unnecessary conversion when invoking a method handle
>>> and just do a pairwise type conversion.
>>> 
>>> Testing: failing test case, nashorn w/ experimental features (octane)
>>> 
>>> Thanks!
>>> 
>>> Best regards,
>>> Vladimir Ivanov
>>> 
>>> [1] MethodHandle.invokeWithArguments
>>> "Performs a variable arity invocation, ..., as if via an inexact invoke
>>> from a call site which mentions only the type Object, and whose arity is
>>> the length of the argument array."
>>> 
>>> [2] MethodHandle.asVarargsCollector
>>> "When called with plain, inexact invoke, if the caller type is the same
>>> as the adapter, the adapter invokes the target as with invokeExact.
>>> (This is the normal behavior for invoke when types match.)
>>> 
>>> Otherwise, if the caller and adapter arity are the same, and the
>>> trailing parameter type of the caller is a reference type identical to
>>> or assignable to the trailing parameter type of the adapter, the
>>> arguments and return values are converted pairwise, as if by asType on a
>>> fixed arity method handle.
>>> 
>>> Otherwise, the arities differ, or the adapter's trailing parameter type
>>> is not assignable from the corresponding caller type. In this case, the
>>> adapter replaces all trailing arguments from the original trailing
>>> argument position onward, by a new array of type arrayType, whose
>>> elements comprise (in order) the replaced arguments."
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
>> ___
>> mlvm-dev mailing list
>> mlvm-...@openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: [9] RFR (S): 8036117: MethodHandles.catchException doesn't handle VarargsCollector right (8034120 failed)

[Christian Thalinger]

Seems good to me.  I’d like to have another name for this method:

+ private static Object invokeCustom(MethodHandle target, Object... args) 
throws Throwable {

On Mar 4, 2014, at 12:00 PM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8036117/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8036117
> 84 lines changed: 74 ins; 3 del; 7 mod
> 
> I have to revert a cleanup I did for 8027827. 
> MethodHandle.invokeWithArguments (and generic invocation) has unpleasant 
> peculiarity in behavior when used with VarargsCollector. So, 
> unfortunately, invokeWithArguments is not an option there.
> 
> Looking at the API (excerpts from javadoc [1] [2]), the following 
> condition doesn't hold in that case:
>   "trailing parameter type of the caller is a reference type identical 
> to or assignable to the trailing parameter type of the adapter".
> 
> Example:
>   target.invokeWithArguments((Object[])args)
>   =>
>   target.invoke((Object)o1,(Object)o2,(Object)o3)
>   =/>
>   target.invokeExact((Object)o1, (Object)o2, (Object[])o3)
> 
> because Object !<: Object[].
> 
> The fix is to skip unnecessary conversion when invoking a method handle 
> and just do a pairwise type conversion.
> 
> Testing: failing test case, nashorn w/ experimental features (octane)
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> 
> [1] MethodHandle.invokeWithArguments
> "Performs a variable arity invocation, ..., as if via an inexact invoke 
> from a call site which mentions only the type Object, and whose arity is 
> the length of the argument array."
> 
> [2] MethodHandle.asVarargsCollector
> "When called with plain, inexact invoke, if the caller type is the same 
> as the adapter, the adapter invokes the target as with invokeExact. 
> (This is the normal behavior for invoke when types match.)
> 
> Otherwise, if the caller and adapter arity are the same, and the 
> trailing parameter type of the caller is a reference type identical to 
> or assignable to the trailing parameter type of the adapter, the 
> arguments and return values are converted pairwise, as if by asType on a 
> fixed arity method handle.
> 
> Otherwise, the arities differ, or the adapter's trailing parameter type 
> is not assignable from the corresponding caller type. In this case, the 
> adapter replaces all trailing arguments from the original trailing 
> argument position onward, by a new array of type arrayType, whose 
> elements comprise (in order) the replaced arguments."
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S): 8033666: Make sure @ForceInline is everywhere it needs to be in sun.misc and java.lang.invoke

[Christian Thalinger]

Looks good.

While touching the code some time ago John and I were playing around with the 
idea to call Class.cast in the failing case:

+ static  T castReference(Class t, U x) {
+ // inlined Class.cast because we can't ForceInline it
+ if (x != null && !t.isInstance(x))
+ t.cast(x);
+ return (T) x;
+ }
+ 

Then we don’t have to duplicate the throwing logic.

On Feb 25, 2014, at 4:16 PM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8033666/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8033666
> 
> ValueConversions::castReference was introduced to workaround a problem 
> with unreliable inlining of Class::cast method. Unfortunately, since 
> ValueConversions class is located in sun.invoke.util, 
> @java.lang.invoke.ForceInline annotation isn't visible to it.
> 
> A proper fix would be to teach Hotspot to treat Class::cast specifically 
> and always inline it, but it requires extensive exploration of 
> performance implications. Filed 8035809 [1] to track that.
> 
> As an interim fix, I moved castReference method into 
> java.lang.invoke.MethodHandleImpl class and added new entry point 
> ValueConversions::cast which accepts a method handle to a method which 
> should be used for casting.
> 
> Testing: manual (looked through -XX:+PrintInlining output to ensure 
> MHI::castReference is inlined), jdk/java/lang/invoke, octane.
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> 
> [1] 8035809: Improve inlining of Class::cast method
> https://bugs.openjdk.java.net/browse/JDK-8035809
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: [9] RFR (M): 8027827: Improve performance of catchException combinator

[Christian Thalinger]

On Feb 20, 2014, at 9:57 AM, Vladimir Ivanov  
wrote:

> Paul,
> 
> Thanks for the feedback! See my answers inline.
> 
> Updated webrev:
> http://cr.openjdk.java.net/~vlivanov/8027827/final/webrev.01/
> 
> I finally figured out how to make caching work. This webrev contains 
> these changes.
> 
> I changed LF representation a bit and added 2 auxiliary method handles 
> - argument boxing and wrapping into Object[] and result unboxing. These 
> operations depend on actual type and can't be shared among arbitrary 
> combinators with the same basic type. They are used only during LF 
> interpretation and are completely ignored in compiled LFs.
> 
> On 2/20/14 7:51 PM, Paul Sandoz wrote:
>> Hi Vladimir,
>> 
>> I know just enough about this area to be dangerous
>> 
>> 
>> src/share/classes/java/lang/invoke/BoundMethodHandle.java
>> 
>>  865 private static final SpeciesData[] SPECIES_DATA_CACHE = new 
>> SpeciesData[4];
>> 
>> 
>> Only 3 elements are stored in the above array?
> Fixed.
> 
>> 
>> 
>> 
>> src/share/classes/java/lang/invoke/MethodHandleImpl.java
>> 
>>  634 // t_{i+2}:L=ValueConversions.unbox(t_{i+1}) OR 
>> ValueConversions.identity(t_{i+1})
>>  635 if (type.returnType().isPrimitive()) {
>>  636 names[UNBOX_RESULT] = new 
>> Name(ValueConversions.unbox(type.returnType()),
>>  637   names[TRY_CATCH]);
>>  638 } else {
>>  639 names[UNBOX_RESULT] = new Name(ValueConversions.identity(),
>>  640   names[TRY_CATCH]);
>>  641 }
>> 
>> 
>> You could create the form without the identity transform for the 
>> non-primitive case?
>> 
>>   final int UNBOX_RESULT = type.returnType().isPrimitive() ? nameCursor++ : 
>> 0;
>> ...
>> 
>>   if (UNBOX_RESULT > 0) { ...
>>   names[UNBOX_RESULT] = new 
>> Name(ValueConversions.unbox(type.returnType()), names[TRY_CATCH]);
>>   }
> I can, but it complicates matching and compiling the pattern in 
> InvokerBytecodeGenerator. I decided to keep the shape uniform for all cases.
> 
>> 
>> 
>>  699 try {
>>  700 GUARD_WITH_CATCH
>>  701 = IMPL_LOOKUP.findStatic(MethodHandleImpl.class, 
>> "guardWithCatch",
>>  702 MethodType.methodType(Object.class, 
>> MethodHandle.class, Class.class, MethodHandle.class, Object[].class));
>>  703 } catch (ReflectiveOperationException ex) {
>>  704 throw new RuntimeException(ex);
>>  705 }
>>  706 return GUARD_WITH_CATCH;
>> 
>> 
>> Should #704 be:
>> 
>>   throw newInternalError(ex);
>> 
>> ?
> Fixed. Also updated other places where RuntimeException was thrown.

A tiny bit of history here on newInternalError:  in JDK 8 we added new 
constructors that take a cause as argument; JDK 7 doesn’t have them.  In order 
to make java.lang.invoke backporting from 8 to 7 easier we used a wrapper 
method to instantiate InternalErrors.

> 
>> 
>> 
>> test/java/lang/invoke/MethodHandles/TestCatchException.java
>> 
>>  100 Object o = new Object();
>>  101 Object[] obj1 = new Object[] { "str" };
>>  102
>>  103 Object r1 = target.invokeExact(o, o, o, o, o, o, o, o, obj1);
>>  104 Object r2 = gwc.invokeExact(o, o, o, o, o, o, o, o, obj1);
>>  105 assertTrue(r1 != null);
>>  106 assertTrue(r1.equals(r2));
>> 
>> To be on the safe side should probably assert as follows:
>> 
>>   assertEquals(r1, obj);
>>   assertEquals(r2, obj);
> Fixed.
> 
> Best regards,
> Vladimir Ivanov
> 
>> Paul.
>> 
>> 
>> On Feb 19, 2014, at 10:46 PM, Vladimir Ivanov  
>> wrote:
>> 
>>> http://cr.openjdk.java.net/~vlivanov/8027827/final/webrev.00
>>> https://jbs.oracle.com/bugs/browse/JDK-8027827
>>> 354 lines changed: 193 ins; 91 del; 70 mod
>>> 
>>> OVERVIEW
>>> 
>>> MethodHandles.catchException combinator implementation is based on generic 
>>> invokers (MethodHandleImpl$GuardWithCatch.invoke_*). It is significantly 
>>> slower than a Java equivalent (try-catch).
>>> 
>>> Future Nashorn performance improvements require catchException combinator 
>>> speed to be on par with try-catch in Java.
>>> 
>>> So, it should be represented in a more efficient form.
>>> 
>>> I chose the following lambda form representation:
>>> 
>>>  t_{i}:L=ValueConversions.array(a1:L,...,a_{k}:L);
>>> t_{i+1}:L=MethodHandleImpl.guardWithCatch(t_{p1}, t_{p2}, t_{p3}, t_{i}:L);
>>> t_{i+2}:I=ValueConversions.unbox(t7:L);
>>> OR :L=ValueConversions.identity(t_{n+1})
>>> OR :V=ValueConversions.ignore(t_{n+1})
>>> 
>>> where:
>>>  a1, ..., a_{k} - arguments
>>>  t_{p1}, t_{p2}, t_{p3} - target method handle, exception class, catcher 
>>> method handle respectively; passed as bounded parameters;
>>> 
>>> During lambda form compilation it is converted into bytecode equivalent of 
>>> the following Java code:
>>>  try {
>>>  return target.invokeBasic(...);
>>>  } catch(Throwable e) {
>>>  if (!exClass.isInst

Re: [8] RFR (S): 8033278: Missed access checks for Lookup.unreflect* after 8032585

[Christian Thalinger]

Looks good.

On Jan 30, 2014, at 4:58 PM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8033278/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8033278
> 
> The fix for 8032585 [1] introduced a regression: in some cases access 
> check on a reference class is omitted.
> 
> The fix is to ensure that access check on a reference class is always 
> performed.
> 
> Testing: regression test, jdk/test/java/lang/invoke/, 
> jdk/test/java/util/stream, vm.defmeth.testlist, vm.mlvm.testlist, 
> nashorn (unit tests, octane), groovy
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> 
> [1] http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: [8] RFR (XS): 8032585: JSR292: IllegalAccessError when attempting to invoke protected method from different package

[Christian Thalinger]

Looks good.

On Jan 27, 2014, at 8:05 AM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8032585/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8032585
> 
> JSR292 access verification logic refuses method handle lookup access to 
> methods which are defined on inaccessible classes. This is usually 
> correct, but in the corner case of inheritance through a public class, 
> it is wrong. 8029507 makes the JVM provide more correct information 
> about the defining class of a looked-up method and this corrected 
> information is causing the old and wrong checks to fail where they 
> didn't fail before.
> 
> The fix is to relax the check: don't require the class where protected 
> member is declared to be public. It is enough to check that defining 
> class is a super class of the class lookup request comes from to ensure 
> there are enough privileges to access protected member.
> 
> Testing: regression test, enumeration tests on access checks, 
> jdk/test/java/lang/invoke, vm.mlvm.testlist
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

[9] RFR (XXS): 8031043: ClassValue's backing map should have a smaller initial size

[Christian Thalinger]

https://bugs.openjdk.java.net/browse/JDK-8031043
http://cr.openjdk.java.net/~twisti/8031043/webrev.00

8031043: ClassValue's backing map should have a smaller initial size
Reviewed-by:

The current initial size for ClassValue's backing WeakHashMap (ClassValueMap) 
is: 

private static final int INITIAL_ENTRIES = 32; 

This is too big and wastes a lot of memory. Usually a dynamic language or other 
users of ClassValue associate only one value with a Class. Even if users need 
more entries it's better to start small and grow as needed since adding new 
values to a Class is a one-time thing and not performance critical.

Here is some discussion on the mlvm-dev list:

http://mail.openjdk.java.net/pipermail/mlvm-dev/2014-January/005597.html

Re: [JDK8] RFR (XS): JSR292: IncompatibleClassChangeError in LambdaForm for CharSequence.toString() method handle type converter

[Christian Thalinger]

On Jan 15, 2014, at 11:41 AM, Vladimir Ivanov  
wrote:

> Chris,
> 
> Thanks for looking into this. See my answers inline.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 1/15/14 9:51 PM, Christian Thalinger wrote:
>> [I’m resending something I sent earlier today to Vladimir directly.]
>> 
>> On Jan 15, 2014, at 7:31 AM, Vladimir Ivanov  
>> wrote:
>> 
>>> http://cr.openjdk.java.net/~vlivanov/8031502/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8031502
>>> 
>>> InvokeBytecodeGenerator can produce incorrect bytecode for a LambdaForm
>>> when invoking a method from Object declared in an interface.
>>> 
>>> The problem is the following:
>>>   (1) java.lang.CharSequence interface declares abstract method "String
>>> toString()";
>>> 
>>>   (2) after 8014013 fix, VM resolves
>>> CharSequence::toString()/invokeInterface to
>>> CharSequence::toString()/invokeVirtual;
>> 
>> Without having looked at the changes of 8014013, why did the invoke type 
>> change?  Is it an optimization that was added?
>> 
> 
> After 8014013, LinkResolver::resolve_interface_call returns virtual 
> (_call_kind = CallInfo::vtable_call), instead of interface method and 
> MethodHandles::init_method_MemberName uses it as is (without any fine 
> tuning, which was done before).
> 
> It's caused by the following:
>   - LinkResolver::linktime_resolve_interface_method returns 
> CharSequence::toString method, but it has vtable index, instead of 
> itable index;
> 
>   - LinkResolver::runtime_resolve_interface_method looks at resolved 
> method and sets _call_kind to vtable_call, since resolved method doesn't 
> have itable index.
> 
>   - then MethodHandles::init_method_MemberName looks at CallInfo passed 
> in and sets the reference kind flag to JVM_REF_invokeVirtual.
> 
> That's how the conversion from invokeInterface to invokeVirtual occurs.
> 
> I did a quick debugging session with pre-8014013 hotspot to check how it 
> worked before, but don't remember all the details now.
> 
>>> 
>>>   (3) during LambdaForm compilation, CharSequence is considered
>>> statically invocable (see
>>> InvokeBytecodeGenerator::isStaticallyInvocable) and invokevirtual for
>>> CharSequence::toString() is issued, which is wrong (invokevirtual throws
>>> ICCE if it references an interface);
>>> 
>>> The fix is straightforward: during LambdaForm compilation, switch back
>>> from invokevirtual to invokeinterface instruction when invoking a method
>>> on an interface.
>> 
>> I find this risky.  Right now MemberName is only used in a couple places in 
>> java.lang.invoke but in the future it might be used for other things (e.g. 
>> java.lang.reflect).  The information MemberName contains should be correct 
>> and usable without changing.  Otherwise all users have to patch the 
>> information the same way as you do in your patch.
>> 
>> I would like to see the VM passing correct information (whatever the 
>> definition of correct is here).
>> 
> 
> It's an interesting question what kind of correctness is required for 
> MemberName and I don't know the answer. Looking through the code, I got 
> an impression MemberName isn't designed to provide information to be 
> used "as is" for bytecode generation, because, at least:
>   - MemberName::referenceKindIsConsistent already expects 
> (clazz.isInterface() && refKind == REF_invokeVirtual && 
> isObjectPublicMethod()) case;
> 
>   - InvokeBytecodeGenerator::emitStaticInvoke already has a special 
> case for REF_invokeSpecial referenceKind, converting it to 
> REF_invokeVirtual;

John would know the answer.

Given this change should go into JDK 8 I think we should push for now.  If we 
can come up with a better way to handle these situations we can push another 
change for 9 or 8u20.

> 
> Best regards,
> Vladimir Ivanov
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: [JDK8] RFR (XS): JSR292: IncompatibleClassChangeError in LambdaForm for CharSequence.toString() method handle type converter

[Christian Thalinger]

[I’m resending something I sent earlier today to Vladimir directly.]

On Jan 15, 2014, at 7:31 AM, Vladimir Ivanov  
wrote:

> http://cr.openjdk.java.net/~vlivanov/8031502/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8031502
> 
> InvokeBytecodeGenerator can produce incorrect bytecode for a LambdaForm 
> when invoking a method from Object declared in an interface.
> 
> The problem is the following:
>   (1) java.lang.CharSequence interface declares abstract method "String 
> toString()";
> 
>   (2) after 8014013 fix, VM resolves 
> CharSequence::toString()/invokeInterface to 
> CharSequence::toString()/invokeVirtual;

Without having looked at the changes of 8014013, why did the invoke type 
change?  Is it an optimization that was added?

> 
>   (3) during LambdaForm compilation, CharSequence is considered 
> statically invocable (see 
> InvokeBytecodeGenerator::isStaticallyInvocable) and invokevirtual for 
> CharSequence::toString() is issued, which is wrong (invokevirtual throws 
> ICCE if it references an interface);
> 
> The fix is straightforward: during LambdaForm compilation, switch back 
> from invokevirtual to invokeinterface instruction when invoking a method 
> on an interface.

I find this risky.  Right now MemberName is only used in a couple places in 
java.lang.invoke but in the future it might be used for other things (e.g. 
java.lang.reflect).  The information MemberName contains should be correct and 
usable without changing.  Otherwise all users have to patch the information the 
same way as you do in your patch.

I would like to see the VM passing correct information (whatever the definition 
of correct is here).

> 
> The fix is targeted for 8. Will be also integrated into 9.
> 
> Testing: regression test, jdk/test/java/lang/invoke, vm.mlvm.testlist, 
> nashorn, jruby.
> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

hg: jdk8/tl/jdk: 8026502: java/lang/invoke/MethodHandleConstants.java fails on all platforms

[christian . thalinger]

Changeset: 66884b270b44
Author:twisti
Date:  2013-10-24 10:52 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/66884b270b44

8026502: java/lang/invoke/MethodHandleConstants.java fails on all platforms
Reviewed-by: iveresov, jrose

! src/share/classes/java/lang/invoke/MethodHandles.java

Re: Review Request for 8025799: Restore sun.reflect.Reflection.getCallerClass(int)

[Christian Thalinger]

Unfortunate but I understand.  It might be a good idea to add a 
getCallerClass(-1) call to the test case.

On Oct 7, 2013, at 1:24 AM, Mandy Chung  wrote:

> JDK 8 was feature complete in June and there just isn't sufficient time 
> remaining to get agreement and feedback on an API to examine the caller 
> frames. To that end, I propose to restore the old unsupported 
> Reflection.getCallerClass(int) and that we will look to define a standard API 
> for JDK 9.
> 
> Webrev at:
>  http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8025799/
> 
> It remains to be an unsupported API and JDK should not use this method and 
> it's not annotated with @CallerSensitive.  I considered detecting if this 
> method is called by a system class (loaded by null loader) and throw an 
> error.  I decided to minimize the compatibility risk in case if there is any 
> existing code added to the bootclasspath depending on this private API.
> 
> Thanks
> Mandy

Re: RFR (M) 8001110: method handles should have a collectArguments transform, generalizing asCollector

[Christian Thalinger]

On Oct 4, 2013, at 2:40 PM, John Rose  wrote:

> Actually it's OK:  The name "coll" is defined a couple lines up by the "A 
> collection adapter {@code collectArguments(mh, 0, coll)} ...", and the term 
> "filter" is persistently applied to it. So I think it is intelligible as 
> posted.  — John

I'm fine with that.

> 
> On Oct 4, 2013, at 11:34 AM, John Rose  wrote:
> 
>> Yikes; good catch.  I used javac -Xdoclint to find a couple typos in @param 
>> also.  — John
>> 
>> On Oct 4, 2013, at 11:17 AM, Christian Thalinger 
>>  wrote:
>> 
>>> You have renamed "coll" to "filter" but the documentation still references 
>>> "coll" in multiple places, e.g.:
>>> 
>>> + * If the filter method handle {@code coll} consumes one argument and 
>>> produces
>>> + * a non-void result, then {@code collectArguments(mh, N, coll)}
>>> + * is equivalent to {@code filterArguments(mh, N, coll)}.
>> 
>> ___
>> mlvm-dev mailing list
>> mlvm-...@openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (M) 8001110: method handles should have a collectArguments transform, generalizing asCollector

[Christian Thalinger]

src/share/classes/java/lang/invoke/MethodHandles.java:

You have renamed "coll" to "filter" but the documentation still references 
"coll" in multiple places, e.g.:

+ * If the filter method handle {@code coll} consumes one argument and 
produces
+ * a non-void result, then {@code collectArguments(mh, N, coll)}
+ * is equivalent to {@code filterArguments(mh, N, coll)}.

Otherwise this looks good.

On Sep 12, 2013, at 7:55 PM, John Rose  wrote:

> Please review this change for a change to the JSR 292 implementation:
> 
> http://cr.openjdk.java.net/~jrose/8001110/webrev.00/
> 
> Summary: promote an existing private method; make unit tests on all argument 
> positions to arity 10 with mixed types
> 
> The change is to javadoc and unit tests, documenting and testing some corner 
> cases of JSR 292 APIs.
> 
> Bug Description:  Currently, a method handle can be transformed so that 
> multiple arguments are collected and passed to the original method handle. 
> However, the two routes to doing this are both limited to special purposes. 
> 
> (They are asCollector, which collects only trailing arguments, and only into 
> an array; and foldArguments, which collects only leading arguments into 
> filter function, and passes both the filtered result *and* the original 
> arguments to the original.)
> 
> MethodHandles.collectArguments(mh, pos, collector) should produce a method 
> handle which acts like lambda(a*, b*, c*) { x = collector(b*); return mh(a*, 
> x, c*) }, where the span of arguments b* is located by pos and the arity of 
> the collector.
> 
> There is internal machinery in any JSR 292 implementation to do this. It 
> should be made available to users.
> 
> This is a missing part of the JSR 292 spec.
> 
> Thanks,
> — John
> 
> P.S. Since this is a change which oriented toward JSR 292 functionality, the 
> review request is to mlvm-dev and core-libs-dev.
> Changes which are oriented toward performance will go to mlvm-dev and 
> hotspot-compiler-dev.
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S) 8024599: JSR 292 direct method handles need to respect initialization rules for static members

[Christian Thalinger]

Looks good.

On Oct 2, 2013, at 12:33 PM, John Rose  wrote:

> Push-button webrev generator to the rescue:
>  http://cr.openjdk.java.net/~jrose/8024599/webrev.01
> 
> — John
> 
> On Oct 2, 2013, at 11:23 AM, Christian Thalinger 
>  wrote:
> 
>> Since there is no new webrev I assume you incorporated all the stuff below.  
>> If that's the case then it looks good.
>> 
>> On Sep 20, 2013, at 6:18 PM, John Rose  wrote:
>> 
>>> On Sep 20, 2013, at 8:29 AM, Vladimir Ivanov  
>>> wrote:
>>> 
>>>> John,
>>>> 
>>>> I don't see much value in documenting buggy behavior of early JDK7 in JDK8 
>>>> code. So, I would remove it.
>>> 
>>> OK.  I think I had it in mainly to make sure the unit tests did something 
>>> interesting.
>> 
>>> 
>>>> Regarding the test:
>>>> 31  * @run main/othervm/timeout=3600
>>>> - why do you have timeout set to 1h?
>>> 
>>> Copy-and-paste from some other test.  Removed.
>>> 
>>>> I like the idea how you count events.
>>>> 
>>>> As a suggestion for enhancement - maybe it's more reliable to check the 
>>>> "type" of event as well? To ensure that particular class was initialized.
>>> 
>>> Good idea.  But since each unique init event is stored in a separate 
>>> variable, it's easy to check this without explicit event types.  I did the 
>>> following, for each of the six test cases:
>>> 
>>> @@ -150,9 +150,11 @@
>>>   }
>>> 
>>>   private static int runFoo() throws Throwable {
>>> +assertEquals(Init1Tick, 0);  // Init1 not initialized yet
>>>   int t1 = tick("runFoo");
>>>   int t2 = (int) INDY_foo().invokeExact();
>>>   int t3 = tick("runFoo done");
>>> +assertEquals(Init1Tick, t2);  // when Init1 was initialized
>>>   assertEquals(t1+2, t3);  // exactly two ticks in between
>>>   assertEquals(t1+1, t2);  // init happened inside
>>>   return t2;
>> 
>>> 
>>> 
>>> — John
>>> 
>>>> Best regards,
>>>> Vladimir Ivanov
>>>> 
>>>> On 9/20/13 1:38 AM, John Rose wrote:
>>>>> On Sep 12, 2013, at 7:24 PM, John Rose >>>> <mailto:john.r.r...@oracle.com>> wrote:
>>>>> 
>>>>>> Please review this change for a change to the JSR 292 implementation:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~jrose/8024599/webrev.00/
>>>>>> 
>>>>>> Summary: Align MH semantic with bytecode behavior of constructor and
>>>>>> static member accesses, regarding  invocation.
>>>>>> 
>>>>>> The change is to javadoc and unit tests, documenting and testing some
>>>>>> corner cases of JSR 292 APIs.
>>>>> 
>>>>> I have a reviewer (Alex Buckley) for the documentation changes, but I
>>>>> would also like a quick code review for the unit test.
>>>>> 
>>>>> Also, there is a code insertion (predicated on a "false" symbolic
>>>>> constant) which serves to document the buggy JDK 7 behavior.  I'm not
>>>>> particularly attached to it, so I'm open to either a yea or nay on
>>>>> keeping it.  Leaning nay at the moment.
>>>>> 
>>>>> — John
>>>>> 
>>>>> 
>>>>> ___
>>>>> mlvm-dev mailing list
>>>>> mlvm-...@openjdk.java.net
>>>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>>>> 
>>> 
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
>> ___
>> mlvm-dev mailing list
>> mlvm-...@openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (M) 8001110: method handles should have a collectArguments transform, generalizing asCollector

[Christian Thalinger]

On Sep 20, 2013, at 5:09 PM, John Rose  wrote:

> On Sep 20, 2013, at 3:07 PM, Vladimir Ivanov  
> wrote:
> 
>> I cleaned javadoc a little [1], so it is more readable in the browser now.
> 
> Thanks; I applied those edits.  I fixed the problem of a missing  in a few 
> other places too.
> 
>> The test code looks ok, though the logic is over-complicated.
>> But the whole MethodHandlesTest is written in the same vein.
> 
> Thanks.  (Looks like it wasn't written by a real test engineer.)

:-D  I try to not touch MethodHandlesTest.  We should think about splitting it 
into smaller test cases.

> 
> — John
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S) 8025112: JSR 292 spec updates for security manager and caller sensitivity

[Christian Thalinger]

Thank you for doing this; it is much clearer now.  Looks good.

On Oct 1, 2013, at 10:19 PM, John Rose  wrote:

> Chris Thalinger suggested removing the new booleans from the changed 
> "getDirectMethod" call sites and instead put the intended usage into the 
> method names, e.g., "getDirectMethodNoSecurityManager".  The result is more 
> clearly correct and maintainable.
> 
> Here is the respin:
>  http://cr.openjdk.java.net/~jrose/8025112/webrev.01
> 
> — John
> 
> On Oct 1, 2013, at 3:15 PM, John Rose  wrote:
> 
>> This change updates the javadoc to reflect previous changes in the behavior 
>> of the security manager, especially with respect to caller sensitivity.
>> 
>> It also adjusts some unit tests.
>> 
>> The key change is to the order of the security manager logic.  The purpose 
>> is to align the "bytecode behavior" of method handles more closely with the 
>> native behavior of the corresponding bytecode instructions.  This means that 
>> "fully trusted" method handles do not incur security checks if they are 
>> equivalent to bytecodes that the user could have written.
>> 
>> The API spec. and security rules have been internally reviewed.  This is a 
>> review of implementation and unit tests.
>> 
>> http://cr.openjdk.java.net/~jrose/8025112/webrev.00
>> 
>> For more background, see my JavaOne presentation:
>> http://cr.openjdk.java.net/~jrose/pres/201309-IndyUpdate.pdf
>> 
>> Thanks,
>> — John
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S) 8024599: JSR 292 direct method handles need to respect initialization rules for static members

[Christian Thalinger]

Since there is no new webrev I assume you incorporated all the stuff below.  If 
that's the case then it looks good.

On Sep 20, 2013, at 6:18 PM, John Rose  wrote:

> On Sep 20, 2013, at 8:29 AM, Vladimir Ivanov  
> wrote:
> 
>> John,
>> 
>> I don't see much value in documenting buggy behavior of early JDK7 in JDK8 
>> code. So, I would remove it.
> 
> OK.  I think I had it in mainly to make sure the unit tests did something 
> interesting.

> 
>> Regarding the test:
>> 31  * @run main/othervm/timeout=3600
>> - why do you have timeout set to 1h?
> 
> Copy-and-paste from some other test.  Removed.
> 
>> I like the idea how you count events.
>> 
>> As a suggestion for enhancement - maybe it's more reliable to check the 
>> "type" of event as well? To ensure that particular class was initialized.
> 
> Good idea.  But since each unique init event is stored in a separate 
> variable, it's easy to check this without explicit event types.  I did the 
> following, for each of the six test cases:
> 
> @@ -150,9 +150,11 @@
> }
> 
> private static int runFoo() throws Throwable {
> +assertEquals(Init1Tick, 0);  // Init1 not initialized yet
> int t1 = tick("runFoo");
> int t2 = (int) INDY_foo().invokeExact();
> int t3 = tick("runFoo done");
> +assertEquals(Init1Tick, t2);  // when Init1 was initialized
> assertEquals(t1+2, t3);  // exactly two ticks in between
> assertEquals(t1+1, t2);  // init happened inside
> return t2;

> 
> 
> — John
> 
>> Best regards,
>> Vladimir Ivanov
>> 
>> On 9/20/13 1:38 AM, John Rose wrote:
>>> On Sep 12, 2013, at 7:24 PM, John Rose >> > wrote:
>>> 
 Please review this change for a change to the JSR 292 implementation:
 
 http://cr.openjdk.java.net/~jrose/8024599/webrev.00/
 
 Summary: Align MH semantic with bytecode behavior of constructor and
 static member accesses, regarding  invocation.
 
 The change is to javadoc and unit tests, documenting and testing some
 corner cases of JSR 292 APIs.
>>> 
>>> I have a reviewer (Alex Buckley) for the documentation changes, but I
>>> would also like a quick code review for the unit test.
>>> 
>>> Also, there is a code insertion (predicated on a "false" symbolic
>>> constant) which serves to document the buggy JDK 7 behavior.  I'm not
>>> particularly attached to it, so I'm open to either a yea or nay on
>>> keeping it.  Leaning nay at the moment.
>>> 
>>> — John
>>> 
>>> 
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>>> 
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (M): 8024635: Caching MethodType's descriptor string improves lambda linkage performance

[Christian Thalinger]

Looks good.

On Sep 27, 2013, at 5:39 AM, Sergey Kuksenko  wrote:

> 
> Updated version at
> http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.02/
> according comments:
> - remove "null check" comment near Object.requireNonNull calls
> - distort/(change parameters order) in key-purposed MethodType constructor
> - move count-slot logic from checkPType to checkPTypes.
> 
> 
> 
> On 09/26/2013 11:09 PM, John Rose wrote:
>> (Quick note on format:  I have changed the subject line to include the 
>> conventional bug number and size estimate.  The bug number makes it easier 
>> to track in mailers.)
>> 
>> On Sep 26, 2013, at 9:22 AM, Sergey Kuksenko  
>> wrote:
>> 
>>> Hi All,
>>> 
>>> I updated fix.
>>> You may find it here
>>> http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.01/
>>> 
>>> See my comments and new webrev descition below
>>> 
>>> On 09/18/2013 12:59 AM, John Rose wrote:
>> We can tweak that later if necessary.  There are probably only a small 
>> number of such strings, so maybe a small leaky map would do the trick as 
>> well. 
> In case of small amount of such strings we will get a huge overhead from
> any kind of map.
 I would expect O(10^1.5) memory references and branches.  Depends on what 
 you mean by "huge".
>>> Sure. I think the question to use external map or field may be decided
>>> later when/if it will be needed.
>>> Just some statictics, I've collected on nashorn/octane benchmarks
>>> (statistics collected only for the single(first) benchmark iteration:
>>> mandreel: 514 unique strings, toMethodDescriptorString called 69047 times
>>> box2d: 560 unique strings, 34776 toMethodDescriptorString invocations.
>> 
>> Good data; thanks.
>> 
>>> 
 Would there be any benefit from recording the original string from the 
 constant pool?  The JVM path for this is 
 SystemDictionary::find_method_handle_type, which makes an upcall to 
 MethodHandleNatives.findMethodHandleType.  Currently only the ptypes and 
 rtype are passed, but the JVM could also convert the internal Symbol to a 
 java.lang.String and pass that also.  In that case, MT's created by JVM 
 upcalls would be pre-equipped with descriptor strings.
 
 This is probably worth an experiment, although it requires some JVM work.
>>> 
>>> I am definitely sure that we shouldn't do that.
>>> Right now caching desriptor strings is internal decision of MethodType.
>>> Otherwise it will make interfaces more complex.
>> 
>> Yes, I agree.  Also, it would only benefit the 514 calls which introduce 
>> unique strings, whereas your change helps the 69047 calls for descriptor 
>> strings.
>> 
 I hope you get my overall point:  Hand optimizations have a continuing 
 cost, related to obscuring the logical structure of the code.  The 
 transforming engineer might make a mistake, and later maintaining 
 engineers might also make mistakes.
 
 https://blogs.oracle.com/jrose/entry/three_software_proverbs
>>> 
>>> And it doesn't mean that we should afraid any kind of optimization.
>>> Lucky positions - someone(or something) will optimize it for us. But
>>> sometimes JIT (even smart JIT) is not enough.
>> 
>> Sometimes.  When that happens we reluctantly hand-optimize our code.
>> 
>>> Let's back to the patch.construstors
>>> I decided not change original checkPType/checkRType code, except
>>> explicit Objects.requireNonNull. The problem here that we are creating
>>> new MethodType objects which are already exists, we have to create MT as
>>> a key for searching in the internedTable. Ratio between already exiting
>>> and new MethodTypes a quite huge.
>>> Here is some statistics I've collected on nashorn/octane benchmarks
>>> (statistics collected only for the single(first) benchmark iteration:
>>> 
>>> mandreel:
>>> - 1739 unique MethodTypes,
>>> - 878414 MethodType.makeImpl requests;
>>> box2d:
>>> - 1861 unique MethodTypes,
>>> - 527486 MethodType.makeImpl requests;
>>> gameboy:
>>> - 2062 unique MethodTypes,
>>> - 311378 MethodType.makeImpl requests;
>>> 
>>> So
>>> 1. MethodType constructor do checkPTypes/checkRType which are frequently
>>> (99%) useless - we already have the same (checked) MethodType in the
>>> internTable.
>>> 2. Less than 1% of created MethodTypes will be stored into internTable,
>>> but reusing that MethodType objects prevent ti from scalar replacement.
>>> (I've heard that Graal may do flow-sensitive scalar replacement, but C2
>>> can't do).
>>> 
>>> What I did. I separate constructors:
>>> - for long lived MethodTypes with all checks,
>>> - for short lived MethodTypes used only as key for searching in the
>>> InterTable, no checks are performed.
>>> if we didn't find  MethodType in the table we always create a new one
>>> (with checks) that is required in less than 1% cases. And we remove at
>>> least one obstacle for scalar replacement. Unfortunately, scalaer
>>> replacement for expression "internTable.get(new MethodType(rtype,

Re: RFR (S) 8001108: an attempt to use "" as a method name should elicit NoSuchMethodException

[Christian Thalinger]

On Sep 19, 2013, at 2:31 PM, John Rose  wrote:

> 
> On Sep 18, 2013, at 5:05 PM, Christian Thalinger 
>  wrote:
> 
>> src/share/classes/java/lang/invoke/MethodHandles.java:
>> 
>> + * methods as if they were normal methods, but the JVM verifier rejects 
>> them.
>> 
>> I think you should say "JVM byte code verifier" here.
> 
> Done.  s/byte code/bytecode/.
> 
>> 
>> + * (Note:  JVM internal methods named {@code } not 
>> visible to this API,
>> + * even though the {@code invokespecial} instruction can refer to 
>> them
>> + * in special circumstances.  Use {@link #findConstructor 
>> findConstructor}
>> 
>> Not exactly sure but should this read "are not visible"?
> 
> Yes.
> 
>> 
>>MemberName resolveOrFail(byte refKind, Class refc, String name, 
>> MethodType type) throws NoSuchMethodException, IllegalAccessException {
>> +type.getClass();  // NPE
>>checkSymbolicClass(refc);  // do this before attempting to resolve
>> -name.getClass(); type.getClass();  // NPE
>> +checkMethodName(refKind, name);
>> 
>> Could you add a comment here saying that checkMethodName does an implicit 
>> null pointer check on name?  Maybe a comment for checkMethodName too.
> 
> Done.
> 
>> 
>> What was the problem with the null pointer exceptions?  Is it okay that we 
>> might throw another exception before null checking name?
> 
> Foo.  The reordering of those null checks seems to be a needless change that 
> crept in.  I'm going to keep them the way they are.
> 
> See updated webrev:
>  http://cr.openjdk.java.net/~jrose/8001108/webrev.01/

Looks good.

> 
> — John
> 
>> On Sep 12, 2013, at 6:47 PM, John Rose  wrote:
>> 
>>> Please review this change for a change to the JSR 292 implementation:
>>> 
>>> http://cr.openjdk.java.net/~jrose/8001108/webrev.00
>>> 
>>> Summary: add an explicit check for leading "<", upgrade the unit tests
>>> 
>>> The change is mostly to javadoc and unit tests, documenting and testing 
>>> some corner cases of JSR 292 APIs.
>>> 
>>> In the RI, there is an explicit error check.
>>> 
>>> Thanks,
>>> — John
>>> 
>>> P.S. Since this is a change which oriented toward JSR 292 functionality, 
>>> the review request is to mlvm-dev and core-libs-dev.
>>> Changes which are oriented toward performance will go to mlvm-dev and 
>>> hotspot-compiler-dev.
>>> ___
>>> mlvm-dev mailing list
>>> mlvm-...@openjdk.java.net
>>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
>> 
>> ___
>> mlvm-dev mailing list
>> mlvm-...@openjdk.java.net
>> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(S+M) / 8022701 Accessibility checking: InvocationTargetException is thrown instead of IllegalAccessError

[Christian Thalinger]

On Sep 19, 2013, at 9:57 AM, David Chase  wrote:

> Recommended changes made:
> 
> http://cr.openjdk.java.net/~drchase/8022701/webrev.04/

Looks good.  Thanks for using ASM.

> 
> Test with jtreg (for pass and for induced failure) on MacOS,
> not sure what additional other testing is desired since it is entirely in the 
> libraries.
> 
> I included code to handle the case of a broken field-referencing methodhandle,
> but did not test it because there's no syntax for these in Java, nor is their 
> creation
> well-documented.
> 
> David
> 
> 
> On 2013-09-13, at 12:02 AM, John Rose  wrote:
> 
>> On Sep 12, 2013, at 5:44 PM, David Chase  wrote:
>> 
>>> Do these sibling bugs have numbers?
>> 
>> Yes, 8022701.  I just updated the bug to explain their common genesis.
>> 
>>> And I take it you would like to see
>>> 
>>> 1) a test enhanced with ASM to do all 3 variants of this
>> 
>> Yes, please.  The case of "no such field" does not have a direct cause from 
>> lambda expressions AFAIK, but it can occur with "raw" 
>> CONSTANT_MethodHandles.  (It would be reasonable for javac to emit 
>> CONSTANT_MH's for fields in some very limited circumstances, but I'll bet it 
>> doesn't.)
>> 
>>> 2) DO attach the underlying cause
>> 
>> Yes.  Read the javadoc for ExceptionInInitializerError for an example of why 
>> users want the underlying cause for linkage errors.
>> 
>> (Golly, I wonder what happens if resolution of a CONSTANT_MethodHandle tries 
>> to touch a class with a booby-trapped .  I hope it's the case that 
>> the ExceptionInInitializerError just passes straight up the stack.  But if 
>> it were to get wrapped in a ROE of some sort, it would probably bubble up as 
>> an IAE.  Could be a charming exploration.  Actually, no, I don't want to go 
>> in there.  Getting all possible linkage errors correct is fine, but we have 
>> more important things to do.)
>> 
>> — John
>> 
>>> David
>>> 
>>> On 2013-09-12, at 5:35 PM, John Rose  wrote:
>>> 
>>>> It looks good.  I have three requests.
>>>> 
>>>> Regarding this comment:
>>>> + * See MethodSupplier.java to see how to produce these bytes.
>>>> + * They encode that class, except that method m is private, not 
>>>> public.
>>>> 
>>>> The recipe is incomplete, since it does not say which bits to tweak to 
>>>> make m private.  Please add that step to the comments more explicitly, or 
>>>> if possible to the code (so bogusMethodSupplier is a clean copy of the od 
>>>> output).  I suppose you could ask sed to change the byte for you.  That 
>>>> will make this test a better example for future tests, and make it easier 
>>>> to maintain if javac output changes.  The high road to use would be asm, 
>>>> although for a single byte tweak od works OK.
>>>> 
>>>> Also, this bug has two twins.  The same thing will happen with 
>>>> NoSuchMethodE* and NoSuchFieldE*.  Can you please make fixes for those 
>>>> guys also?
>>>> 
>>>> FTR, see MemberName.makeAccessException() for logic which maps the other 
>>>> way, from *Error to *Exception.  I don't see a direct way to avoid the 
>>>> double mapping (Error=>Exception=>Error) when it occurs.
>>>> 
>>>> For the initCause bit we should do this:
>>>> 
>>>> } catch (IllegalAccessException ex) {
>>>> Error err = new IllegalAccessError(ex.getMessage());
>>>> err.initCause(ex.getCause());  // reuse underlying cause of ex
>>>> throw err;
>>>> } ... and for NSME and NSFE...
>>>> 
>>>> That way users can avoid the duplicate exception but can see any 
>>>> underlying causes that the JVM may include.
>>>> 
>>>> Thanks for untangling this!
>>>> 
>>>> — John
>>>> 
>>>> On Sep 12, 2013, at 12:17 PM, David Chase  wrote:
>>>> 
>>>>> The test is adapted from the test in the bug report.
>>>>> The headline on the bug report is wrong -- because it uses reflection in 
>>>>> the test to do the invocation,
>>>>> the thrown exception is wrapped with InvocationTargetException, which is 
>>>>> completely correct.
>>>>> HOWEVER, the exception inside the wrapper is the wrong one.
>>>>> 
>>>&

hg: jdk8/tl/jdk: 8019192: StringIndexOutOfBoundsException: in Class.getSimpleName()

[christian . thalinger]

Changeset: 78b4dc33e6e6
Author:twisti
Date:  2013-09-26 18:20 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/78b4dc33e6e6

8019192: StringIndexOutOfBoundsException: in Class.getSimpleName()
Reviewed-by: jrose

! src/share/classes/java/lang/invoke/MemberName.java

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

[Christian Thalinger]

On Sep 26, 2013, at 5:24 PM, Vitaly Davidovich  wrote:

> Why not add the required code when/if it's actually needed? It's not like the 
> pattern is tricky :)
> 
Of course the pattern is not tricky.  The fact that expandFromVM calls down 
into the VM which is filling in fields and name can actually be null is the 
tricky part.  Someone down the line in 5 years might not know about this 
conversation and be surprised.
> your call of course, just looks odd as-is.
> 
> Sent from my phone
> 
> On Sep 26, 2013 8:15 PM, "Christian Thalinger" 
>  wrote:
> 
> On Sep 26, 2013, at 5:11 PM, Vitaly Davidovich  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" 
>>  wrote:
>> 
>> On Sep 26, 2013, at 2:28 PM, John Rose  wrote:
>> 
>> > On Sep 26, 2013, at 1:13 PM, Christian Thalinger 
>> >  wrote:
>> >
>> >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger 
>> >>  wrote:
>> >>
>> >>>
>> >>> On Sep 26, 2013, at 1:22 AM, Peter Levart  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.
>> 

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

[Christian Thalinger]

On Sep 26, 2013, at 5:11 PM, Vitaly Davidovich  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" 
>  wrote:
> 
> On Sep 26, 2013, at 2:28 PM, John Rose  wrote:
> 
> > On Sep 26, 2013, at 1:13 PM, Christian Thalinger 
> >  wrote:
> >
> >> On Sep 26, 2013, at 11:50 AM, Christian Thalinger 
> >>  wrote:
> >>
> >>>
> >>> On Sep 26, 2013, at 1:22 AM, Peter Levart  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!
> 

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

[Christian Thalinger]

On Sep 26, 2013, at 2:28 PM, John Rose  wrote:

> On Sep 26, 2013, at 1:13 PM, Christian Thalinger 
>  wrote:
> 
>> On Sep 26, 2013, at 11:50 AM, Christian Thalinger 
>>  wrote:
>> 
>>> 
>>> On Sep 26, 2013, at 1:22 AM, Peter Levart  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!

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

[Christian Thalinger]

On Sep 26, 2013, at 11:50 AM, Christian Thalinger 
 wrote:

> 
> On Sep 26, 2013, at 1:22 AM, Peter Levart  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/

> 
>> 
>> Regards, Peter
>> 
> 

Re: RFR: Caching MethodType's descriptor string improves lambda linkage performance

[Christian Thalinger]

On Sep 26, 2013, at 9:22 AM, Sergey Kuksenko  wrote:

> Hi All,
> 
> I updated fix.
> You may find it here
> http://cr.openjdk.java.net/~skuksenko/jsr335/8024635/webrev.01/

The change looks good.  Since we use Objects.requireNonNull now we can remove 
the null check comment:

+Objects.requireNonNull(rtype);  // null check

> 
> See my comments and new webrev descition below
> 
> On 09/18/2013 12:59 AM, John Rose wrote:
 We can tweak that later if necessary.  There are probably only a small 
 number of such strings, so maybe a small leaky map would do the trick as 
 well. 
>>> In case of small amount of such strings we will get a huge overhead from
>>> any kind of map.
>> I would expect O(10^1.5) memory references and branches.  Depends on what 
>> you mean by "huge".
> Sure. I think the question to use external map or field may be decided
> later when/if it will be needed.
> Just some statictics, I've collected on nashorn/octane benchmarks
> (statistics collected only for the single(first) benchmark iteration:
> mandreel: 514 unique strings, toMethodDescriptorString called 69047 times
> box2d: 560 unique strings, 34776 toMethodDescriptorString invocations.
> 
>> Would there be any benefit from recording the original string from the 
>> constant pool?  The JVM path for this is 
>> SystemDictionary::find_method_handle_type, which makes an upcall to 
>> MethodHandleNatives.findMethodHandleType.  Currently only the ptypes and 
>> rtype are passed, but the JVM could also convert the internal Symbol to a 
>> java.lang.String and pass that also.  In that case, MT's created by JVM 
>> upcalls would be pre-equipped with descriptor strings.
>> 
>> This is probably worth an experiment, although it requires some JVM work.
> 
> I am definitely sure that we shouldn't do that.
> Right now caching desriptor strings is internal decision of MethodType.
> Otherwise it will make interfaces more complex.
> 
>> I hope you get my overall point:  Hand optimizations have a continuing cost, 
>> related to obscuring the logical structure of the code.  The transforming 
>> engineer might make a mistake, and later maintaining engineers might also 
>> make mistakes.
>> 
>> https://blogs.oracle.com/jrose/entry/three_software_proverbs
> 
> And it doesn't mean that we should afraid any kind of optimization.
> Lucky positions - someone(or something) will optimize it for us. But
> sometimes JIT (even smart JIT) is not enough.
> 
> Let's back to the patch.construstors
> I decided not change original checkPType/checkRType code, except
> explicit Objects.requireNonNull. The problem here that we are creating
> new MethodType objects which are already exists, we have to create MT as
> a key for searching in the internedTable. Ratio between already exiting
> and new MethodTypes a quite huge.
> Here is some statistics I've collected on nashorn/octane benchmarks
> (statistics collected only for the single(first) benchmark iteration:
> 
> mandreel:
>  - 1739 unique MethodTypes,
>  - 878414 MethodType.makeImpl requests;
> box2d:
>  - 1861 unique MethodTypes,
>  - 527486 MethodType.makeImpl requests;
> gameboy:
>  - 2062 unique MethodTypes,
>  - 311378 MethodType.makeImpl requests;
> 
> So
> 1. MethodType constructor do checkPTypes/checkRType which are frequently
> (99%) useless - we already have the same (checked) MethodType in the
> internTable.
> 2. Less than 1% of created MethodTypes will be stored into internTable,
> but reusing that MethodType objects prevent ti from scalar replacement.
> (I've heard that Graal may do flow-sensitive scalar replacement, but C2
> can't do).
> 
> What I did. I separate constructors:
> - for long lived MethodTypes with all checks,
> - for short lived MethodTypes used only as key for searching in the
> InterTable, no checks are performed.
> if we didn't find  MethodType in the table we always create a new one
> (with checks) that is required in less than 1% cases. And we remove at
> least one obstacle for scalar replacement. Unfortunately, scalaer
> replacement for expression "internTable.get(new MethodType(rtype,
> ptypes))" was not performed, the reason may be evaluated, and hope it
> would be easier to achieve scalarization in this case.
> 
> 
> 
> 
> -- 
> Best regards,
> Sergey Kuksenko

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

[Christian Thalinger]

On Sep 26, 2013, at 1:22 AM, Peter Levart  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.

> 
> Regards, Peter
> 

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

[Christian Thalinger]

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

Re: RFR (S) 8001108: an attempt to use "" as a method name should elicit NoSuchMethodException

[Christian Thalinger]

src/share/classes/java/lang/invoke/MethodHandles.java:

+ * methods as if they were normal methods, but the JVM verifier rejects 
them.

I think you should say "JVM byte code verifier" here.

+ * (Note:  JVM internal methods named {@code } not visible 
to this API,
+ * even though the {@code invokespecial} instruction can refer to them
+ * in special circumstances.  Use {@link #findConstructor 
findConstructor}

Not exactly sure but should this read "are not visible"?

 MemberName resolveOrFail(byte refKind, Class refc, String name, 
MethodType type) throws NoSuchMethodException, IllegalAccessException {
+type.getClass();  // NPE
 checkSymbolicClass(refc);  // do this before attempting to resolve
-name.getClass(); type.getClass();  // NPE
+checkMethodName(refKind, name);

Could you add a comment here saying that checkMethodName does an implicit null 
pointer check on name?  Maybe a comment for checkMethodName too.

What was the problem with the null pointer exceptions?  Is it okay that we 
might throw another exception before null checking name?

On Sep 12, 2013, at 6:47 PM, John Rose  wrote:

> Please review this change for a change to the JSR 292 implementation:
> 
> http://cr.openjdk.java.net/~jrose/8001108/webrev.00
> 
> Summary: add an explicit check for leading "<", upgrade the unit tests
> 
> The change is mostly to javadoc and unit tests, documenting and testing some 
> corner cases of JSR 292 APIs.
> 
> In the RI, there is an explicit error check.
> 
> Thanks,
> — John
> 
> P.S. Since this is a change which oriented toward JSR 292 functionality, the 
> review request is to mlvm-dev and core-libs-dev.
> Changes which are oriented toward performance will go to mlvm-dev and 
> hotspot-compiler-dev.
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S) 8001109: arity mismatch on a call to spreader method handle should elicit IllegalArgumentException

[Christian Thalinger]

Looks good.

On Sep 12, 2013, at 6:02 PM, John Rose  wrote:

> Please review this change for a change to the JSR 292 implementation:
> 
> http://cr.openjdk.java.net/~jrose/8001109/webrev.00/
> 
> The change is mostly to javadoc and unit tests, documenting and testing some 
> corner cases of JSR 292 APIs.
> 
> In the RI, the exception-raising code is simplified to throw just IAE.
> 
> Thanks,
> — John
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR (S) 8019417: JSR 292 javadoc should clarify method handle arity limits

[Christian Thalinger]

On Sep 12, 2013, at 4:34 PM, John Rose  wrote:

> Please review this change for a change to the JSR 292 implementation:
> 
> http://cr.openjdk.java.net/~jrose/8019417/webrev.00
> 
> The change is to javadoc and unit tests, documenting and testing some corner 
> cases of JSR 292 APIs.
> 
> Since the RI is already correct, there are no implementation code changes.

This looks good.  The only thing that sounds odd is "very large arity":

+ * very large arity,

-- Chris

> 
> Thanks,
> — John
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(S+M) / 8022701 Accessibility checking: InvocationTargetException is thrown instead of IllegalAccessError

[Christian Thalinger]

On Sep 12, 2013, at 11:28 AM, David Chase  wrote:

> New webrev, commented line removed:
> http://cr.openjdk.java.net/~drchase/8022701/webrev.03/

I think the change is good given that the tests work now.  Is your test derived 
from the test in the bug report?

And it would be good if John could also have a quick look (just be to be sure).

-- Chris

> 
> On 2013-09-12, at 1:53 PM, David Chase  wrote:
> 
>> I believe it produced extraneous output -- it was not clear to me that it 
>> was either necessary or useful to fully describe all the converted 
>> exceptions that lead to the defined exception being thrown.  The commented 
>> line should have just been removed (I think).
>> 
>> On 2013-09-12, at 1:24 PM, Christian Thalinger 
>>  wrote:
>> 
>>> + // err.initCause(ex);
>>> 
>>> Why is this commented?
>>> 
>>> -- Chris
> 

Re: RFR(S+M) / 8022701 Accessibility checking: InvocationTargetException is thrown instead of IllegalAccessError

[Christian Thalinger]

+ // err.initCause(ex);

Why is this commented?

-- Chris

On Sep 6, 2013, at 4:59 PM, David Chase  wrote:

> new, improved webrev: http://cr.openjdk.java.net/~drchase/8022701/webrev.02/
> Same small changes to the sources, plus a test.
> 
> bug: wrong exception was thrown in call of methodhandle to private method
> 
> fix: detect special case of IllegalAccessException, convert to 
> IllegalAccessError
> 
> testing:
> verified that the test would pass with modified library
> verified that the test would fail with old library
> verified that the test would fail if the class substitution (for some reason) 
> did not occur
> jtreg of jdk/test/java/lang/invoke -- note that the new exception thrown is a 
> subtype of the old one, so this is unlikely to create a new surprise
> 
> 
> ___
> mlvm-dev mailing list
> mlvm-...@openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: Classes on the stack trace (was: getElementClass/StackTraceElement, was: @CallerSensitive public API, was: sun.reflect.Reflection.getCallerClass)

[Christian Thalinger]

On Jul 30, 2013, at 4:11 PM, Nick Williams 
 wrote:

> Quick question for those of you that know anything about @CallerSensitive...
> 
> After looking at the code and experimenting some, I've discovered that 
> getCallerClass() doesn't actually keep going until it finds the first method 
> without @CallerSensitive. It only returns the caller of the caller. So, for 
> example:
> 
> Stack 1
> @CallerSensitive Reflection.getCallerClass()
> @CallerSensitive MyClass1.method1();
> MyClass2.method2();
> 
> In this case, getCallerClass() returns MyClass2.class. BUT:
> 
> Stack 2
> @CallerSensitive Reflection.getCallerClass()
> @CallerSensitive MyClass1.method1();
> @CallerSensitive MyClass2.method2();
> MyClass3.method3();
> 
> In this case, getCallerClass() STILL returns MyClass2.class. Based on the 
> plain-language meaning of @CallerSensitive, I would expect getCallerClass() 
> to return MyClass3.class in the second case. But, indeed, the JavaDoc for 
> Reflection.getCallerClass() says: "Returns the class of the caller of the 
> method calling this method." So, then, what's the point of @CallerSensitive? 
> Looking at the code:
> 
>   vframeStream vfst(thread);
>   // Cf. LibraryCallKit::inline_native_Reflection_getCallerClass
>   for (int n = 0; !vfst.at_end(); vfst.security_next(), n++) {
> Method* m = vfst.method();
> assert(m != NULL, "sanity");
> switch (n) {
> case 0:
>   // This must only be called from Reflection.getCallerClass
>   if (m->intrinsic_id() != vmIntrinsics::_getCallerClass) {
> THROW_MSG_NULL(vmSymbols::java_lang_InternalError(), 
> "JVM_GetCallerClass must only be called from Reflection.getCallerClass");
>   }
>   // fall-through
> case 1:
>   // Frame 0 and 1 must be caller sensitive.
>   if (!m->caller_sensitive()) {
> THROW_MSG_NULL(vmSymbols::java_lang_InternalError(), 
> err_msg("CallerSensitive annotation expected at frame %d", n));
>   }
>   break;
> default:
>   if (!m->is_ignored_by_security_stack_walk()) {
> // We have reached the desired frame; return the holder class.
> return (jclass) JNIHandles::make_local(env, 
> m->method_holder()->java_mirror());
>   }
>   break;
> }
>   }
> 
> It seems to me that @CallerSensitive is completely pointless. This is ALWAYS 
> going to return the first non-reflection frame after frame 1, regardless of 
> @CallerSensitive. If @CallerSensitive were really supposed to have an actual 
> purpose, it would seem to me that the last part should be this:
> 
>   if (!m->is_ignored_by_security_stack_walk() && !m->caller_sensitive()) {
> // We have reached the desired frame; return the holder class.
> return (jclass) JNIHandles::make_local(env, 
> m->method_holder()->java_mirror());
>   }
> 
> Am I completely missing the point here? I just don't see a reason for 
> @CallerSensitive. The code could do the exact same thing it currently is 
> without @CallerSensitive (except for enforcing that frame 1 is 
> @CallerSensitive, which really isn't necessary if you aren't using it in 
> further frames).
> 
> Thoughts?

You are missing the second (and perhaps more important) part of this change.  
Read:

http://openjdk.java.net/jeps/176

-- Chris

> 
> Nick
> 
> On Jul 30, 2013, at 10:33 AM, Jochen Theodorou wrote:
> 
>> Am 30.07.2013 16:16, schrieb Peter Levart:
>>> 
>>> On 07/30/2013 03:19 PM, Jochen Theodorou wrote:
 Am 30.07.2013 14:17, schrieb Peter Levart:
 [...]
> So what would give Groovy or other language runtimes headaches when all
> there was was a parameter-less getCallerClass() API? Aren't the
> intermediate frames inserted by those runtimes controlled by the
> runtimes? Couldn't the "surface" runtime-inserted methods capture the
> caller and pass it down? I guess the problem is supporting calling the
> caller-sensitive methods like Class.forName(String) and such which don't
> have the overloaded variant taking caller Class or ClassLoader as an
> argument...
 Speaking for Groovy...
 those intermediate frames are runtime controlled, yes, but passing down
 the caller class is exactly the problem. Imagine I would suggest that
 each and every method definition in Java automatically gets an
 additional parameter for the caller class, just to have access to it
 inside the method. You would not accept that for Java, would you? And so
 we cannot accept that for Groovy if we want to keep integration with
 Java...
>>> 
>>> Are you talking about internal Groovy implementation (the
>>> runtime-inserted methods) or the publicly visible API?
>> 
>> that's the problem, it is a mix, some internal, other not. We are going to 
>> change that in Groovy 3
>> 
>>> One solution for
>>> internal implementation of Groovy could be (speaking by heart since I
>>> don't know the internals of Groovy) for the "surface" public API method
>>> which doesn't have to have the special call

hg: jdk8/tl/jdk: 8019184: MethodHandles.catchException() fails when methods have 8 args + varargs

[christian . thalinger]

Changeset: bd6949f9dbb2
Author:twisti
Date:  2013-07-03 11:35 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bd6949f9dbb2

8019184: MethodHandles.catchException() fails when methods have 8 args + varargs
Reviewed-by: jrose

! src/share/classes/java/lang/invoke/MethodHandleImpl.java
+ test/java/lang/invoke/TestCatchExceptionWithVarargs.java

hg: jdk8/tl/jdk: 7177472: JSR292: MethodType interning penalizes scalability

[christian . thalinger]

Changeset: 2b63fda275a3
Author:twisti
Date:  2013-06-17 16:24 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2b63fda275a3

7177472: JSR292: MethodType interning penalizes scalability
Reviewed-by: twisti
Contributed-by: Aleksey Shipilev 

! src/share/classes/java/lang/invoke/MethodType.java

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 13, 2013, at 12:09 PM, Aleksey Shipilev  
wrote:

> On 06/13/2013 09:51 PM, Christian Thalinger wrote:
>> While preparing the push I noticed the new code gives a warning:
>> 
>> src/share/classes/java/lang/invoke/MethodType.java:1106: warning: 
>> [unchecked] unchecked cast
>>T that = ((WeakEntry) obj).get();
>> ^
>>  required: WeakEntry
>>  found:Object
>>  where T is a type-variable:
>>T extends Object declared in class WeakEntry
>> 1 warning
>> 
>> Could you fix that, please?
> 
> Can't reproduce that warning in my builds (are you having
> -Xlint:unchecked enabled in the new build system somehow?)

More or less.  I'm only building java/lang/invoke plus friends and use 
-Xlint:unchecked.

> , but good
> catch! There is the preceding instanceof check that ought to make this
> cast safe now. Also we don't need to declare locals as T in equals().
> 
> Please try this:
>   http://cr.openjdk.java.net/~shade/7177472/webrev.03/
> 
> This seems a trivial change, so I only tested java/lang/invoke
> regression tests afterwards, those are OK.

Looks good now.  Thanks for the quick turnaround.

-- Chris

> 
> -Aleksey.

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 11, 2013, at 7:16 PM, Christian Thalinger 
 wrote:

> 
> On Jun 11, 2013, at 2:21 PM, Aleksey Shipilev  
> wrote:
> 
>> On 06/11/2013 10:24 PM, Christian Thalinger wrote:
>>> 
>>> On Jun 11, 2013, at 9:08 AM, Aleksey Shipilev  
>>> wrote:
>>> 
>>>> On 06/11/2013 08:06 PM, Christian Thalinger wrote:
>>>>> Anyway, let's push this.
>>>> 
>>>> Do you want me any other testing done? vm.mlvm.testlist is OK, but I
>>>> haven't done the full JPRT test cycle, only the build one.
>>> 
>>> You could do a full Nashorn 262 run.  That would shake out bugs.
>> 
>> Done. Linux x86_64/release passes test262parallel with either clean or
>> patched build.
> 
> Thanks for verifying.  I'll push your change tomorrow.

While preparing the push I noticed the new code gives a warning:

src/share/classes/java/lang/invoke/MethodType.java:1106: warning: [unchecked] 
unchecked cast
T that = ((WeakEntry) obj).get();
 ^
  required: WeakEntry
  found:Object
  where T is a type-variable:
T extends Object declared in class WeakEntry
1 warning

Could you fix that, please?

-- Chris

> 
> -- Chris
> 
>> 
>> -Aleksey.
>> 
> 

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 11, 2013, at 2:21 PM, Aleksey Shipilev  
wrote:

> On 06/11/2013 10:24 PM, Christian Thalinger wrote:
>> 
>> On Jun 11, 2013, at 9:08 AM, Aleksey Shipilev  
>> wrote:
>> 
>>> On 06/11/2013 08:06 PM, Christian Thalinger wrote:
>>>> Anyway, let's push this.
>>> 
>>> Do you want me any other testing done? vm.mlvm.testlist is OK, but I
>>> haven't done the full JPRT test cycle, only the build one.
>> 
>> You could do a full Nashorn 262 run.  That would shake out bugs.
> 
> Done. Linux x86_64/release passes test262parallel with either clean or
> patched build.

Thanks for verifying.  I'll push your change tomorrow.

-- Chris

> 
> -Aleksey.
> 

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 11, 2013, at 9:08 AM, Aleksey Shipilev  
wrote:

> On 06/11/2013 08:06 PM, Christian Thalinger wrote:
>> Anyway, let's push this.
> 
> Do you want me any other testing done? vm.mlvm.testlist is OK, but I
> haven't done the full JPRT test cycle, only the build one.

You could do a full Nashorn 262 run.  That would shake out bugs.

-- Chris

> 
> -Aleksey.

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 11, 2013, at 9:03 AM, Aleksey Shipilev  
wrote:

> On 06/11/2013 07:43 PM, Christian Thalinger wrote:
>> Thanks for this but I was thinking running it with -Xprof to see
>> whether some of the table methods show up on the profile.
> 
> I put little trust into this kind of profiling, but here's the sample
> output from JMH -prof stack, while running with 4 threads:
> 
> Before:
>> Iteration   5 (20s in 4 threads): 380.982 msec/op
>>   Stack |  24.8%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.ByteCodeMachine.matchAt
>> |   6.5%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.SearchAlgorithm$3.search
>> |   5.2%   RUNNABLE java.lang.Class.getClassLoader0
>> |   3.3%   RUNNABLE java.lang.ThreadLocal.get
>> |   3.0%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.JoniRegExp$JoniMatcher.
>> |   2.9%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeRegExp.replace
>> |   2.4%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.Matcher.forwardSearchRange
>> |   2.3%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeRegExp.exec
>> |   2.1%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeRegExp.groups
>> |   1.9%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeString.replace
>> |  45.6%(other)
>> |
> 
> 
> After:
>> Iteration   5 (20s in 4 threads): 358.121 msec/op
>>   Stack |  24.7%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.ByteCodeMachine.matchAt
>> |   5.7%   RUNNABLE java.lang.Class.getClassLoader0
>> |   5.3%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.SearchAlgorithm$3.search
>> |   3.5%   RUNNABLE java.lang.ThreadLocal.get
>> |   3.4%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.JoniRegExp$JoniMatcher.
>> |   2.7%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeRegExp.replace
>> |   2.3%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.Matcher.forwardSearchRange
>> |   1.8%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeRegExp.exec
>> |   1.7%   RUNNABLE 
>> jdk.nashorn.internal.objects.NativeString.replace
>> |   1.5%   RUNNABLE 
>> jdk.nashorn.internal.runtime.regexp.joni.StackMachine.pop
>> |  47.3%(other)
>> |
> 
> -Xprof shows the similar data.

Odd.  It showed up for me last time.  Maybe it's related to using Joni now.

Anyway, let's push this.

-- Chris

> 
> -Aleksey.

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 11, 2013, at 1:47 AM, Aleksey Shipilev  
wrote:

> On 06/11/2013 02:52 AM, Christian Thalinger wrote:
>> This looks good to me.  
> 
> Thanks for the review!
> 
>> Could you run RegExp with Nashorn again with and without your changes?
> 
> Being single-threaded, the benchmark will not show much of the
> improvement. Also, the results are rather flaky.
> 
> Before:
>  1 thread: 175 +- 5
> 2 threads: 199 +- 5  msec/op
> 
> After:
>  1 thread: 177 +- 6 msec/op
> 2 threads: 189 +- 6 msec/op

Thanks for this but I was thinking running it with -Xprof to see whether some 
of the table methods show up on the profile.

-- Chris

> 
> -Aleksey.

Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Christian Thalinger]

On Jun 10, 2013, at 8:47 AM, Aleksey Shipilev  
wrote:

> On 06/09/2013 12:17 AM, Peter Levart wrote:
>> In case the loop retries, there's no need to construct another WeakEntry...
>> 
>>T interned;
>> WeakEntry e = new WeakEntry<>(elem, stale);
>>do {
>>expungeStaleElements();
>>WeakEntry exist = map.putIfAbsent(e, e);
>>interned = (exist == null)? elem: exist.get();
>>} while (interned == null);
>>return interned;
> 
> That's right, thanks!
> 
> The update is here:
>  http://cr.openjdk.java.net/~shade/7177472/webrev.02/
> 
> Testing:
>  - Linux x86_64 builds OK
>  - Linux x86_64 java/lang/invoke/ jtreg passes OK
>  - The microbenchmark scores in the original note are still the same

This looks good to me.  One thing I mentioned in the bug report is that V8's 
RegExp shows some MethodType related methods in a -Xprof run:

 Compiled + native Method 
 11.9% 2455 + 0 java.util.regex.Pattern$GroupTail.match 
 11.5% 2382 + 0 java.util.regex.Pattern$Start.match 
 10.2% 2110 + 0 java.util.regex.Pattern$5.isSatisfiedBy 
  8.5% 1765 + 0 java.util.regex.Pattern$Branch.match 
  7.1% 1461 + 0 java.lang.invoke.MethodType$WeakInternSet.get 
  3.4% 709 + 0 java.util.regex.Pattern$CharProperty.match 
  2.9% 597 + 1 java.lang.invoke.MethodType.makeImpl 

Could you run RegExp with Nashorn again with and without your changes?

-- Chris

> 
> -Aleksey.
> 

Re: Review request for 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

On Mar 27, 2013, at 8:01 PM, Mandy Chung  wrote:

> Thanks for the review.  I forgot to mention that Chris contributed the 
> initial patch (thanks).
> 
> On 3/27/2013 1:13 PM, Christian Thalinger wrote:
>> On Mar 27, 2013, at 10:35 AM, Mandy Chung  wrote:
>> 
>>> This is the JDK change for JEP 176: JEP 176: Mechanical Checking of 
>>> Caller-Sensitive Methods [1].  Christian has posted the webrev for the 
>>> hotspot VM change a couple weeks ago [2].
>>> 
>>> Webrev at:
>>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.00/
>> src/share/classes/java/lang/ClassLoader.java:
>> 
>> +static void checkClassLoaderPermission(ClassLoader cl, Class caller) 
>> {
>> 
>> I think we should rename that method to:
>> 
>> +static void checkGetClassLoaderPermission(ClassLoader cl, Class 
>> caller) {
> 
> I think checkClassLoaderPermission and needsClassLoaderPermissionCheck are 
> just fine.  I'd like to keep it as it is not to make the method name too long.
> 
>> src/share/classes/java/lang/invoke/MethodHandleImpl.java:
>> 
>> +@sun.reflect.CallerSensitive
>> +Class actual = sun.reflect.Reflection.getCallerClass();
>> 
>> Why are we not using imports here?
> 
> imports are for convenience and ease of development.  For only one reference,

Well, it's actually two ;-)  I was thinking about people grep'ing for 
@CallerSensitive and not getting a hit on this one.  Just a suggestion.

> I don't see any difference to import or not.
> 
>> src/share/classes/java/util/logging/Logger.java:
>> 
>>  // 0: Reflection 1: Logger.demandLogger 2: Logger.getLogger 3: 
>> caller
>>  final int SKIP_FRAMES = 3;
>> 
>> Please remove these lines as well.
> 
> Removed.  Thanks for catching the leftover comment.
>> src/share/native/sun/reflect/Reflection.c:
>> 
>> Could you put back this comment:
>> 
>> + // Let's do at least some basic handshaking:
>> + const int depth = -1;
>> 
>> It makes it clearer why it's -1.
> 
> I added this comment:
>  32 // with the presence of @CallerSensitive annotation,
>  33 // JVM_GetCallerClass asserts depth == -1 as the basic handshaking

Thanks.

> 
>> test/sun/reflect/GetCallerClass.sh:
>> 
>> Could you please don't use a shell script to copy the class file?
> 
> The shell test doesn't do a copy.  It compiles the source file in a separate 
> directory that will be specified in -Xbootclasspath/a option in javac and 
> java commands.
> 
> jtreg in the code-tool repo has added the bootclasspath support:
>http://hg.openjdk.java.net/code-tools/jtreg/rev/98387c9f36e3
> 
> You can specify in the @run tag:
>@run main/bootclasspath opt class
> 
> This will be a better way to run a test on the bootclasspath.

That's great.

> 
>> For example this test:
>> 
>> http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/compiler/whitebox/DeoptimizeAllTest.java
>> 
>> does the same thing using a little Java program ClassFileInstaller:
>> 
>> http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/testlibrary/ClassFileInstaller.java
> 
> This is a nice workaround to avoid shell tests.  It compiles the source file 
> in $TESTCLASSES and copies the one in a different location (dest) that will 
> be used in a @run main -Xbootclasspath/a:dest class.
> 
> I prefer to use the new jtreg bootclasspath support when it's released rather 
> than adding yet another workaround to avoid shell tests.   We should replace 
> many, if not all, existing shell tests that currently put classes in the 
> bootclasspath with the jtreg bootclasspath support in one patch.  I keep the 
> test as it is.

I take your word for it that you will remove it.  Checking back in a year... :-)

-- Chris

> 
> Mandy
> 
>> -- Chris
>> 
>>> While it touches many files, the fix is simple and straight-forward for 
>>> review.
>>> 
>>> This fix annotates all methods that call Reflection.getCallerClass() method 
>>> with @sun.reflect.CallerSensitive annotation so that it enables the VM to 
>>> reliably enforce that methods looking up its immediate caller class are 
>>> marked as caller-sensitive. The JVM will set a new caller-sensitive bit 
>>> when resolving a MemberName and 
>>> java.lang.invoke.MethodHandleNatives.isCallerSensitive is upgraded to query 
>>> it directly.
>>> The hand-maintained method list in MethodHandleNatives is removed.
>>> 
>>

Re: Review request for 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

On Mar 27, 2013, at 10:35 AM, Mandy Chung  wrote:

> This is the JDK change for JEP 176: JEP 176: Mechanical Checking of 
> Caller-Sensitive Methods [1].  Christian has posted the webrev for the 
> hotspot VM change a couple weeks ago [2].
> 
> Webrev at:
> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7198429/webrev.00/

src/share/classes/java/lang/ClassLoader.java:

+static void checkClassLoaderPermission(ClassLoader cl, Class caller) {

I think we should rename that method to:

+static void checkGetClassLoaderPermission(ClassLoader cl, Class caller) 
{

src/share/classes/java/lang/invoke/MethodHandleImpl.java:

+@sun.reflect.CallerSensitive
+Class actual = sun.reflect.Reflection.getCallerClass();

Why are we not using imports here?

src/share/classes/java/util/logging/Logger.java:

 // 0: Reflection 1: Logger.demandLogger 2: Logger.getLogger 3: 
caller
 final int SKIP_FRAMES = 3;

Please remove these lines as well.

src/share/native/sun/reflect/Reflection.c:

Could you put back this comment:

+ // Let's do at least some basic handshaking:
+ const int depth = -1;

It makes it clearer why it's -1.

test/sun/reflect/GetCallerClass.sh:

Could you please don't use a shell script to copy the class file?  For example 
this test:

http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/compiler/whitebox/DeoptimizeAllTest.java

does the same thing using a little Java program ClassFileInstaller:

http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/file/tip/test/testlibrary/ClassFileInstaller.java

-- Chris

> 
> While it touches many files, the fix is simple and straight-forward for 
> review.
> 
> This fix annotates all methods that call Reflection.getCallerClass() method 
> with @sun.reflect.CallerSensitive annotation so that it enables the VM to 
> reliably enforce that methods looking up its immediate caller class are 
> marked as caller-sensitive. The JVM will set a new caller-sensitive bit when 
> resolving a MemberName and 
> java.lang.invoke.MethodHandleNatives.isCallerSensitive is upgraded to query 
> it directly.
> The hand-maintained method list in MethodHandleNatives is removed.
> 
> A couple things to mention:
> 1. I am working on a fix for 8007035 that proposes to deprecate 
> SecurityManager.checkMemberAccess method as it requires the caller’s frame to 
> be at a stack depth of four, which is fragile and difficult to enforce.
> 
> 2. NashornScriptEngineFactory.getAppClassLoader()
> 
> The change is to workaround the issue until 8009783 is resolved.
> 
> The current implementation walks the stack to find the classloader of the 
> user context that NashornScriptEngine is running on which is fragile.  Also 
> other script engine implementations may require similiar capability.  8009783 
> has been filed to revisit the scripting API to pass the user "context" to the 
> script engine rather than relying the implementation to find it magically.
> 
> Thanks
> Mandy
> 
> [1] http://openjdk.java.net/jeps/176
> [2] http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-March/008915.html

Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

On Mar 19, 2013, at 6:02 PM, Christian Thalinger 
 wrote:

> 
> On Mar 19, 2013, at 5:21 PM, John Rose  wrote:
> 
>> On Mar 14, 2013, at 8:31 PM, Christian Thalinger 
>>  wrote:
>> 
>>> [This is the HotSpot part of JEP 176]
>>> 
>>> http://cr.openjdk.java.net/~twisti/7198429
>>> 
>>> 7198429: need checked categorization of caller-sensitive methods in the JDK
>>> Reviewed-by:
>> 
>> 
>> Over all, great work on a tricky problem.  I'd add a few asserts and tweak a 
>> couple of lines; see below.  Reviewed as is or with suggested changes. — John
>> 
>> --- Method::is_ignored_by_security_stack_walk
>> I would like to see reflect_invoke_cache go away some day.  Would it be 
>> possible to strengthen the asserts to prove that it is an expensive alias 
>> for an intrinsic_id check?  (I realize this is a question mainly for folks 
>> working on JVMTI.)
> 
> That's what I tried to do:  if the intrinsic_id == _invoke it also must be 
> the same method in reflect_invoke_cache.  More than that would mean to 
> enhance ActiveMethodOopsCache because you can't ask for methods in the cache.
> 
>> 
>> --- JVM_GetCallerClass
>> Suggest an assert for vfst.method() == NULL.  Should not happen, and 
>> previous code would apparently have crashed already, but...
>> 
>> (The corner case I'm thinking of is a compiled frame with nmethod::method 
>> returning null after nmethod::make_unloaded.  Should not happen.)
> 
> Sure, I can add that assert but there is other code in jvm.cpp that relies on 
> the fact that vfst.method() is non-null.  We should add asserts in all that 
> places but that's for another RFE.
> 
>> 
>> --- JVM_GetClassContext
>> What do these lines do:
>> +   // Collect method holders
>> +   GrowableArray* klass_array = new 
>> GrowableArray();
>> 
>> It looks like a paste-o from another source base.
> 
> Left over.  I filed an RFE for that improvement:
> 
> JDK-8010124: JVM_GetClassContext: use GrowableArray instead of KlassLink
> 
>> 
>> --- LibraryCallKit::inline_native_Reflection_getCallerClass
>> 
>> I believe this assertion, but I would prefer to see it checked more forcibly:
>> +   // NOTE: Start the loop at depth 1 because the current JVM state does
>> +   // not include the Reflection.getCallerClass() frame.
>> 
>> Not sure there is a good way to do this.  But, perhaps put the comment here:
>>  case 0:
>>// ...comment...
>>ShouldNotReachHere();
> 
> How about:
> 
>case 0:
>  fatal("current JVM state does not include the 
> Reflection.getCallerClass() frame");
>  break;
> 
>> 
>> Also, if something goes wrong with caller sensitivity, we just get a "return 
>> false".  Perhaps do a "caller_jvm=NULL;break" to branch to the pretty 
>> failure message?  That makes it slightly easier to see what happened.
> 
> It seems easier to add printing code to the case statement:
> 
>case 1:
>  // Frame 0 and 1 must be caller sensitive (see JVM_GetCallerClass).
>  if (!m->caller_sensitive()) {
> #ifndef PRODUCT
>if ((PrintIntrinsics || PrintInlining || PrintOptoInlining) && 
> Verbose) {
>  tty->print_cr("  Bailing out: CallerSensitive annotation expected at 
> frame %d", n);
>}
> #endif
>return false;  // bail-out; let JVM_GetCallerClass do the work
>  }
>  break;
> 
>> 
>> The LogCompilation switch should leave a "paper trail".  Actually, I see 
>> that LogCompilation doesn't mention failed intrinsic inlines.  Rats.  At 
>> least PrintInlining or PrintIntrinsics (diagnostic flags) will give us some 
>> leverage if we need to debug.
>> 
>> --- JVM_RegisterUnsafeMethods
>> That's an improvement.  Thanks.
>> 
>> (A nagging worry:  How big are those static tables getting?)
> 
> We could remove some very old ones like 1.4.0 and 1.4.1.  This time, next 
> time?
> 
>> 
>> --- vframeStreamCommon::security_get_caller_frame
>> This now does something odd if depth < 0.  Suggest an assert.
> 
> The behavior with depth < 0 in the current code is even worse.  An assert is 
> a good idea.  As discussed I want to remove that method in the future because 
> its uses are dubious.

I forgot to update the webrev.  Here you go:

http://cr.openjdk.java.net/~twisti/7198429/

-- Chris

Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

On Mar 19, 2013, at 5:21 PM, John Rose  wrote:

> On Mar 14, 2013, at 8:31 PM, Christian Thalinger 
>  wrote:
> 
>> [This is the HotSpot part of JEP 176]
>> 
>> http://cr.openjdk.java.net/~twisti/7198429
>> 
>> 7198429: need checked categorization of caller-sensitive methods in the JDK
>> Reviewed-by:
> 
> 
> Over all, great work on a tricky problem.  I'd add a few asserts and tweak a 
> couple of lines; see below.  Reviewed as is or with suggested changes. — John
> 
> --- Method::is_ignored_by_security_stack_walk
> I would like to see reflect_invoke_cache go away some day.  Would it be 
> possible to strengthen the asserts to prove that it is an expensive alias for 
> an intrinsic_id check?  (I realize this is a question mainly for folks 
> working on JVMTI.)

That's what I tried to do:  if the intrinsic_id == _invoke it also must be the 
same method in reflect_invoke_cache.  More than that would mean to enhance 
ActiveMethodOopsCache because you can't ask for methods in the cache.

> 
> --- JVM_GetCallerClass
> Suggest an assert for vfst.method() == NULL.  Should not happen, and previous 
> code would apparently have crashed already, but...
> 
> (The corner case I'm thinking of is a compiled frame with nmethod::method 
> returning null after nmethod::make_unloaded.  Should not happen.)

Sure, I can add that assert but there is other code in jvm.cpp that relies on 
the fact that vfst.method() is non-null.  We should add asserts in all that 
places but that's for another RFE.

> 
> --- JVM_GetClassContext
> What do these lines do:
> +   // Collect method holders
> +   GrowableArray* klass_array = new 
> GrowableArray();
> 
> It looks like a paste-o from another source base.

Left over.  I filed an RFE for that improvement:

JDK-8010124: JVM_GetClassContext: use GrowableArray instead of KlassLink

> 
> --- LibraryCallKit::inline_native_Reflection_getCallerClass
> 
> I believe this assertion, but I would prefer to see it checked more forcibly:
> +   // NOTE: Start the loop at depth 1 because the current JVM state does
> +   // not include the Reflection.getCallerClass() frame.
> 
> Not sure there is a good way to do this.  But, perhaps put the comment here:
>   case 0:
> // ...comment...
> ShouldNotReachHere();

How about:

case 0:
  fatal("current JVM state does not include the Reflection.getCallerClass() 
frame");
  break;

> 
> Also, if something goes wrong with caller sensitivity, we just get a "return 
> false".  Perhaps do a "caller_jvm=NULL;break" to branch to the pretty failure 
> message?  That makes it slightly easier to see what happened.

It seems easier to add printing code to the case statement:

case 1:
  // Frame 0 and 1 must be caller sensitive (see JVM_GetCallerClass).
  if (!m->caller_sensitive()) {
#ifndef PRODUCT
if ((PrintIntrinsics || PrintInlining || PrintOptoInlining) && Verbose) 
{
  tty->print_cr("  Bailing out: CallerSensitive annotation expected at 
frame %d", n);
}
#endif
return false;  // bail-out; let JVM_GetCallerClass do the work
  }
  break;

> 
> The LogCompilation switch should leave a "paper trail".  Actually, I see that 
> LogCompilation doesn't mention failed intrinsic inlines.  Rats.  At least 
> PrintInlining or PrintIntrinsics (diagnostic flags) will give us some 
> leverage if we need to debug.
> 
> --- JVM_RegisterUnsafeMethods
> That's an improvement.  Thanks.
> 
> (A nagging worry:  How big are those static tables getting?)

We could remove some very old ones like 1.4.0 and 1.4.1.  This time, next time?

> 
> --- vframeStreamCommon::security_get_caller_frame
> This now does something odd if depth < 0.  Suggest an assert.

The behavior with depth < 0 in the current code is even worse.  An assert is a 
good idea.  As discussed I want to remove that method in the future because its 
uses are dubious.

-- Chris

Re: RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

On Mar 19, 2013, at 1:14 PM, Mandy Chung  wrote:

> I do a partial review in particular to make sure the jdk and hotspot change 
> are in sync.
> 
> javaClasses.hpp - MN_CALLER_SENSITIVE and MN_SEARCH_SUPERCLASSES have the 
> same value.  Should they be different?
> 
> 1057 MN_CALLER_SENSITIVE = 0x0010, // @CallerSensitive annotation 
> detected
> 1061 MN_SEARCH_SUPERCLASSES  = 0x0010, // walk super classes

They can have the same value because they are used for different things in 
different places.  I talked to John about this and he said that the MN_SEARCH_* 
guys don't add much value and might be removed.

> 
> 
> method.hpp - Is caller_sensitive set to true if @CallerSensitive annotation 
> is present and must be loaded by null class loader?  Does it only check 
> annotations if the class of that method is defined by the null class loader?  
> Per our offline discussion early, classes loaded by the extension class 
> loader may also be caller-sensitive.

Right.  I forgot to add that code.  Here is an incremental webrev:

http://cr.openjdk.java.net/~twisti/7198429/edit/

And the full thing:

http://cr.openjdk.java.net/~twisti/7198429/

Let me know if that works for you.

> 
> If a method calls Reflection.getCallerClass but its class is defined by other 
> class loader (non-null and not extension class loader), your fix will throw 
> InternalError with the same error message even if that method is annotated 
> with @CS.  Is there a way to improve the error message so that we can 
> differentiate this case (i.e. @CS is present but not supported)?

Not easily.  We set a flag on the method when parse the class.  At that point 
we decide if the annotation is there or not.  If the annotation is not allowed 
in parsed class because it's not loaded by the right class loader then it does 
not "exist".

> 
> jvm.cpp: have you considered adding a new entry point instead of having 
> JVM_GetCallerClass to behave differently depending on the existence of 
> sun.reflect.CallerSensitive class? There are pros and cons of both options. 
> Having a new entry point is cleaner and enables the opportunity to remove 
> JVM_GetCallerClass(int) in the future.  I am fine with either approach but 
> just to bring it up.

Yes.  I talked to Vladimir about that yesterday.  The better solution seems to 
be to leave the old entry point.  If we add a new one that kind of adds a new 
method to the "official" VM interface.  Other VM implementors would have to 
implement that one as well because the JDK then links to this new method.

Thanks for the review!

-- Chris

> 
> Mandy
> 
> 
> On 3/14/13 8:31 PM, Christian Thalinger wrote:
>> [This is the HotSpot part of JEP 176]
>> 
>> http://cr.openjdk.java.net/~twisti/7198429
>> 
>> 7198429: need checked categorization of caller-sensitive methods in the JDK
>> Reviewed-by:
>> 
>> More information in JEP 176:
>> 
>> http://openjdk.java.net/jeps/176
>> 
>> src/share/vm/ci/ciMethod.cpp
>> src/share/vm/ci/ciMethod.hpp
>> src/share/vm/classfile/classFileParser.cpp
>> src/share/vm/classfile/classFileParser.hpp
>> src/share/vm/classfile/javaClasses.hpp
>> src/share/vm/classfile/systemDictionary.hpp
>> src/share/vm/classfile/vmSymbols.hpp
>> src/share/vm/oops/method.cpp
>> src/share/vm/oops/method.hpp
>> src/share/vm/opto/library_call.cpp
>> src/share/vm/prims/jvm.cpp
>> src/share/vm/prims/methodHandles.cpp
>> src/share/vm/prims/unsafe.cpp
>> src/share/vm/runtime/vframe.cpp
>> src/share/vm/runtime/vframe.hpp
>> 
> 

RFR (M): 7198429: need checked categorization of caller-sensitive methods in the JDK

[Christian Thalinger]

[This is the HotSpot part of JEP 176]

http://cr.openjdk.java.net/~twisti/7198429

7198429: need checked categorization of caller-sensitive methods in the JDK
Reviewed-by:

More information in JEP 176:

http://openjdk.java.net/jeps/176

src/share/vm/ci/ciMethod.cpp
src/share/vm/ci/ciMethod.hpp
src/share/vm/classfile/classFileParser.cpp
src/share/vm/classfile/classFileParser.hpp
src/share/vm/classfile/javaClasses.hpp
src/share/vm/classfile/systemDictionary.hpp
src/share/vm/classfile/vmSymbols.hpp
src/share/vm/oops/method.cpp
src/share/vm/oops/method.hpp
src/share/vm/opto/library_call.cpp
src/share/vm/prims/jvm.cpp
src/share/vm/prims/methodHandles.cpp
src/share/vm/prims/unsafe.cpp
src/share/vm/runtime/vframe.cpp
src/share/vm/runtime/vframe.hpp

Re: Request for review (M): JDK-7087570: java.lang.invoke.MemberName information wrong for method handles created with findConstructor

[Christian Thalinger]

On Feb 8, 2013, at 10:38 AM, Krystal Mo  wrote:

> Hi all,
> 
> Could I have a couple of review for this change:
> 
> 7087570: java.lang.invoke.MemberName information wrong for method handles 
> created with findConstructor
> Summary: REF_invokeSpecial DMHs (which are unusual) get marked explicitly; 
> tweak the MHI to use this bit
> Reviewed-by: ?
> 
> Webrev: http://cr.openjdk.java.net/~kmo/7087570/webrev.00/

src/share/classes/java/lang/invoke/DirectMethodHandle.java:

+ static DirectMethodHandle make(Class receiver, MemberName member) {
+ byte refKind = member.getReferenceKind();
+ if (refKind == REF_invokeSpecial)
+ refKind =  REF_invokeVirtual;
+ return make(refKind, receiver, member);
+ }

We are replacing all invokespecials with invokevirtuals?

-- Chris

> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7087570
> ( And a duplicate of this: 
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005119 )
> 
> Background:
> 
> When linking methods, HotSpot VM may strength-reduce the invocation mode of 
> some virtual methods from invokevirtual to invokespecial.
> e.g. virtual methods in a final class, or non-private member methods marked 
> as final.
> When that happens, a method's invocation mode may be nominally 
> "invokevirtual", but the VM actually treats it as "invokespecial".
> 
> Before this fix, the Java-level MethodHandle implementation didn't tell apart 
> the "real" invokespecials with the strength-reduced ones. When creating a 
> MethodHandle via lookup/ldc/unreflect, wrong metadata may be returned if this 
> strength-reduction happened. This broke a few things, including the two 
> scenarios in JDK-7087570 and JDK-8005119.
> 
> With the fix, special handling is added so that a "real" invokespecial is 
> truly a "Special" version of DirectMethodHandle, so users of the MethodHandle 
> will not be affected by the mismatch between the nominal and the actual 
> invocation mode.
> 
> For the record, credit goes to John Rose who did the actual fix. I only added 
> the unit test to verify the fix.
> 
> Tested with JPRT, jtreg test/java/lang/invoke, including the new unit test.
> 
> I intend to push this to jdk8/tl/jdk as it's a Java-only change; I believe 
> langtool people could get the bits earlier this way.
> Could somebody from the jdk side help with the push?
> 
> Thanks,
> Kris

Re: Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86

[Christian Thalinger]

On Jan 11, 2013, at 10:19 AM, Ulf Zibis  wrote:

> Hi Sherman,
> 
> Am 11.01.2013 06:47, schrieb Xueming Shen:
>> (1) sp++ at Ln#159 probably can be moved into ln#155? the local sp here 
>> should
>> not change the real "sp" after "break+return".
>> (2) maybe the "overflowflag" can just be replaced by "CoderResult cr", with 
>> init value
>> of CoderResult.UNDERFLOW;",  then "cr = CoderResult.OVERFLOW" at ln#182, 
>> and
>> simply "return cr" at ln#193, without another "if".
> 
> I've enhanced your suggestions, see more below...
> Additionally, some part of encodeArrayLoop(...) maybe could be moved into a 
> separate method, to be reused by encode(char[] src, int sp, int len, byte[] 
> dst).
> Some of the re-engineering could be adapted to the Decoder methods.
> 
>> I'm surprised we only get 1.6% boost.  Maybe it is worth measuring the 
>> performance
>> of some "small" buf/array encoding? I'm a little concerned that the overhead 
>> may
>> slow down the small size buf/array encoding. There is a simple benchmark for 
>> String
>> en/decoding at test/sun/nio/cs/StrCodingBenchmark.
> 
> I think we should balance 4 cases in rating the performance:
> a) few loops, small buf/array
> b) few loops, big buf/array
> c) many loops, small buf/array
> d) many loops, big buf/array
> In a), b) the loop surrounding code will not be JIT-compiled, so should be 
> optimized for interpreter.
> In c) d) the loop surrounding code *may be* JIT-compiled and consequtively 
> inline the inner loop, should be verified.
> In b), d) the small inner loop, as demonstrated below, will be JIT-compiled, 
> regardless if moved into separate method.

But you guys noticed that sentence in the initial review request, right?

"Move encoding loop into separate method for which VM will use intrinsic on 
x86."

Just wanted to make sure ;-)

-- Chris

> 
> -Ulf
> 
> 1) Check for (sp >= sl) is superfluous.
> ==
> private static int copyISOs(
>char[] sa, int sp, byte[] da, int dp, int len) {
>int i = 0;
>for (; i < len; i++) {
>char c = sa[sp++];
>// if (c & '\uFF00' != 0) // needs bug 6935994 to be fixed, would be 
> fastest
>if ((byte)(c >> 8) != 0) // temporary replacement, fast byte-length 
> operation on x86
>break;
>da[dp++] = (byte)c;
>}
>return i;
> }
> 
> private CoderResult encodeArrayLoop(
>CharBuffer src, ByteBuffer dst) {
>char[] sa = src.array();
>int soff = src.arrayOffset();
>int sp = soff + src.position();
>int sr = src.remaining();
>int sl = sp + sr;
>byte[] da = dst.array();
>int doff = dst.arrayOffset();
>int dp = doff + dst.position();
>int dr = dst.remaining();
>CoderResult cr;
>if (dr < sr) {
>sr = dr;
>cr = CoderResult.OVERFLOW;
>} else
>cr = CoderResult.UNDERFLOW;
>try {
>int ret = copyISOs(sa, sp, da, dp, sr);
>sp = sp + ret;
>dp = dp + ret;
>if (ret != sr) {
>if (sgp.parse(sa[sp], sa, sp, sl) < 0)
>return sgp.error();
>return sgp.unmappableResult();
>}
>return cr;
>} finally {
>src.position(sp - soff);
>dst.position(dp - doff);
>}
> }
> 
> 2) Avoids sp, dp to be recalculated; shorter surrounding code -> best chance 
> to become JIT-compiled.
> ==
> // while inlinig, JIT will erase the surrounding int[] p
> private static boolean copyISOs(
>char[] sa, byte[] da, final int[] p, int sl) {
>for (int sp = p[0], dp = p[1]; sp < sl; sp++) {
>char c = sa[sp];
>// if (c & '\uFF00' != 0) // needs bug 6935994 to be fixed, would be 
> fastest
>if ((byte)(c >> 8) != 0) // temporary replacement, fast byte-length 
> operation on x86
>return false;
>da[dp++] = (byte)c;
>}
>return true;
> }
> 
> private CoderResult encodeArrayLoop(
>CharBuffer src, ByteBuffer dst) {
>char[] sa = src.array();
>int soff = src.arrayOffset();
>int sp = soff + src.position();
>int sr = src.remaining();
>byte[] da = dst.array();
>int doff = dst.arrayOffset();
>int dp = doff + dst.position();
>int dr = dst.remaining();
>CoderResult cr;
>if (dr < sr) {
>sr = dr;
>cr = CoderResult.OVERFLOW;
>} else
>cr = CoderResult.UNDERFLOW;
>try {
>int sl = sp + sr;
>final int[] p = { sp, dp };
>if (!copyISOs(sa, da, p, sl)) {
>if (sgp.parse(sa[sp], sa, sp, sl) < 0)
>return sgp.error();
>return sgp.unmappableResult();
>}
>return cr;
>} finally {
>src.position(sp - soff);
>dst.position(dp - doff);
>}
> }
> 
> 3) No more needs try...finally block.
> ==
> private CoderResult encodeArrayLoop(
>CharBuffer src, ByteBuffer dst) {
>char[] sa = src.array(

Re: Request for review (XXS): JDK-8004066: TEST_BUG: test/java/lang/Math/DivModTests.java assumes ArithmeticException message

[Christian Thalinger]

On Nov 30, 2012, at 10:48 AM, Krystal Mo  wrote:

> Hi all,
> 
> Could I get a couple of review for this small change, please?
> And could someone from the JDK side sponsor this change?
> 
> Bug: https://jbs.oracle.com/bugs/browse/JDK-8004066
> Webrev: http://cr.openjdk.java.net/~kmo/8004066/webrev.00/

Looks good to me.  -- Chris

> 
> The DivModTest introduced in JDK-6282196 [1] checks the contents of the 
> exception message, but that message isn't specified in JavaDoc and thus may 
> vary between VM implementations (or even in the same VM).
> 
> The issue has affected HotSpot Server VM in nightlies, and the Shark VM. As 
> OpenJDK library code may receive broader adoption in the future, this issue 
> may affect other VM implementations in the future as well.
> 
> (Details:
> The HotSpot Server Compiler may recompile hot throw sites to throw a 
> preallocated exception object, with null exception message, leading to a NPE 
> in the test;
> The bytecode interpreter in Zero/Shark VM throws the ArithmeticException with 
> "/ by long zero" for ldiv, which is different from normal HotSpot behavior 
> (which is expected by the test) where the exception message is "/ by zero".)
> 
> This change relaxes the test so that it's more lenient and less sensitive to 
> the error message produced by the VM.
> I don't think there's a good/reliable way of verifying whether an 
> ArithmeticException came from "division by zero", checking the type should be 
> enough for now.
> 
> Tested with the failing nightly test case and jprt.
> 
> $ java -Xcomp -showversion -XX:DefaultMaxRAMFraction=8 -XX:-TieredCompilation 
> -XX:CompileThreshold=100 -XX:+UnlockExperimentalVMOptions 
> -XX:+IgnoreUnrecognizedVMOptions -XX:+AggressiveOpts 
> -XX:+IgnoreUnrecognizedVMOptions -XX:-UseCompressedOops DivModTests
> java version "1.8.0-ea"
> Java(TM) SE Runtime Environment (build 1.8.0-ea-b65)
> Java HotSpot(TM) 64-Bit Server VM (build 25.0-b11-internal-fastdebug, 
> compiled mode)
> 
> Exception in thread "main" java.lang.NullPointerException
>at DivModTests.resultEquals(DivModTests.java:390)
>at DivModTests.testIntFloorMod(DivModTests.java:126)
>at DivModTests.testIntFloorDivMod(DivModTests.java:103)
>at DivModTests.testIntFloorDivMod(DivModTests.java:68)
>at DivModTests.main(DivModTests.java:45)
> 
> 
> Description from the bug report:
> ~/jdk/test/java/lang/Math$ java -Xint -showversion DivModTests
> java version "1.8.0-ea"
> Java(TM) SE Runtime Environment (build 1.8.0-ea-b65)
> OpenJDK 64-Bit Shark VM (build 25.0-b11-internal, interpreted mode)
> 
> FAIL: long Math.floorDiv(4, 0) = java.lang.ArithmeticException: / by long 
> zero; expected java.lang.ArithmeticException: / by zero
> FAIL: long StrictMath.floorDiv(4, 0) = java.lang.ArithmeticException: / by 
> long zero; expected java.lang.ArithmeticException: / by zero
> FAIL: long Math.floorMod(4, 0) = java.lang.ArithmeticException: / by long 
> zero; expected java.lang.ArithmeticException: / by zero
> FAIL: long StrictMath.floorMod(4, 0) = java.lang.ArithmeticException: / by 
> long zero; expected java.lang.ArithmeticException: / by zero
> Exception in thread "main" java.lang.RuntimeException: 4 errors found in 
> DivMod methods.
> at DivModTests.main(DivModTests.java:49)
> 
> Regards,
> Kris
> 
> [1]: https://jbs.oracle.com/bugs/browse/JDK-6282196

Re: RFR (XXS): 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp

[Christian Thalinger]

On Oct 19, 2012, at 10:42 AM, Mandy Chung  wrote:

> On 10/18/2012 6:11 PM, David Holmes wrote:
>> Hi Martin,
>> 
>> On 19/10/2012 10:58 AM, Martin Buchholz wrote:
>>> http://code.google.com/p/guava-libraries/source/browse/guava-testlib/src/com/google/common/testing/GcFinalization.java
>>>  
>> 
>> That code uses runFinalization in places which means it is not exercising 
>> the primary finalization mechanism. But looking further into the whole thing 
>> I don't think it really makes much difference semantically afterall. All 
>> that changes is which thread executes the finalizer - the object comes from 
>> the same ReferenceQueue regardless.
>> 
>> Which means that runFinalization() will really only help if the main 
>> finalizer thread is delayed executing another finalizer - which for some 
>> tests may be caused by compilation of the finalize method.
>> 
>> So Chris/Mandy: I think the original suggested fix is fine here.
> 
> Thanks David. I looked at the Finalizer class and also concur that what Chris 
> has is fine.
> 
> Chris - do you have the committer right to push to jdk8/tl?  If not, I can 
> push it for you.

Yes, I'm reviewer of jdk8.  But I would appreciate if you could push it.

-- Chris

> 
> Mandy

Re: RFR (XXS): 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp

[Christian Thalinger]

On Oct 18, 2012, at 6:11 PM, David Holmes  wrote:

> Hi Martin,
> 
> On 19/10/2012 10:58 AM, Martin Buchholz wrote:
>> http://code.google.com/p/guava-libraries/source/browse/guava-testlib/src/com/google/common/testing/GcFinalization.java
> 
> That code uses runFinalization in places which means it is not exercising the 
> primary finalization mechanism. But looking further into the whole thing I 
> don't think it really makes much difference semantically afterall. All that 
> changes is which thread executes the finalizer - the object comes from the 
> same ReferenceQueue regardless.
> 
> Which means that runFinalization() will really only help if the main 
> finalizer thread is delayed executing another finalizer - which for some 
> tests may be caused by compilation of the finalize method.
> 
> So Chris/Mandy: I think the original suggested fix is fine here.

Thank you for looking into this and verifying that it's okay.

-- Chris

> 
> Cheers,
> David
> 
> 

Re: RFR (XXS): 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp

[Christian Thalinger]

On Oct 18, 2012, at 4:04 PM, Mandy Chung  wrote:

> On 10/18/2012 3:37 PM, Christian Thalinger wrote:
>> 
>>> Just curious - the test runs with a max of 10 GCs. You
>>> reproduced this bug on a slower machine with fastdebug build.
>>> If you increase the max number of GCs, I wonder how long
>>> the test will take to complete/pass?
>> Around 14 GCs (runtime with -Xcomp approximately 40 seconds on that 
>> particular machine).
> 
> Would increasing the max to 20 fix this bug?  I don't have
> a strong objection to call System.runFinalization.  As I don't
> know what behavior the test wants to cover, adjusting the
> number of max GC invocations seems a good alternative.

I have no idea either.  It's a very old test from Mark.  I would be fine with 
increasing the GC count but for even slower machines (VMs) it might not be 
enough.

-- Chris

> 
> Mandy
> 

Re: RFR (XXS): 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp

[Christian Thalinger]

On Oct 18, 2012, at 3:31 PM, Mandy Chung  wrote:

> Hi Christian,
> 
> Thanks for taking on this bug.

Sure.  I'm trying to get HotSpot nightly failures down.

> 
> Just curious - the test runs with a max of 10 GCs. You
> reproduced this bug on a slower machine with fastdebug build.
> If you increase the max number of GCs, I wonder how long
> the test will take to complete/pass?

Around 14 GCs (runtime with -Xcomp approximately 40 seconds on that particular 
machine).

> 
> With your fix, how many GC does it take to complete?
> I suspect it's one.  

Between 2 and 3 GCs.

> I am guessing that the test might
> want to test that the finalizers are being invoked during
> GC and references are dequeued.  I wonder if there is
> another alternate fix instead of forcing the finalizers
> to be run.  Just a thought.

The fix was not my idea.  I just picked the bug and tried the suggested fix and 
it worked.  The other suggested fix is to increase the sleep time (but I don't 
like that).

-- Chris

> 
> Mandy
> 
> 
> On 10/18/2012 2:43 PM, Christian Thalinger wrote:
>> http://cr.openjdk.java.net/~twisti/6771058
>> 
>> 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp
>> Reviewed-by:
>> 
>> This test can fail if finalizer of Basic is not called for some
>> reason.  It happens in compiled mode if compilation is more slow than
>> execution of the main loop.
>> 
>> The fix is to call System.runFinalization() after System.gc() to
>> ensure that finalizer is run.
>> 
>> test/java/lang/ref/Basic.java
>> 

RFR (XXS): 6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp

[Christian Thalinger]

http://cr.openjdk.java.net/~twisti/6771058

6771058: TEST_BUG: java/lang/ref/Basic.java may fail with -server -Xcomp
Reviewed-by:

This test can fail if finalizer of Basic is not called for some
reason.  It happens in compiled mode if compilation is more slow than
execution of the main loop.

The fix is to call System.runFinalization() after System.gc() to
ensure that finalizer is run.

test/java/lang/ref/Basic.java

Re: RFR (S): 8000999: backport of JSR 292 to 7u

[Christian Thalinger]

On Oct 16, 2012, at 8:59 PM, David Holmes  wrote:

> Hi Christian,
> 
> Is this just a preliminary review request, as actual backport requests have 
> to go to the jdk7u-dev mailing list for approval.

Kind of.  I will pass on the reviewed changes to John Coomes to be integrated 
in a 7u repository to do PIT.  But I guess I have to send a backport request to 
jdk7u-dev as well.

> 
> And are these all just bug fixes, or are there any API/spec changes involved?

No API changes.  Just bug fixes (which replaced the whole implementation behind 
the API).  The HotSpot changes are already in HS24.

-- Chris

> 
> David
> 
> On 17/10/2012 5:54 AM, Christian Thalinger wrote:
>> http://cr.openjdk.java.net/~twisti/8000999
>> 
>> 8000999: backport of JSR 292 to 7u
>> Reviewed-by:
>> 
>> This is an umbrella bug for these changes (which are backported in one
>> changeset):
>> 
>> 6983728: JSR 292 remove argument count limitations
>> 7128512: Javadoc typo in java.lang.invoke.MethodHandle
>> 7117167: Misc warnings in java.lang.invoke and sun.invoke.*
>> 7129034: VM crash with a field setter method with a filterArguments
>> 7087658: MethodHandles.Lookup.findVirtual is confused by interface methods 
>> that are multiply inherited
>> 7127687: MethodType leaks memory due to interning
>> 7023639: JSR 292 method handle invocation needs a fast path for compiled code
>> 7188911: nightly failures after JSR 292 lazy method handle update (round 2)
>> 7190416: JSR 292: typo in InvokerBytecodeGenerator.getConstantPoolSize
>> 7191102: nightly failures after JSR 292 lazy method handle update (round 3)
>> 7194612: 
>> api/java_lang/invoke/MethodHandles/Lookup/index.html#ExceptionsTests[findVirtualNSME]
>>  fails w/ -esa
>> 7194662: JSR 292: PermuteArgsTest times out in nightly test runs
>> 
>> The backport is just copying over the files from JDK 8.  That's why the 
>> webrev is so big and pretty useless.  The real changes between 8 and 7 are 
>> these:
>> 
>> diff -Nur jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java 
>> jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java
>> --- jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java
>> 2012-10-15 12:21:52.806052959 -0700
>> +++ jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java   
>> 2012-10-16 10:48:29.728257304 -0700
>> @@ -94,10 +94,14 @@
>> 
>>  // handy shared exception makers (they simplify the common case code)
>>  /*non-public*/ static InternalError newInternalError(String message, 
>> Throwable cause) {
>> -return new InternalError(message, cause);
>> +InternalError e = new InternalError(message);
>> +e.initCause(cause);
>> +return e;
>>  }
>>  /*non-public*/ static InternalError newInternalError(Throwable cause) {
>> -return new InternalError(cause);
>> +InternalError e = new InternalError();
>> +e.initCause(cause);
>> +return e;
>>  }
>>  /*non-public*/ static RuntimeException newIllegalStateException(String 
>> message) {
>>  return new IllegalStateException(message);
>> diff -Nur jdk8/src/share/classes/sun/invoke/util/ValueConversions.java 
>> jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java
>> --- jdk8/src/share/classes/sun/invoke/util/ValueConversions.java
>> 2012-10-16 10:49:36.081911283 -0700
>> +++ jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java   
>> 2012-10-16 10:48:19.626424849 -0700
>> @@ -1211,9 +1211,13 @@
>> 
>>  // handy shared exception makers (they simplify the common case code)
>>  private static InternalError newInternalError(String message, Throwable 
>> cause) {
>> -return new InternalError(message, cause);
>> +InternalError e = new InternalError(message);
>> +e.initCause(cause);
>> +return e;
>>  }
>>  private static InternalError newInternalError(Throwable cause) {
>> -return new InternalError(cause);
>> +InternalError e = new InternalError();
>> +e.initCause(cause);
>> +return e;
>>  }
>>  }
>> diff --git a/src/share/classes/sun/misc/Unsafe.java 
>> b/src/share/classes/sun/misc/Unsafe.java
>> --- a/src/share/classes/sun/misc/Unsafe.java
>> +++ b/src/share/classes/sun/misc/Unsafe.java
>> @@ -678,6 +678,14 @@
>>  public native Object staticFieldBase(Field f);
>> 
>>  /**
>> + * Detect if the given class may need to be initialized. This is often
>> + * needed in conjunction with obtaining the static field base of a
>> + * class.
>> + * @return false only if a call to {@code ensureClassInitialized} would 
>> have no effect
>> + */
>> +public native boolean shouldBeInitialized(Class c);
>> +
>> +/**
>>   * Ensure the given class has been initialized. This is often
>>   * needed in conjunction with obtaining the static field base of a
>>   * class.
>> 

Re: RFR (M): 8000989: smaller code changes to make future JSR 292 backports easier

[Christian Thalinger]

On Oct 16, 2012, at 8:54 PM, David Holmes  wrote:

> Not sure what relevance there is to hotspot :)

I don't expect anyone from the core library team to actually review these 
changes :-)

> 
> Not meaning to be difficult but why not just apply this change to the 7u code 
> and use the appropriate constructors? As I general rule (there are exceptions 
> eg java.util.concurrent) I don't think the libraries code is written to be 
> directly usable in multiple JDK versions.

It's not about running in different JDK versions.  It's about keeping the 
merging effort to a minimum when we back port future performance work.

-- Chris

> 
> Or even add a package-private InternalError class that subclasses 
> java.lang.InternalError to add the new constructors? (for 7u)
> 
> David
> 
> 
> On 17/10/2012 4:01 AM, Christian Thalinger wrote:
>> http://cr.openjdk.java.net/~twisti/8000989
>> 
>> 8000989: smaller code changes to make future JSR 292 backports easier
>> Reviewed-by:
>> 
>> In 8 we added two new constructors to InternalError which we use in
>> 292.  Factor InternalError generation to a method to make future
>> backports to 7u easier.
>> 
>> src/share/classes/java/lang/invoke/BoundMethodHandle.java
>> src/share/classes/java/lang/invoke/CallSite.java
>> src/share/classes/java/lang/invoke/DirectMethodHandle.java
>> src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
>> src/share/classes/java/lang/invoke/Invokers.java
>> src/share/classes/java/lang/invoke/LambdaForm.java
>> src/share/classes/java/lang/invoke/MemberName.java
>> src/share/classes/java/lang/invoke/MethodHandle.java
>> src/share/classes/java/lang/invoke/MethodHandleImpl.java
>> src/share/classes/java/lang/invoke/MethodHandleStatics.java
>> src/share/classes/sun/invoke/util/ValueConversions.java
>> test/java/lang/invoke/BigArityTest.java
>> test/java/lang/invoke/PrivateInvokeTest.java
>> 

RFR (S): 8000999: backport of JSR 292 to 7u

[Christian Thalinger]

http://cr.openjdk.java.net/~twisti/8000999

8000999: backport of JSR 292 to 7u
Reviewed-by:

This is an umbrella bug for these changes (which are backported in one
changeset):

6983728: JSR 292 remove argument count limitations 
7128512: Javadoc typo in java.lang.invoke.MethodHandle 
7117167: Misc warnings in java.lang.invoke and sun.invoke.* 
7129034: VM crash with a field setter method with a filterArguments 
7087658: MethodHandles.Lookup.findVirtual is confused by interface methods that 
are multiply inherited 
7127687: MethodType leaks memory due to interning 
7023639: JSR 292 method handle invocation needs a fast path for compiled code 
7188911: nightly failures after JSR 292 lazy method handle update (round 2) 
7190416: JSR 292: typo in InvokerBytecodeGenerator.getConstantPoolSize 
7191102: nightly failures after JSR 292 lazy method handle update (round 3) 
7194612: 
api/java_lang/invoke/MethodHandles/Lookup/index.html#ExceptionsTests[findVirtualNSME]
 fails w/ -esa 
7194662: JSR 292: PermuteArgsTest times out in nightly test runs

The backport is just copying over the files from JDK 8.  That's why the webrev 
is so big and pretty useless.  The real changes between 8 and 7 are these:

diff -Nur jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java 
jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java
--- jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java
2012-10-15 12:21:52.806052959 -0700
+++ jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java   
2012-10-16 10:48:29.728257304 -0700
@@ -94,10 +94,14 @@
 
 // handy shared exception makers (they simplify the common case code)
 /*non-public*/ static InternalError newInternalError(String message, 
Throwable cause) {
-return new InternalError(message, cause);
+InternalError e = new InternalError(message);
+e.initCause(cause);
+return e;
 }
 /*non-public*/ static InternalError newInternalError(Throwable cause) {
-return new InternalError(cause);
+InternalError e = new InternalError();
+e.initCause(cause);
+return e;
 }
 /*non-public*/ static RuntimeException newIllegalStateException(String 
message) {
 return new IllegalStateException(message);
diff -Nur jdk8/src/share/classes/sun/invoke/util/ValueConversions.java 
jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java
--- jdk8/src/share/classes/sun/invoke/util/ValueConversions.java
2012-10-16 10:49:36.081911283 -0700
+++ jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java   
2012-10-16 10:48:19.626424849 -0700
@@ -1211,9 +1211,13 @@
 
 // handy shared exception makers (they simplify the common case code)
 private static InternalError newInternalError(String message, Throwable 
cause) {
-return new InternalError(message, cause);
+InternalError e = new InternalError(message);
+e.initCause(cause);
+return e;
 }
 private static InternalError newInternalError(Throwable cause) {
-return new InternalError(cause);
+InternalError e = new InternalError();
+e.initCause(cause);
+return e;
 }
 }
diff --git a/src/share/classes/sun/misc/Unsafe.java 
b/src/share/classes/sun/misc/Unsafe.java
--- a/src/share/classes/sun/misc/Unsafe.java
+++ b/src/share/classes/sun/misc/Unsafe.java
@@ -678,6 +678,14 @@
 public native Object staticFieldBase(Field f);
 
 /**
+ * Detect if the given class may need to be initialized. This is often
+ * needed in conjunction with obtaining the static field base of a
+ * class.
+ * @return false only if a call to {@code ensureClassInitialized} would 
have no effect
+ */
+public native boolean shouldBeInitialized(Class c);
+
+/**
  * Ensure the given class has been initialized. This is often
  * needed in conjunction with obtaining the static field base of a
  * class.

Re: RFR (M): 8000989: smaller code changes to make future JSR 292 backports easier

[Christian Thalinger]

Thank you, John.  After this is in 8 I will send a 7 backport webrev.

-- Chris

On Oct 16, 2012, at 11:11 AM, John Rose  wrote:

> Reviewed; good.  This removes a significant source of friction (mismerges) 
> for sharing changes between 7 and 8.  — John
> 
> On Oct 16, 2012, at 11:01 AM, Christian Thalinger wrote:
> 
>> http://cr.openjdk.java.net/~twisti/8000989
>> 
>> 8000989: smaller code changes to make future JSR 292 backports easier
>> Reviewed-by:
>> 
>> In 8 we added two new constructors to InternalError which we use in
>> 292.  Factor InternalError generation to a method to make future
>> backports to 7u easier.
>> 
>> src/share/classes/java/lang/invoke/BoundMethodHandle.java
>> src/share/classes/java/lang/invoke/CallSite.java
>> src/share/classes/java/lang/invoke/DirectMethodHandle.java
>> src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
>> src/share/classes/java/lang/invoke/Invokers.java
>> src/share/classes/java/lang/invoke/LambdaForm.java
>> src/share/classes/java/lang/invoke/MemberName.java
>> src/share/classes/java/lang/invoke/MethodHandle.java
>> src/share/classes/java/lang/invoke/MethodHandleImpl.java
>> src/share/classes/java/lang/invoke/MethodHandleStatics.java
>> src/share/classes/sun/invoke/util/ValueConversions.java
>> test/java/lang/invoke/BigArityTest.java
>> test/java/lang/invoke/PrivateInvokeTest.java
>> 
> 

RFR (M): 8000989: smaller code changes to make future JSR 292 backports easier

[Christian Thalinger]

http://cr.openjdk.java.net/~twisti/8000989

8000989: smaller code changes to make future JSR 292 backports easier
Reviewed-by:

In 8 we added two new constructors to InternalError which we use in
292.  Factor InternalError generation to a method to make future
backports to 7u easier.

src/share/classes/java/lang/invoke/BoundMethodHandle.java
src/share/classes/java/lang/invoke/CallSite.java
src/share/classes/java/lang/invoke/DirectMethodHandle.java
src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java
src/share/classes/java/lang/invoke/Invokers.java
src/share/classes/java/lang/invoke/LambdaForm.java
src/share/classes/java/lang/invoke/MemberName.java
src/share/classes/java/lang/invoke/MethodHandle.java
src/share/classes/java/lang/invoke/MethodHandleImpl.java
src/share/classes/java/lang/invoke/MethodHandleStatics.java
src/share/classes/sun/invoke/util/ValueConversions.java
test/java/lang/invoke/BigArityTest.java
test/java/lang/invoke/PrivateInvokeTest.java

Re: review request (L): 7030453: JSR 292 ClassValue.get method is too slow

[Christian Thalinger]

On Sep 19, 2011, at 11:58 PM, John Rose wrote:

> http://cr.openjdk.java.net/~jrose/7030453/webrev.00

src/share/classes/java/lang/ClassValue.java:

 233 /** Value stream for for hashCodeForCache.  See similar structure in 
ThreadLocal. */

Two for's.

 578  *  from the ehad of a non-null run, which would allow them

"ehad"?

Otherwise I think this looks good.

-- Christian

> 
> The existing JDK 7 implementation of ClassValue is a place-holder which is 
> defective in several ways:
> - It uses cascaded WeakHashMaps to map from (Class, ClassValue) pairs to 
> values, which is slow.
> - It does not lock the root WeakHashMap, which can cause a race condition the 
> first time a class is encountered.
> - It relies on internal details of WeakHashMap to avoid other race conditions.
> 
> The new implementation uses a concurrently readable cache per Class object 
> with entry versioning to manage lookups.  It is more correct and scalable.
> 
> The tunable parameters CACHE_LOAD_LIMIT and PROBE_LIMIT were chosen by 
> looking at the behavior of artificial workloads.  Experience with real 
> workloads will probably lead to further modifications (under new Change 
> Requests).  I thought of making them tunable from JVM command line 
> properties, but since no other class in java.lang does this, I held back.
> 
> The previous implementation had a store barrier which pushed (via lazySet) 
> pending store values from the user-supplied value before the ClassValue 
> mapping was published.  I removed it because it is a false fix for 
> user-caused race conditions.  (False because it has the desired effect only 
> on some platforms.)  I think it is better to put that issue back onto the 
> user.  We still need a memory fence API to give users the right hook for such 
> problems.
> 
> There is a package-private change to java.lang.Class, adding two new fields 
> (to the existing 19 fields declared in Class.java).
> 
> Although this class is in java.lang, it is part of JSR 292.  Therefore the 
> review comments will be collected in mlvm-dev.  The review request is CC-ed 
> to hotspot-compiler (where JSR 292 changes are pushed) and core-libs (which 
> is responsible for java.lang).
> 
> -- John

crash in Java_sun_awt_X11_XToolkit_getNumberOfButtonsImpl on solaris-sparcv9

[Christian Thalinger]

While testing one of my changes with CompileTheWorld I hit a problem:

CompileTheWorld (1411) : com/sun/java/swing/plaf/motif/ 
MotifInternalFrameTitlePane$Title
= 
= 
= 
= 
= 
= 


Unexpected Error
--
SIGBUS (0xa) at pc=0x76c05264, pid=20849, tid=1

Do you want to debug the problem?

To debug, run 'dbx - 20849'; then switch to thread 1
Enter 'yes' to launch dbx automatically (PATH must include dbx)
Otherwise, press RETURN to abort...
= 
= 
= 
= 
= 
= 



The debugger shows that it seems to be a problem in getNumButtons:

   called from signal handler with signal 10 (SIGBUS) --
  [10] XListInputDevices(0x10118d0d0, 0x7fffb710, 0x1,  
0x10172d218, 0x10172d3d8, 0x10172d554), at 0x76c05264
  [11] getNumButtons() (optimized), at 0x77c1db40 (line ~950)  
in "XToolkit.c"
  [12] Java_sun_awt_X11_XToolkit_getNumberOfButtonsImpl(env = , cls  
= ) (optimized), at 0x77c1da34 (line ~925) in "XToolkit.c"


Is this a known issue?  I couldn't find anything related in Bugster  
but it also could be a VM problem.


-- Christian

core-libs-dev@openjdk.java.net

[Christian Thalinger]

On Mon, 2009-12-14 at 10:16 -0800, Joseph D. Darcy wrote:
> > Not a review, but did you think about implementing the whole FDLIBM in
> > Java, as done here:
> >
> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2009-August/001970.html
> >   
> 
> Yes, porting FDLIBM to Java has been an oft-delayed "nice to have" 
> project of mine.  It is not obvious from looking at my ceil/floor code, 
> but it started with the FDLIBM versions of those algorithms.  The tests 
> are new and greatly outnumber the code changes, as it typical in this 
> line of work :-)  I think getting an all-java StrictMath library would 
> be best done as a series of small batches so floor/ceil could be a start.

All in all sounds good :-)  -- Christian

core-libs-dev@openjdk.java.net

[Christian Thalinger]

On Mon, 2009-12-14 at 01:31 -0800, Joseph D. Darcy wrote:
> Hello.
> 
> Please review my fix for
> 
> 6908131 Pure Java implementations of StrictMath.floor(double) 
> &StrictMath.ceil(double)
> http://cr.openjdk.java.net/~darcy/6908131.0/
> 
> I've asked Doug Priest, one of Sun's numerical experts, to review the 
> floor/ceil algorithm and testing.  I've incorporated his feedback and he 
> has approved those aspects of the changes.  The JDK integration has not 
> yet been looked over.

Not a review, but did you think about implementing the whole FDLIBM in
Java, as done here:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2009-August/001970.html

-- Christian

Re: Character.java methods are slower than Surrogate.java's

[Christian Thalinger]

On Wed, 2009-10-28 at 22:44 +0100, Ulf Zibis wrote:
> Hi Christian,
> 
> for a benchmark test I have replaced all sun.nio.cs.Surrogate methods in 
> sun.nio.cs.ext.EUC_TW with corresponding ones from java.lang.Character.
> 
> As result, the overall encoding performance was 10 % worse, so 
> java.lang.Character's methods must be much worser.
> 
> Can you have a look, if there is a chance to better optimize 
> java.lang.Character methods from HotSpot compiler side.

I think you're already looking into this yourself.  -- Christian

Re: JSR-292: Why not java.lang.dyn?

[Christian Thalinger]

On Sat, 2009-10-03 at 23:43 -0500, Paul Benedict wrote:
> I've always found it a bit perplexing that java.lang was never chosen
> for the parent package of the Dynamic API. Why is that? Dynamic types
> are now "part of the language" as proven by spec itself and exotic
> identifiers. Will this be reconsidered?

[I'm forwarding this question to mlvm-dev.]

I think John Rose or another member of the EG should have an answer to
this.

-- Christian

Re: System.identityHashCode performance

[Christian Thalinger]

Tom Hawtin wrote:
> Janda Martin wrote:
>> I would like to ask about any plans of speed improvements to 
>> "System.identityHashCode" or "IdentityHashMap"
>>
>> I use EclipseLink and it uses IdentityHashMap for storing entities and their 
>> clones.
>>
>> In my case System.identityHashCode consumes 54% of time during insert data 
>> to database. There are lots of other project using IdentityHashMap and 
>> identityHashCode.
> 
> CR 6378256 seems to cover this for the client/C1 compiler. Presumably 
> the server/C2 compiler already intrinsifies the method. It's been in 
> "fix understood" for a couple of years...
> 
> http://bugs.sun.com/view_bug.do?bug_id=6378256

Martin, are you having performance problems with client or server compiler?

-- Christian

Re: Math trig intrinsics and compiler options

[Christian Thalinger]

gustav trede wrote:
> Hello,

You should better ask these questions on hotspot-dev.

-- Christian