Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7122: Rules evaluation never complete #291

Closed
wants to merge 1 commit into from
Closed

Conversation

@aptmac
Copy link
Member

@aptmac aptmac commented Jul 28, 2021

This PR addresses JMC-7122 [0], in which certain rules (mainly GC) do not finish evaluation.

This occurs when the result [1] of a rule is never updated, and still holds it's initialized value of IN_PROGRESS despite exiting the evaluation loop. What happens in the case of the 4 GC rules listed in the bug report (1. GCs Caused by System.gc(), 2. GCs Caused by Heap Inspection, 3. GC Stall, 4. GCs Caused by GC Locker) is that they have a dependency on GarbageCollectionInfoRule, which ends up being ignored [2] because it doesn't match the event availability map. As a result, the 4 GC rules cause shouldEvaluate(rule) [3] to return false, but because the result value was already initialized it isn't caught by the subsequent if-statements.

In this approach, the conditional is adjusted to check the event availability map and shouldEvaluate() at the same time, because if either of them are false we should ignore that rule.

Before:
2021-07-28-160356_1489x270_scrot

After:
2021-07-28-160243_1087x303_scrot

[0] https://bugs.openjdk.java.net/browse/JMC-7122
[1] https://github.com/aptmac/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L133
[2] https://github.com/aptmac/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L164
[3] https://github.com/aptmac/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L143


Progress

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

Issue

  • JMC-7122: Rules evaluation never complete

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 291

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 28, 2021

👋 Welcome back aptmac! 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.

@openjdk openjdk bot added the rfr label Jul 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 28, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@aptmac This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7122: Rules evaluation never complete

Reviewed-by: jpbempel, hirt, hdafgard

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jul 29, 2021
Copy link
Member

@thegreystone thegreystone left a comment

Looks good to me. What’s the thread sleep doing there though?

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jul 29, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jul 29, 2021

@Gunde - maybe you know?

Gunde
Gunde approved these changes Jul 29, 2021
Copy link
Collaborator

@Gunde Gunde left a comment

This looks good to me, we should investigate whether or not that thread sleep is needed at all, but that can be a separate issue.

@aptmac
Copy link
Member Author

@aptmac aptmac commented Jul 29, 2021

Looks good to me. What’s the thread sleep doing there though?

I'm thinking it was there to reduce the frequency of looping to see if the result had resolved by then. FWIW the sleep has been there since JMC was open sourced: https://github.com/openjdk/jmc/blame/65187e9448cf9b8ed9b629295965fd8db63262f6/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L121

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jul 29, 2021

@aptmac - gogogo. :)

@aptmac
Copy link
Member Author

@aptmac aptmac commented Jul 29, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

Going to push as commit eafd075.

@openjdk openjdk bot closed this Jul 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 29, 2021

@aptmac Pushed as commit eafd075.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@aptmac aptmac deleted the 7122 branch Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants