RFR (XS): 8003879: Duplicate definitions in vmStructs

Mon Nov 26 16:07:17 PST 2012

No worries, thanks for clarifying and for the review(s)!

Cheers,
Mikael

On 11/26/2012 3:00 PM, serguei.spitsyn at oracle.com wrote:
> 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/f2d40822/attachment-0001.html 