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

Christian Thalinger christian.thalinger at oracle.com
Wed Dec 16 16:42:49 UTC 2015 

Looks good.  Thanks.

> On Dec 16, 2015, at 1:13 AM, Konstantin Shefov <konstantin.shefov at oracle.com> 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 <konstantin.shefov at oracle.com <mailto: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/%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 <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).
>>>>> 
>>>>> 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" <konstantin.shefov at oracle.com>
>>>>>>>>>>> Cc: "hotspot-dev developers" <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
>>>>>>>>>>>> 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
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> -Konstantin
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>

More information about the core-libs-dev mailing list