RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

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/324e6d1c/attachment-0001.html