JMC-5640: Used Heap After GC Chart

[Henrik Dafgård]

Hi all,

I agree that there should be a better way to do this, but for now this
looks good with the minor change of moving the Attribute and Query to the
GarbageCollectionsPage instead of having them in core. I think it's far too
confusing to have an attribute called HEAP_USED_POST_GC that's a duplicate
of HEAP_USED in the core APIs, this is just going to lead to hard to
identify bugs for downstream applications in the future.


Cheers,
Henrik Dafgård


On Wed, 7 Aug 2019 at 15:39, Mario Torre <neugens at redhat.com> wrote:

> 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
>