RFR (XS): 8003879: Duplicate definitions in vmStructs

Mon Nov 26 15:00:42 PST 2012

Mikael,

Sorry for the confusion. I see what you mean.
I missed that you made it consistent with the current style in these files.
Agree, that is the best you can do.

Thanks,
Serguei

On 11/26/12 2:37 PM, Mikael Vidstedt wrote:
>
> Serguei,
>
> I'm confused. When I look at the latest webrev 
> (http://cr.openjdk.java.net/~mikael/8003879/webrev.02/) I believe the 
> indentation is correct - correct being defined as aligned on the first 
> letter of the type name. Please clarify what you expect if you do not 
> agree.
>
> Thanks,
> Mikael
>
> On 11/26/2012 12:21 PM, serguei.spitsyn at oracle.com wrote:
>>
>> Looks good.
>> However, it'd be nice to fix the indentation commented by David (use 
>> frames to notice it):
>>
>> *vmStructs_cms.hpp*:
>> 68            declare_type(AFLBinaryTreeDictionary, FreeBlockDictionary<FreeChunk>)
>>
>> *jni.cpp:*
>> 2110            declare_type(MetablockTreeDictionary, FreeBlockDictionary<Metablock>)
>>
>> Thanks,
>> Serguei
>>
>>
>> On 11/22/12 5:04 PM, Mikael Vidstedt wrote:
>>>
>>> On 2012-11-22 16:35, David Holmes wrote:
>>>> Hi Mikael,
>>>>
>>>> I prefer this as an internal test too. Though I don't understand 
>>>> why execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - 
>>>> this is an enhancement to the VM interface not the JNI interface 
>>>> isn't it? (Separate issue of course).
>>>
>>> I think we'll find that when we get a few more tests in there it's 
>>> time to look at breaking it out into a separate file completely or 
>>> do it in a completely different way, but that's for another day. I 
>>> do hope it's soon though.
>>>
>>>> BTW the weird indentation makes more sense viewing frames rather 
>>>> than diffs :)
>>>
>>> Right :)
>>>
>>>>
>>>> Looks good to go.
>>>
>>> Thanks! Eagerly awaiting a second review!
>>>
>>> /Mikael
>>>
>>>>
>>>> David
>>>>
>>>> On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:
>>>>>
>>>>> I gave it some more thought and decided to try what it would look 
>>>>> like
>>>>> having the test be part of the ExecuteInternalVMTests family 
>>>>> instead. I
>>>>> like this one better personally, but comments are appreciated!
>>>>>
>>>>> http://cr.openjdk.java.net/~mikael/8003879/webrev.02
>>>>>
>>>>> Cheers,
>>>>> Mikael
>>>>>
>>>>> On 2012-11-22 14:25, Mikael Vidstedt wrote:
>>>>>>
>>>>>> New webrev available here:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mikael/8003879/webrev.01
>>>>>>
>>>>>> Cheers,
>>>>>> Mikael
>>>>>>
>>>>>> On 2012-11-22 14:19, Mikael Vidstedt wrote:
>>>>>>> On 2012-11-21 20:17, David Holmes wrote:
>>>>>>>> Hi Mikael,
>>>>>>>>
>>>>>>>> On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:
>>>>>>>>>
>>>>>>>>> Please review the below change.
>>>>>>>>>
>>>>>>>>> The change for 7045397 introduced a couple of duplicate 
>>>>>>>>> entries in the
>>>>>>>>> vmStructs::localHotSpotVMTypes array. This shows up when using 
>>>>>>>>> the
>>>>>>>>> jmap
>>>>>>>>> tool in a rather ugly way:
>>>>>>>> <snip>
>>>>>>>>
>>>>>>>> Other than an indentation problem (which I don't think you
>>>>>>>> introduced) this fixup seems fine.
>>>>>>>
>>>>>>> For some reason that's the standard for declaring types in that 
>>>>>>> long
>>>>>>> list - aligning on the first letter of the type name. I followed 
>>>>>>> suit.
>>>>>>>
>>>>>>>>
>>>>>>>>> In addition to removing the two duplicated entries I also added a
>>>>>>>>> simple, naive runtime test to walk through and make sure no 
>>>>>>>>> type is
>>>>>>>>> repeated. The VMStructs::init only called in debug_only so 
>>>>>>>>> there's no
>>>>>>>>> startup overhead in product, but it may be better to turn the 
>>>>>>>>> test
>>>>>>>>> into
>>>>>>>>> a unit test and only running it as part of 
>>>>>>>>> ExecuteInternalVMTests.
>>>>>>>>> Feedback appreciated!
>>>>>>>>
>>>>>>>> I have a few comments on this part:
>>>>>>>>
>>>>>>>> - Array indices should be int's not size_t (ie signed not 
>>>>>>>> unsigned)
>>>>>>>
>>>>>>> Will fix.
>>>>>>>
>>>>>>>> - I can't clearly see how the localHotSpotVMTypes array is 
>>>>>>>> declared
>>>>>>>> or filled but I assume there is guaranteed to be a sentinel 
>>>>>>>> entry at
>>>>>>>> the end so that we don't index past the end?
>>>>>>>
>>>>>>> Yeah. I'm not sure you want to know. It took me a few minutes to
>>>>>>> figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY
>>>>>>> macro which generates the end marker is passed to VM_TYPES,
>>>>>>> VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is 
>>>>>>> allowed to
>>>>>>> actually allowed to call the macro and actually generate the 
>>>>>>> marker,
>>>>>>> meaning only the "implementation" of VM_TYPES_OS_CPU actually 
>>>>>>> has the
>>>>>>> call to last_entry(). Not sure why the end marker isn't just
>>>>>>> explicitly added at the end instead, but I think I'll queue that up
>>>>>>> for later.
>>>>>>>
>>>>>>> All in all, as far as I can tell there is one and only one end 
>>>>>>> marker
>>>>>>> and it is generated as part of the expansion of VM_TYPES_OS_CPU.
>>>>>>>
>>>>>>>> - assert(0, ...) should be assert(false, ...) (as per style guide
>>>>>>>> [1] ;-) )
>>>>>>>
>>>>>>> Will fix.
>>>>>>>
>>>>>>>> That all said I'm not sure this test belongs there, but I don't 
>>>>>>>> feel
>>>>>>>> strongly either way.
>>>>>>>
>>>>>>> Me neither :)
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Mikael
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> David
>>>>>>>>
>>>>>>>> [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide
>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~mikael/8003879/webrev.00/
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Mikael
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121126/8e36da4e/attachment.html 