[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: Review Request (S) 8006542: JSR 292: the VM_RedefineClasses::append_entry()
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2013-01-30 23:15:57
Message-ID: 5109A9AD.2080203 () oracle ! com
[Download RAW message or body]

Thanks, Coleen!
Serguei

On 1/30/13 3:04 PM, Coleen Phillimore wrote:
>
> This looks really good!  Thanks for doing the refactoring!
> Coleen
>
> On 1/28/2013 11:28 PM, serguei.spitsyn@oracle.com wrote:
>> Coleen,
>>
>> This is a version with a refactoring you requested:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.3/ 
>>
>>
>> Now, the NameAndType, FieldRef, InterfaceMethodRef and MethodRef use 
>> the find_or_append_indirect_entry().
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 1/28/13 2:18 PM, serguei.spitsyn@oracle.com wrote:
>>> Hi Coleen,
>>>
>>>
>>> This is a new webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.2/ 
>>>
>>>
>>> As you pointed out, the InvokeDynamic entry support should also do 
>>> cross-checks with
>>> the bootstrap method operands (arguments) and recursive extra 
>>> appends if necessary.
>>>
>>> I've filed a separate bug to track this issue:
>>>     https://jbs.oracle.com/bugs/browse/JDK-8007037
>>>
>>> I want to separate this issue to be able to integrate what I have 
>>> now and concentrate on the rest later.
>>> The VM SQE team developed new INDY tests that cover the 
>>> RedefineClasses issues.
>>> I hope to adopt and use new tests soon to make sure most of the 
>>> issues are actually resolved.
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 1/24/13 5:23 PM, serguei.spitsyn@oracle.com wrote:
>>>> Hi Coleen,
>>>>
>>>>
>>>> Thank you a lot for the review!
>>>>
>>>> On 1/24/13 3:55 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Hi Serguei,
>>>>>
>>>>> Putting functions in alphabetical order seems silly, it's better 
>>>>> to have utility functions directly above (or below) the function 
>>>>> that calls them.  I'd take out the comment.
>>>>>
>>>>> I have started looking at this code a bit.  This function 
>>>>> find_or_append_indirect_entry() should be used for the other 
>>>>> indirect entries also, shouldn't it?  The code looks familiar to 
>>>>> the inside of the case statement to FieldRef, InterfaceRef and 
>>>>> MethodRef.
>>>>
>>>> You've got it right.
>>>> Yes, I noticed it but did not want to mess with it until it is 
>>>> proven to work well.
>>>> My plan was to fix it for the FieldRef, InterfaceRef and MethodRef 
>>>> in a next round of fixes.
>>>>
>>>>>
>>>>> Also is boot_spec an indirect index too that has to be appended?
>>>>>
>>>>>  564       int boot_spec = 
>>>>> scratch_cp->invoke_dynamic_bootstrap_method_ref_index_at(scratch_i);
>>>>
>>>> This is a nice catch, will fix it.
>>>> I thought, it is an index into the operands array, but it refers to 
>>>> a CONSTANT_MethodHandle element.
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>> On 01/24/2013 05:56 PM, serguei.spitsyn@oracle.com wrote:
>>>>>> Christian,
>>>>>>
>>>>>>
>>>>>> Thank you a lot for the review!
>>>>>>
>>>>>> Nice catch with the ordering.
>>>>>> In fact, I tried to follow the order but missed that the 
>>>>>> find_new_index should go first. :)
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 1/24/13 2:14 PM, Christian Thalinger wrote:
>>>>>>> On Jan 22, 2013, at 4:07 PM, serguei.spitsyn@oracle.com wrote:
>>>>>>>
>>>>>>>> Please, review the fix for:
>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8006542
>>>>>>>>
>>>>>>>>
>>>>>>>> Open webrev:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8006542-JVMTI-JSR292.1/ 
>>>>>>>>
>>>>>>> src/share/vm/prims/jvmtiRedefineClasses.hpp:
>>>>>>>
>>>>>>> +  // Support for constant pool merging (these routines are in 
>>>>>>> alpha order):
>>>>>>>     void append_entry(constantPoolHandle scratch_cp, int scratch_i,
>>>>>>>       constantPoolHandle *merge_cp_p, int *merge_cp_length_p, 
>>>>>>> TRAPS);
>>>>>>> +  int find_or_append_indirect_entry(constantPoolHandle 
>>>>>>> scratch_cp, int scratch_i,
>>>>>>> +    constantPoolHandle *merge_cp_p, int *merge_cp_length_p, 
>>>>>>> TRAPS);
>>>>>>>     int find_new_index(int old_index);
>>>>>>>
>>>>>>> Not really alpha order ;-)
>>>>>>>
>>>>>>> Otherwise this looks good (as far as I can tell).
>>>>>>>
>>>>>>> -- Chris
>>>>>>>
>>>>>>>> Summary:
>>>>>>>>   Need a support for invokedynamic entry kinds when new and old 
>>>>>>>> constant pools are merged.
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>   vm/mlvm/indy/func/jvmti/redefineClassInBootstrap
>>>>>>>>   Asked the VM SQE team to develop new INDY tests for better 
>>>>>>>> coverage.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]