RFR (S): 8010294: Refactor HeapInspection to make it more reusable

[Mikael Gerdin]

Erik,


On 2013-03-25 09:32, Erik Helin wrote:
> All,
>
> I have updated the change based on internal feedback.
>
> The changes between webrev.00 and webrev.01 are mostly in the test code:
> - The test uses correct @summary description
> - The test now uses the correct command line flags
> - The test is more documented to describe how the test code is run
>
> There is also a very minor change in the hotspot code. I am now using
> the operator != instead of operator > when comparing the missed count.
> This was done just to make the diff one line smaller :)
>
> Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8010294/webrev.01/

This looks good,
I have some small comments but I'll leave it to you to decide if you 
want to fix them or not.

When you're changing this line, would you mind adding a space before "true"?
   _elements = new (ResourceObj::C_HEAP, mtInternal) 
GrowableArray<KlassInfoEntry*>(_histo_initial_size,true);


Did you consider using
SharedHeap::heap() instead of:
     SharedHeap* sh = (SharedHeap*)Universe::heap();


Either way, ship it.
/Mikael


>
> Thanks,
> Erik
>
> On 03/22/2013 10:06 AM, Erik Helin wrote:
>> Hi all,
>>
>> I have refactored the HeapInspection class to prepare for the
>> vm/gc/detailed/object_count_after_gc event.
>>
>> The refactoring mainly consists of breaking up the rather long
>> heap_inspect function to multiple small functions that can be reused.
>>
>> I also moved the public enums used to initialize KlassInfoTable and
>> KlassInfoHisto to private constants. Now they no longer have to be
>> passed as an argument to the constructor.
>>
>> I also added a test to ensure that my refactoring did not break anything
>> (the test is not that extensive but at least it ensures that the basic
>> stuff is working).
>>
>> Webrev:
>> http://cr.openjdk.java.net/~ehelin/8010294/webrev.00/
>>
>> Bug:
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8010294
>>
>> Testing:
>> - JPRT
>> - A new jtreg test (see webrev)
>>
>> Thanks,
>> Erik
>