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

Konstantin Shefov konstantin.shefov at oracle.com
Wed Dec 16 11:13:41 UTC 2015 

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