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

Konstantin Shefov konstantin.shefov at oracle.com
Mon Jan 11 13:34:10 UTC 2016 

Kindly reminder.

Already approved by C. Thalinger and I. Ignatyev.

Thanks
-Konstantin

On 12/17/2015 11:26 AM, Konstantin Shefov wrote:
> 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
>

More information about the core-libs-dev mailing list