RFR (XS): 8003879: Duplicate definitions in vmStructs

[serguei.spitsyn at oracle.com]

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/02c470e1/attachment.html