JMC-6256: JMC doesn't respect Long.MIN_VALUE as a missing value

[Marcus Hirt]

Looks good Alex! Please go ahead!

 

Kind regards,

Marcus

 

From: Alex Macdonald <almacdon at redhat.com>
Date: Thursday, 13 December 2018 at 22:16
To: Marcus Hirt <marcus.hirt at oracle.com>, <jmc-dev at openjdk.java.net>
Subject: Re: JMC-6256: JMC doesn't respect Long.MIN_VALUE as a missing value

 

Whoops, just realized I hit reply instead of reply-all so this didn't get sent to list, please find attached an updated patch.

On Thu, Dec 13, 2018 at 10:00 AM Alex Macdonald <almacdon at redhat.com> wrote:

Hi Marcus,

 

Thanks for the review and suggestion, I've amended the patch to keep the string constants inside TypeHandling.

 

On Wed, Dec 12, 2018 at 4:48 PM Marcus Hirt <marcus.hirt at oracle.com> wrote:

Hi Alex,

Quick comment before going to bed:

I don't believe we should externalize Integer.MIN_VALUE etc. They are defined 
in the java core libraries. One could argue for MISSING_VALUE and 
MISSING_VALUE_TOOLTIP to be externalized though.

Kind regards,
Marcus


On 2018-12-12, 22:10, "jmc-dev on behalf of Alex Macdonald" <jmc-dev-bounces at openjdk.java.net on behalf of almacdon at redhat.com> wrote:

    Hi!

    This patch addresses JMC-6256 [0], in which MIN_VALUEs are displayed in
    their numeric form instead of being displayed as a N/A or missing value.
    For testing purposes, I have been using the memoryleak.jfr recording that
    is available as an attachment on JMC-6127 [1].

    As per the comments in the bug report, there are 6 cases to consider for
    missing values in JMC [2]. Additionally, it may be nice to show the actual
    value in parenthesis in the tooltip [3].

    Screenshots (album [4]):
    Before: https://imgur.com/aoGgzRn [5]
    After: https://imgur.com/DVnHQGZ [6]

    The culprit here seems to be the way the conditionals in the
    "getValueString()" [7][8] end up processing the value. There is a
    duplication of the "instanceof IDisplayable" in the JfrPropertySheet [7]
    and the TypeHandling [8], and the TypeHandling.getValueString() ends up
    being called anyways in the JfrPropertySheet.getValueString(). As a result,
    the conditional to see if it's an IDisplayable from the JfrPropertySheet
    happens before the conditional to see if it is a min value [9] (which is
    where we want to end up in this case).

    This patch removes the duplicated conditional from the JfrPropertySheet,
    because it is already correctly handled in TypeHandling. Additionally, a
    function "getNumericString" has been introduced for the display of the
    tooltip. If the value is an IQuantity then "getNumericString" will figure
    out which missing value type it is, and display it in the tooltip. If the
    value is an IQuantity but not a missing value, then it delegates to
    TypeHandling.getValueString(). I've also included a quick unit test to
    verify the behaviour of "getNumericString".

    Thoughts?

    Cheers,

    Alex

    [0] https://bugs.openjdk.java.net/browse/JMC-6256
    [1] https://bugs.openjdk.java.net/browse/JMC-6127
    [2]
    https://bugs.openjdk.java.net/browse/JMC-6256?focusedCommentId=14229297&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14229297
    [3]
    https://bugs.openjdk.java.net/browse/JMC-6256?focusedCommentId=14229296&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14229296
    [4] https://imgur.com/a/3BZNHnG
    [5] https://imgur.com/aoGgzRn
    [6] https://imgur.com/DVnHQGZ
    [7]
    http://hg.openjdk.java.net/jmc/jmc/file/a76a464b3764/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrPropertySheet.java#l307
    [8]
    http://hg.openjdk.java.net/jmc/jmc/file/efeb1e97bec3/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/util/TypeHandling.java#l171
    [9]
    http://hg.openjdk.java.net/jmc/jmc/file/efeb1e97bec3/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/util/TypeHandling.java#l167