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

Tue Dec 15 17:36:45 UTC 2015

> On Dec 14, 2015, at 11:11 PM, Konstantin Shefov <konstantin.shefov at 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/~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 <konstantin.shefov at oracle.com <mailto:konstantin.shefov at 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;
>>>>>> 
>>>>>> 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 <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.01>
>>>>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.01 <http://cr.openjdk.java.net/%7Ekshefov/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" <christian.thalinger at oracle.com <mailto:christian.thalinger at oracle.com>>
>>>>>>>>> À: "Konstantin Shefov" < <mailto:konstantin.shefov at oracle.com>konstantin.shefov at oracle.com <mailto:konstantin.shefov at oracle.com>>
>>>>>>>>> Cc: "hotspot-dev developers" < <mailto:hotspot-dev at openjdk.java.net>hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>>, core-libs-dev at openjdk.java.net <mailto:core-libs-dev at 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
>>>>>>>>>> <konstantin.shefov at oracle.com <mailto:konstantin.shefov at oracle.com>> 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 <https://bugs.openjdk.java.net/browse/JDK-8141615>
>>>>>>>>>> Webrev hotspot:
>>>>>>>>>> http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00 <http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.00>
>>>>>>>>>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00 <http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00>
>>>>>>>>>> 
>>>>>>>>>> Thanks
>>>>>>>>>> -Konstantin
>>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 