Request for review:8020309:Remove InstanceKlass::_cached_class_file_len and record the length together with the cached class file bytes

[Jiangli Zhou]

Hi Dan,

Thanks for the review!

On 07/15/2013 03:45 PM, Daniel D. Daugherty wrote:
> On 7/12/13 2:55 PM, Jiangli Zhou wrote:
>> Hi Ioi and Serguei,
>>
>> Here is the updated the webrev:
>>
>> http://cr.openjdk.java.net/~jiangli/8020309/webrev.01/.
>
> Thumbs up!
>
> src/share/vm/classfile/classFileParser.cpp
>     nit line 3960: &ptr, &end_ptr,
>     nit line 3961: &cached_class_file);
>         Now that 'cached_class_file' is shorter, you can probably fit
>         lines 3960 and 3961 on the same line without being longer than
>         80 chars.

Fixed.

>
> src/share/vm/oops/instanceKlass.cpp
>     No comments.
>
> src/share/vm/oops/instanceKlass.hpp
>     No comments.
>
> src/share/vm/prims/jvmtiExport.cpp
>     line 620: p = (JvmtiCachedClassFileData *)os::malloc(
>     line 621:   offset_of(JvmtiCachedClassFileData, data) + _curr_len, 
> mtInternal);
>         The whole JvmtiCachedClassFileData* overlay trick reminds me of
>         UDP and/OR TCP packet processing code. Much cleaner.
>
> src/share/vm/prims/jvmtiExport.hpp
>     No comments.
>
> src/share/vm/prims/jvmtiRedefineClasses.cpp
>     No comments.
>
> src/share/vm/prims/jvmtiRedefineClasses.hpp
>     line 522: return cache == NULL ? NULL : cache->data;
>         Will a picky compiler want '&cache->data[0]' to get
>         a 'char *' instead of a 'char [1]'?

I think we are safe in the case. We use this kind of define quite 
commonly in CDC VM (probably CLDC VM as well), all compilers handle it 
properly.

Thanks!
Jiangli

>
> Dan
>
>
>>
>> I've retested with nsk.jvmti, nsk.jdi and nsk.hprof. I'm rerunning 
>> the JPRT and rest of the tests.
>>
>> Thanks,
>> Jiangli
>>
>> On 07/11/2013 01:14 AM, serguei.spitsyn at oracle.com wrote:
>>> Hi Jiangli,
>>>
>>> I like the Ioi's sugestion.
>>> It will help to avoid the manipulations with offsets that look as 
>>> unnecessary complexity.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 7/10/13 6:39 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> That's good suggestion. Let me look into it.
>>>>
>>>> Thanks!
>>>>
>>>> Jiangli
>>>>
>>>> On 07/10/2013 05:53 PM, Ioi Lam wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> I think it's better to move the logic of deciding the length into 
>>>>> JVMTI. E.g., have something like
>>>>>
>>>>> struct JvmtiCachedClassFileData {
>>>>>    int _length;
>>>>>    unsigned char data[1];
>>>>> };
>>>>>
>>>>> void 
>>>>> instanceKlass::set_jvmti_cached_class_file_data(JvmtiCachedClassFileData 
>>>>> *data);
>>>>>
>>>>> - Ioi
>>>>>
>>>>> On 07/10/2013 04:58 PM, Jiangli Zhou wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the webrev for JDK-8020309 
>>>>>> <https://jbs.oracle.com/bugs/browse/JDK-8020309>:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jiangli/8020309/webrev.00/
>>>>>>
>>>>>> The _cached_class_file_bytes and _cached_class_file_len are not 
>>>>>> set/used if no jvmti agent is attached. The 
>>>>>> _cached_class_file_len field can be eliminated and the file 
>>>>>> length can be recorded as part of the cached class data in 
>>>>>> _cached_class_file_bytes for redefined class. The above change 
>>>>>> allocates extra 4bytes when allocating memory for cached class 
>>>>>> file and stores the length as the first 4bytes and the file data 
>>>>>> as the rest. It saves 4 bytes per class. For classes redefined, 
>>>>>> the memory usage is the same.
>>>>>>
>>>>>> Tested with following tests and JPRT:
>>>>>>
>>>>>>     jdk/test/java/lang/instrument
>>>>>>     jdk/test/com/sun/jdi
>>>>>>     nsk.jvmti
>>>>>>     nsk.jdi
>>>>>>     nsk.hprof
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130715/a90e109c/attachment.html