RFR (S): 8004747: Remove last_entry from VM_STRUCT macros

[serguei.spitsyn at oracle.com]

Mikael,

It looks good.
It is also a nice simplification!

The only thing I'd suggest is to be more specific in the assert at the 
end of the vmStructs.cpp.
Instead of "Incorrect last entry" it'd be better to have "Incorrect 
struct last entry", etc.

Also, it'd be useful if Yumin had a chance to surfacely look at the fix.

Thanks,
Serguei


On 12/28/12 5:17 PM, Mikael Vidstedt wrote:
>
> Thanks David!
>
> Still need a 2nd reviewer!
>
> Cheers,
> Mikael
>
> On 12/28/2012 3:34 PM, David Holmes wrote:
>> Mikael,
>>
>> That all sounds fine to me.
>>
>> Thanks,
>> David
>>
>> On 29/12/2012 3:48 AM, Mikael Vidstedt wrote:
>>>
>>> David,
>>>
>>> I did two things (both on linux/x86_64 only):
>>>
>>> 1. I compared the pre-processor output manually. Unfortunately there 
>>> are
>>> line numbers sprinkled around in the pre-processed file which makes
>>> comparing the output a bit more complex than it should be. Modulo the
>>> line numbers and some white space changes the outputs matche.
>>> 2. I wrote a small test which mimics what the SA does - it looks up the
>>> four exported structures dynamically and reads the data. Apart from the
>>> "address" field in the VMStructEntry entries the outputs almost [1]
>>> match. The address field is expected to vary since it reflects the
>>> actual placement in memory of various static variables.
>>>
>>> Are there other experiments you can think of?
>>>
>>> Cheers,
>>> Mikael
>>>
>>> [1] Using this tool I actually found one bug in the table - one entry
>>> (ClassLoaderDataGraph::_unloading) is declared as nonstatic when it is
>>> indeed static. I'll file a separate bug for that.
>>>
>>> On 12/11/2012 8:16 PM, David Holmes wrote:
>>>> Hi Mikael,
>>>>
>>>> This looks okay to me - I like the simplification. However as I don't
>>>> perform mental macro substitution particularly well (okay, not at all
>>>> ;-)) I was wondering if there was a simple validation test where we
>>>> could compare the pre-processed output before and after?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 11/12/2012 7:01 AM, Mikael Vidstedt wrote:
>>>>>
>>>>> Please review the following change:
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~mikael/8004747/webrev.00/
>>>>>
>>>>> Background:
>>>>>
>>>>> This patch does 3 things (depending on how you count it):
>>>>>
>>>>> 1. Move the call to generating the last entry for the VM structures
>>>>> database
>>>>>
>>>>> The vmStructs functionality is a macro based way of creating a 
>>>>> database
>>>>> of information about VM internal structures, types and constants.
>>>>>
>>>>> The database for structures is defined in runtime/vmStructs.cpp in an
>>>>> array called localHotSpotVMStructs. The content of the array is
>>>>> generated with calls to a few different macros (VM_STRUCTS,
>>>>> VM_STRUCTS_PARALLELGC, VM_STRUCTS_CMS, VM_STRUCTS_G1, 
>>>>> VM_STRUCTS_CPU and
>>>>> VM_STRUCTS_OS_CPU respectively). Common for all these macros is 
>>>>> the fact
>>>>> that the last argument passed in is another macro
>>>>> (GENERATE_VM_STRUCT_LAST_ENTRY) which is a generator for the end
>>>>> marker/last entry in the database; a special entry which can be 
>>>>> easily
>>>>> located by an external consumer of the information.
>>>>>
>>>>> Even though the end marker generator macro
>>>>> (GENERATE_VM_STRUCT_LAST_ENTRY) is passed to all the VM_STRUCT* 
>>>>> macros,
>>>>> the actual end marker must be generated once and only once. The 
>>>>> way this
>>>>> is currently handled is that only the last VM_STRUCT macro, which
>>>>> happens to be VM_STRUCTS_OS_CPU, actually makes use of its last 
>>>>> argument
>>>>> and adds it to the end of the array.
>>>>>
>>>>> Not only is this fairly complex to understand, it's also more fragile
>>>>> than it needs to be.
>>>>>
>>>>> This change removes the last argument for all the VM_STRUCT macros 
>>>>> and
>>>>> instead explicitly inserts the end marker at the end of the
>>>>> localHotSpotVMStructs array.
>>>>>
>>>>> The same VM_STRUCTS macros are being used in VMStructs::init to do 
>>>>> type
>>>>> checking. Instead of passing the end marker generator into the macros
>>>>> the last parameter is instead CHECK_SENTINEL, which is defined to 
>>>>> expand
>>>>> to nothing, which means it can be safely removed.
>>>>>
>>>>> 2. Move the end marker generating for the VM types, VM int 
>>>>> constants and
>>>>> VM long constants databases
>>>>>
>>>>> Repeat the exact above story, but replace anything called 
>>>>> Structs/STRUCT
>>>>> with Types/TYPES, IntConstants/INT_CONSTANTS and
>>>>> LongConstants/LONG_CONSTANTS respectively.
>>>>>
>>>>> 3. Minor prettification
>>>>>
>>>>> In addition to the above the change also removes some superfluous
>>>>> backslashes from a few of the VM_STRUCT* macro calls.
>>>>>
>>>>> Thanks,
>>>>> Mikael
>>>>>
>>>
>

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