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

Coleen Phillimore coleen.phillimore at oracle.com
Mon Jan 11 23:38:09 UTC 2016 

This seems fine.  Only one minor comment (where I don't need to see 
another webrev)

+ constantPoolHandle cp = constantPoolHandle(THREAD, 
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));

would be shorter as

+ constantPoolHandle cp(THREAD, 
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));

and avoid implicit copy construction.

Thanks for adding tests.

Coleen

On 1/11/16 8:34 AM, Konstantin Shefov wrote:
> 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