8339067: Convert Threshold flags (like Tier4MinInvocationThreshold and Tier3MinInvocationThreshold) to double

[offamitkumar]

This is trivial PR to change data type of some "*Threshold" variables form intx to double.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8339067: Convert Threshold flags (like Tier4MinInvocationThreshold and Tier3MinInvocationThreshold) to double (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21354/head:pull/21354
$ git checkout pull/21354

Update a local copy of the PR:
$ git checkout pull/21354
$ git pull https://git.openjdk.org/jdk.git pull/21354/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21354

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21354.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@offamitkumar 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.

[offamitkumar]

I did a build, fastdebug-vm, with ubsan-enabled which was successful. I also tried to ran tier1, but I got failures, a lot of failures due to ubsan being enabled on s390x.

[mlbridge]

Webrevs

• 02: Full - Incremental (97d63a29)
• 01: Full - Incremental (ce4ff580)
• 00: Full (a53535f5)

[offamitkumar]

Hi,
Can I get reviews for this trivial change.

[vnkozlov]

Any reasons Tier2*Threshold flags were bit changed? For consistency.

[offamitkumar]

Any reasons Tier2*Threshold flags were bit changed? For consistency.

I guess you're asking why I left them unchanged? I looked into the project, and couldn't find where those flags are being used, so I left them unchanged at first. However, I've now updated them to double as well. Thanks for the suggestion.

[eme64]

Instead of changing the product flags (is a CSR needed for that?), you could also just cast to double at every use site. Would that also work?

[vnkozlov]

Instead of changing the product flags (is a CSR needed for that?), you could also just cast to double at every use site. Would that also work?

Yes, we need CSR for these changes if we do as they are now. Have cast or assign to local variable is preferable, I agree.

[offamitkumar]

you could also just cast to double at every use site. Would that also work?

is that required ? Aren't integers, by default, will be treated as double if they are multiplied by a double data type value ?

[vnkozlov]

you could also just cast to double at every use site. Would that also work?

is that required ? Aren't integers, by default, will be treated as double if they are multiplied by a double data type value ?

Yes, you are right. This RFE could be NOP. My suggestion in JDK-8333098 PR was based on assumption that these flags may cause rounding issue if they are used in integer expressions. Or result of double expression is converted into integer.
But you show that they used with double value scale which cast these flags values into double and only in compare expressions.
The only other place I found is:

compilationPolicy.hpp:  static int min_invocations() { return Tier4MinInvocationThreshold; }

But it is used only double expression again:

bytecodeInfo.cpp:      double min_freq = MAX2(MinInlineFrequencyRatio, 1.0 / CompilationPolicy::min_invocations());

[vnkozlov]

I think we should close this RFE as "Not an Issue".

Change !=0 to > in assert may not be need too because there is check for negative value in ciMethod.cpp:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciMethod.cpp#L148

[offamitkumar]

I think we should close this RFE as "Not an Issue".

Sure I will close it. Thanks for the inputs :-)