8312585: Rename DisableTHPStackMitigation flag to THPStackMitigation

[shipilev]

The flag newly added by JDK-8312182 is prompting the use of -XX:+DisableTHPStackMitigation to disable the THP stack mitigation, thus allowing THP in thread stacks. This double negation does not read well, and not in line with other mitigation flags like IntelJccErratumMitigation.

It would be better to rename the flag to avoid double-negation, before it proliferates to other JDK releases.

(I would have the same comment during the original review, but missed it :P)

Additional testing:

• GHA
• Linux x86_64 fastdebug, affected test

---

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-8312585: Rename DisableTHPStackMitigation flag to THPStackMitigation (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14992

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

• hotspot-runtime

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 (5756558e)
• 00: Full (75b831e3)

[shipilev]

FYI @tstuefe.

[dholmes-ora]

I disagree. This name is exactly what is intended - we disable a mitigation/workaround. It's name is consistent with previous flags of this kind DisableXXXWorkAround IIRC.

[dholmes-ora]

I also don't see a "double negation" here - naming a flag DisableX is not in itself double-negation.

[tstuefe]

I also like the flag as it is. If "mitigation" bothers you, you could name it "DisableTHPStackFix" or similar".

[shipilev]

It was confusing at least for me, because I had to pause and think what exactly -XX:-DisableTHPStackMitigation would mean. It is disabling (-) the disablement (Disable) of the mitigation, so it actually enables it? And yes, I do dislike Use*, Disable* and Enable* prefixes for something that is already covered by binary (+|-) because of collisions like these.

In my opinion, (+|-)IntelJccErratumMitigation did it right, and we should follow that one.

[tstuefe]

Okay, sounds reasonable too. If you can convince David, I'm fine with either outcome.

I know, not very helpful :-)

[dholmes-ora]

I understand how you see it this way but that is, I presume, because you read the +/- as enable/disable. But +Foo simply means "set Foo to true". -Foo means "set Foo to false". If you read it that way there is no confusion: -DisableTHPStackMitigation means DisableTHPStackMitigation==false ie we are not disabling the mitigation, hence it is enabled.

To me the use of Enable/Disable can convey additional information about the nature of the flag - if the flag is DisableX then I would expect X to normally be enabled but in some rare cases we may need to disable it, hence the flag conveys that. And yes you can abuse that by having the flag and the default value of the flag the wrong way around - such is life. To me if the flag is just X then it suggests, albeit there must be a default for X, that there is some ambivalence about whether X should be enabled or not - "hey X is enabled by default, but if you want to turn it off that's fine, your call."

I fully understand both points of view here, but there is plenty of precedence for using Use/Enable/Disable prefixes on flags. And Use in particular is far too entrenched to be displaced.

[stefank]

FWIW, I agree with Aleksey and I don't think we should be adding flags prefixed with Disable.

[tstuefe]

Note that the original name was already discussed a lot and went through several changes.

I am fine with whatever variant we come up (all alternatives sound sensible to me), but it would be good if we resolved that soon before starting to backport JDK-8312182.

[shipilev]

The current positions seem to be: strong "rename, dropping Disable" from me; "rename from Disable" from Stefan; "keep it" from David; "abstain" from Thomas. So while this technically leans toward renaming, I don't think this overrides David's objection to the change. So unless somebody else puts in a strong position "for" or "against", or current folks change their positions, we are in a bind.

[dholmes-ora]

We've had some internal discussion and the general position is "no negative naming" and a preference I believe for "Use" as a prefix if a prefix is needed/desired. There will probably be a style-guide update proposed for the general case. So my objection is overridden and so withdrawn. :)

[openjdk]

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

8312585: Rename DisableTHPStackMitigation flag to THPStackMitigation

Reviewed-by: dholmes, stuefe

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

• dc01604: 8305636: Expand and clean up predicate classes and move them into separate files
• a38fdaf: 8166900: If you wrap a JTable in a JLayer, the cursor is moved to the last row of table by you press the page down key.

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

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

[shipilev]

We've had some internal discussion and the general position is "no negative naming" and a preference I believe for "Use" as a prefix if a prefix is needed/desired. There will probably be a style-guide update proposed for the general case. So my objection is overridden and so withdrawn. :)

Thank you for initiating the discussion, and triggering the future style guide change. It was a long time coming :)

I am going to re-test this thing, while waiting for more approvals.

[dholmes-ora]

Thank you for initiating the discussion, and triggering the future style guide change. It was a long time coming :)

Can't take the credit, that was @stefank .

[tstuefe]

Good.

Thanks to @dholmes-ora for resolving the discussions, I'm happy we agree.

[shipilev]

RISC-V GHA failure is the infrastructural one, to be fixed with another PR.
I think we are good with integrating this one.

/integrate

[openjdk]

Going to push as commit 226cdc6.
Since your change was applied there have been 2 commits pushed to the master branch:

• dc01604: 8305636: Expand and clean up predicate classes and move them into separate files
• a38fdaf: 8166900: If you wrap a JTable in a JLayer, the cursor is moved to the last row of table by you press the page down key.

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 226cdc6.

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