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

Thu Dec 17 08:26:54 UTC 2015

Hi Coleen

You have previously reviewed this enhancement and made a few comments
I have resolved them, so could you look at the webrevs again, please?

I have added tests, removed cp tags that are not in JVM spec (100 - 105) 
and made some other changes.

JDK: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04
HOTSPOT: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02

Thanks
-Konstantin

On 12/16/2015 07:42 PM, Christian Thalinger wrote:
> 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