JMC-5640: Used Heap After GC Chart

[Mario Torre]

On 30/07/2019 22:48, Jie Kang wrote:
> On Tue, Jul 23, 2019 at 10:29 AM Ken Dobson <kdobson at redhat.com> wrote:
>>
>> Hi all,
>>
>> Here is a patch that adds a Used Heap Post GC chart to the Garbage
>> Collections page.
>> The implementation is a bit hacky but the only way I could find to add a
>> variation of an attribute already being displayed on a page was to create a
>> duplicate attribute in JdkAttributes.java as well as compare names for
>> uniqueness as identifiers are no longer unique if we intend to look at
>> different subsets of the same attribute.
>>
>> If anyone has any ideas as to how to enact this change in a way that allows
>> us to maintain consistency with the way the other pages operate as well as
>> a way to avoid multiple copies of attributes feel free to share them.
>>
>> Bug Id: https://bugs.openjdk.java.net/browse/JMC-5640
>> Webrev: http://cr.openjdk.java.net/~kdobson/JMC-5640/webrev/
> 
> Hey Ken,
> 
> One minor nit:
> 
> 
> DataPageToolkit.buildLinesRow(Messages.GarbageCollectionsPage_ROW_HEAP,
> 
> Messages.GarbageCollectionsPage_ROW_HEAP_DESC, allItems, false,
> HEAP_SUMMARY,
>  legendFilter,
>                                         UnitLookup.BYTE.quantity(0),
> null).ifPresent(rows::add);
> +                       DataPageToolkit.buildLinesRow("Heap Post GC",
> +                                       "Heap Post GC", allItems,
> false, HEAP_SUMMARY_POST_GC, legendFilter,
> +                                       UnitLookup.BYTE.quantity(0),
> null).ifPresent(rows::add);
> 
> You should add a String for "Heap Post GC" to Messages and reference
> that instead, like how it's done above your addition. The rest looks
> fine to me.

I think the patch is good to go as is too, with this minor fix.

> Regarding the 'hacky' Attributes addition, it's a bit complicated. The
> toolkit code being used doesn't cover the situation where you are
> showing the same attribute filtered two different ways in the same
> chart. Though it can be confusing, the distinction does need to be
> made somewhere along the line. I think this patch is okay but I could
> see an issue made to address this use-case more cleanly. Trying to
> re-work the existing code might take some time.

The approach is a bit "hacky" indeed, I'm not sure what would be the
best approach either short of refactoring this whole code. Perhaps
Marcus has some suggestion?

Cheers,
Mario


-- 
Mario Torre
Associate Manager, Software Engineering
Red Hat GmbH <https://www.redhat.com>
9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898