7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules

[Suchitainf]

This PR addresses the NullPointerException issue in core module for the following rules:

1. GCs Caused by System.gc()
2. GCs Caused by Heap Inspection
3. GC Stall
4. String Deduplication
5. GCs Caused by GC Locker

---

Progress

• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JMC-7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/302/head:pull/302
$ git checkout pull/302

Update a local copy of the PR:
$ git checkout pull/302
$ git pull https://git.openjdk.java.net/jmc pull/302/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 302

View PR using the GUI difftool:
$ git pr show -t 302

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/302.diff

[bridgekeeper]

👋 Welcome back schaturvedi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 00: Full (4cb40418)

[aptmac]

I think these changes are okay, although they address the issues with the individual rules rather than the system itself. I'd appreciate if @Gunde could lend some expertise or insight here.

From my time looking through the application-side code, the RuleManager makes two checks to determine if a rule should be evaluated [0], and these are to:

1. check the rule's availability, which happens in RulesToolkit.matchesEventAvailabilityMap() [1], and
2. check the rule's dependencies (which happens in RuleManager.shouldEvaluate() [2])

However, the core implementation that leads into JfrHtmlRulesReport does not perform these two checks, which results in the errors for this issue.

To address problem 1 listed above, we're in luck because the matchesEventAvailabilityMap() is in core already in RulesToolkit, so this could be used when evaluating rules at RulesToolkit.evaluateParallel() [3]. This alleviates the issue from the StringDeduplicationRule, although it's still probably nice to have the null check included in this PR just incase something happens. Additionally, for the JFR file I was using to test this bug, by checking the rules with matchesEventAvailabilityMap() I was able to bypass several rules (including StringDeduplicationRule) that looked to be handled properly within their own rule implementation however could be omitted in my case. For context they were: Class Loading Pressure, Fatal Errors, Heap Content, DMS Incidents, Class Leak, Code Cache, Heap Dump, Competing Processes, Exceptional Dump Reason, Java Blocking, Socket Write Peak Duration, Process Started, Primitive To Object Conversion. I'm thinking that adding a check for the rules before evaluating might be nice for future-proofing if new rules are potentially added in the future.

To address problem 2, it's not as easy as reusing the shouldEvaluate() function because that lies in application, but also because it checks currently evaluated rules to determine if a rule depends on another one that has already ran, which isn't checked in core. There are only four rules with dependencies, and those are the four GC rules that are adjusted in this PR, so this is an okay work around. However, it might be nice to have a way of checking dependent rules here just incase new ones are made down the line.

[0] https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L138
[1] https://github.com/openjdk/jmc/blob/master/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java#L494
[2] https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L107
[3] https://github.com/openjdk/jmc/blob/master/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java#L1213

[Gunde]

The ResultProvider should always be available for a IRule imlementation. If it isn't then that's a bug that we need to fix instead. I'd rather fix that at the source than adding null checks all over the code.

[thegreystone]

@Gunde - is there an issue open for this?

[Suchitainf]

Already fixed as part of another PR.