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

[Daniel D. Daugherty]

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.

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]'?

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/serviceability-dev/attachments/20130715/e436e001/attachment-0001.html