8273139: C2: assert(f <= 1 && f >= 0) failed: Incorrect frequency

[eme64]

Relaxed assert which can be triggered falsely upon an unfortunate sequence of float additions, such that the sum of all probabilities exceeds 1 by some very small amount in the order of float precision.

Before: assert(f <= 1 && f >= 0)
Now: assert(f >= 0), truncate f to be <=1

Frequency computation is done with floats, which are calculated through counting occurrences and dividing by totals.
This is done in multiple locations in the code.

We considered three options (in conversation with @rwestrel @chhagedorn @tobiasholenstein ):

1. Consistently use fesetround to correctly round up/down (depends on context) if the results are ever used for frequency/probability calculations (many locations, multiple files). This option requires more code and maintainance. Implementing and testing is difficult. This is fragile if new code is added that impacts frequency computation - would have to remember to round correctly.
2. Modify assert to f <= 1+eps && 1 >= 0, where eps is related to float precision, and is as close to zero as possible. However, since there can be an arbitrary number of additions, this error could grow arbitrarily. It is thus not evident how to determine eps.
3. Drop the f <= 1 condition (our choice). The exact comparison is inadequate for floats, and there is no evident replacement. This requires less code, is easier to maintain. Disadvantage: a developer may break something and not realize because the assert is no longer present. The impact would most likely be limited to performance, and not crash the VM or cause incorrect execution.

Checked that tests are not affected.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8273139: C2: assert(f <= 1 && f >= 0) failed: Incorrect frequency

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to a2a4229
• Christian Hagedorn (@chhagedorn - Reviewer)
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7113

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

Using diff file

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

[bridgekeeper]

👋 Welcome back eme64! 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]

@eme64 The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (b26bb66a)
• 00: Full (a2a42290)

[TobiHartmann]

Nice analysis and summary! Looks good.

[openjdk]

@eme64 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:

8273139: C2: assert(f <= 1 && f >= 0) failed: Incorrect frequency

Reviewed-by: thartmann, chagedorn, roland

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 40 new commits pushed to the master branch:

• 5af7f25: 8274811: Remove superfluous use of boxing in java.base
• 44fe958: 6465404: some problems in CellEditor related API docs
• b0496b0: 8279970: two AppCDS tests fail after JDK-8261455
• 4eb4f94: 8279956: Useless method Scheduling::ComputeLocalLatenciesForward()
• 4f4da3b: 8275318: loaded_classes_do may see ArrayKlass before InstanceKlass is loaded
• 3a421e4: 8280122: SupportedGroupsExtension should output "named groups" rather than "versions"
• 1a20628: 8248404: AArch64: Remove uses of long and unsigned long
• 46fd683: 8176567: nsk/jdi/ReferenceType/instances/instances002: TestFailure: Unexpected size of referenceType.instances(nsk.share.jdi.TestInterfaceImplementer1): 11, expected: 10
• e314a4c: 8280124: Reduce branches decoding latin-1 chars from UTF-8 encoded bytes
• bdfa15d: 8250801: Add clhsdb "threadcontext" command
• ... and 30 more: https://git.openjdk.java.net/jdk/compare/3edcb13272c7d1a587e17fc16be523b3d73053ac...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @chhagedorn, @rwestrel) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[chhagedorn]

Great analysis! Looks good to me, too.

[dean-long]

Would it make sense to use better precision (double instead of float) for intermediate values, to make the problem happen less often?
Also, if prob > 1.0, is that something that would be useful to output to the compilation log?

[eme64]

Thanks for your comment, @dean-long.

I do not think that changing from float to double makes the problem occur less often. The errors will be smaller, yes, but there are still going to be rounding issues, and f <= 1 could still occasionally be violated because of rouding.
We could log all cases where f > 1. But most of these cases are rounding errors and not bugs.

The only use of double I can see is if we use the option 2, with f <= 1+eps. The double mantissa has 52bit instead of 23bit for float. Maybe we can find an eps large enough so that it can never be exceeded by rounding errors, over all operations (add, mul, etc.), while still keeping it small enough that the condition is useful to detect actual bugs. If we assume we only have adds, with a maximal rounding error of 2^-52, and we expect at most 2^32 additions, then we could have eps=2^-20. However, we would have to be sure that these assumptions hold, and also hold in the future.

Changing from float to double is not trivial. We seem to use float to do counts in multiple files, and other code depends on it as well. A quick grep "float.*cnt" . -rE gets me a good number of hits, and not just in the file I edited (loopPredicate.cpp).

@dean-long : what would you recommend?

@rwestrel : what do you think?

[rwestrel]

Looks good to me.

[rwestrel]

Changing from float to double is not trivial. We seem to use float to do counts in multiple files, and other code depends on it as well. A quick grep "float.*cnt" . -rE gets me a good number of hits, and not just in the file I edited (loopPredicate.cpp).

I think Dean's suggestion is to only to use double locally that is in PathFrequency::to(). I suppose that would be feasible. What do you think?

In any case, this logic is used to help c2 decides whether a transformation is beneficial or not. In case of precision errors, worst case, that decision will be slightly wrong and performance should be minimally affected.

As far as logging precision errors we can detect: I don't think it's telling us something that's actionable. It doesn't even suggest that something has gone wrong. So I don't think it's useful.

[eme64]

I think Dean's suggestion is to only to use double locally that is in PathFrequency::to(). I suppose that would be feasible. What do you think?

That would be feasible. But it will not fix the issue, since bad rounding can already happen outside (computing the probabilities and counts). This is most likely why this bug was reported: inside the method we paid attention to round correctly, but the outside numbers most likely had rounding errors, such that if they were added in some adversarial order, they add up to f>1.

Assume a float comes in with a rounding error of 2^-23. If it is converted to a double now in this method, it still still has a rounding error of 2^-23, right? Most likely the mantissa of 23bit is just extended with zeros to 52bit mantissa.

[dean-long]

Based on the extra discussion, it sounds like my suggestions don't really help, which is completely acceptable. Ship it!

[eme64]

Thanks @dean-long @rwestrel for the comments and help :)

[eme64]

/integrate

[openjdk]

@eme64
Your change (at version b26bb66) is now ready to be sponsored by a Committer.

[TobiHartmann]

/sponsor

[openjdk]

Going to push as commit 68b40ec.
Since your change was applied there have been 41 commits pushed to the master branch:

• 39b1d75: 8277822: Remove debug-only heap overrun checks in os::malloc and friends
• 5af7f25: 8274811: Remove superfluous use of boxing in java.base
• 44fe958: 6465404: some problems in CellEditor related API docs
• b0496b0: 8279970: two AppCDS tests fail after JDK-8261455
• 4eb4f94: 8279956: Useless method Scheduling::ComputeLocalLatenciesForward()
• 4f4da3b: 8275318: loaded_classes_do may see ArrayKlass before InstanceKlass is loaded
• 3a421e4: 8280122: SupportedGroupsExtension should output "named groups" rather than "versions"
• 1a20628: 8248404: AArch64: Remove uses of long and unsigned long
• 46fd683: 8176567: nsk/jdi/ReferenceType/instances/instances002: TestFailure: Unexpected size of referenceType.instances(nsk.share.jdi.TestInterfaceImplementer1): 11, expected: 10
• e314a4c: 8280124: Reduce branches decoding latin-1 chars from UTF-8 encoded bytes
• ... and 31 more: https://git.openjdk.java.net/jdk/compare/3edcb13272c7d1a587e17fc16be523b3d73053ac...master

Your commit was automatically rebased without conflicts.

[openjdk]

@TobiHartmann @eme64 Pushed as commit 68b40ec.

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