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

8261075: Create stubRoutines.inline.hpp with SafeFetch implementation #2542

Closed

Conversation

@AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented Feb 12, 2021

Hi,

Please reivew a small non-functional change that extracts inline SafeFetch functions to a separate file. This is preliminary work for JEP-391 integration that will reduce the size of that patch.

CC @dcubed-ojdk

Thanks!


Progress

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

Issue

  • JDK-8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2542/head:pull/2542
$ git checkout pull/2542

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 12, 2021

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

Loading

@openjdk openjdk bot added the rfr label Feb 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 12, 2021

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

  • hotspot

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.

Loading

@openjdk openjdk bot added the hotspot label Feb 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 12, 2021

Webrevs

Loading

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 12, 2021

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

8261075: Create stubRoutines.inline.hpp with SafeFetch implementation

Reviewed-by: dcubed, stuefe, stefank

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

  • 6b6f794: 8248223: KeyAgreement spec update on multi-party key exchange support
  • 8ba390d: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
  • 16bd7d3: 8261336: IGV: enhance default filters
  • 3f8819c: 8261501: Shenandoah: reconsider heap statistics memory ordering
  • 3cbd16d: 8259668: Make SubTasksDone use-once
  • 219b115: 8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
  • cdc874d: 8261601: free memory in early return in Java_sun_nio_ch_sctp_SctpChannelImpl_receive0
  • e2d52ae: 8261413: Shenandoah: Disable class-unloading in I-U mode
  • 34ae7ae: 8261609: remove remnants of XML-driven builders
  • 6badd22: 8261351: Create implementation for NSAccessibilityRadioButton protocol
  • ... and 41 more: https://git.openjdk.java.net/jdk/compare/f4cfd758342a9afc8cc0fb2bb400ed8c791e0588...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 (@dcubed-ojdk, @tstuefe, @stefank) 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).

Loading

@openjdk openjdk bot added the ready label Feb 12, 2021
Copy link
Member

@tstuefe tstuefe left a comment

I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Feb 15, 2021

I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.

Good idea. It should be safefetch.inline.hpp, right?

Loading

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Feb 15, 2021

I would rename the new header safefetch.hpp, just because these functions were never part of the StubRoutines namespace, and the naming is much clearer.

Good idea. It should be safefetch.inline.hpp, right?

I don't think so? To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header. Here, we just have some independent global utility functions.

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Feb 15, 2021

To my understanding, xxx.inline.hpp is a companion file to an xxx.hpp which you create if you want to remove some of the rarely used functions from a high traffic header.

Oh, you're right. I had an impression that there are standalone inline.hpp files, and I could not find the opposite in the Hotspot Style Guide. However, now I see inline.hpp are just the way you've described. Thanks!

Loading

@stefank
Copy link
Member

@stefank stefank commented Feb 15, 2021

If you put non-trivial code in the header, then it should go into an .inline.hpp file (or .cpp file). From the style-guide:

Do not put non-trivial function implementations in .hpp files. If the implementation depends on other .hpp files, put it in a .cpp or a .inline.hpp file.

It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Feb 16, 2021

It doesn't matter if there exists a .hpp file or not. However, what's non-trivial becomes a judgment call. I tend to use rule that if it uses code from another header, then it's non-trivial, and should most likely be moved out of the .hpp file.

I suppose this file to be on the edge between trivial and not. Later, it will have a W^X transition and will include thread.hpp, I don't want to rename it again. @stefank, what do you think, should it be safefetch.inline.hpp? Or are you fine with safefetch.hpp?

Loading

@stefank
Copy link
Member

@stefank stefank commented Feb 16, 2021

I'm fine with leaving it as safefetch.hpp.

Loading

Copy link
Member

@tstuefe tstuefe left a comment

LGTM

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Feb 16, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Feb 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 16, 2021

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

Loading

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Still thumbs up.

Loading

@VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Feb 17, 2021

/sponsor

Loading

@openjdk openjdk bot closed this Feb 17, 2021
@openjdk openjdk bot added integrated and removed sponsor labels Feb 17, 2021
@openjdk openjdk bot removed the ready label Feb 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 17, 2021

@VladimirKempik @AntonKozlov Since your change was applied there have been 59 commits pushed to the master branch:

  • d195033: 8261842: Shenandoah: cleanup ShenandoahHeapRegionSet
  • fc1d032: 8261125: Move VM_Operation to vmOperation.hpp
  • d547e1a: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods
  • 2677f6f: 8261675: ObjectValue::set_visited(bool) sets _visited false
  • e7e20d4: 8261711: Clhsdb "versioncheck true" throws NPE every time
  • 55d7bbc: 8261607: SA attach is exceeding JNI Local Refs capacity
  • 0a50688: 8241372: Several test failures due to javax.net.ssl.SSLException: Connection reset
  • 61a659f: 8260415: Remove unused class ReferenceProcessorMTProcMutator
  • 6b6f794: 8248223: KeyAgreement spec update on multi-party key exchange support
  • 8ba390d: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
  • ... and 49 more: https://git.openjdk.java.net/jdk/compare/f4cfd758342a9afc8cc0fb2bb400ed8c791e0588...master

Your commit was automatically rebased without conflicts.

Pushed as commit b955f85.

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

Loading

@openjdk openjdk bot removed the rfr label Feb 17, 2021
@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Mar 4, 2021

I've messed up with this :( I'm trying to fix @stefank 's note #2200 (comment). I need to rename a file.hpp included in the safepoint.hpp to file.inline.hpp. This will require renaming safepoint.hpp to safepoint.inline.hpp, polluting the PR #2200 again and defeating the purpose of having this patch extracted. I think the better alternative will be a follow-up patch just renaming safepoint.hpp to safepoint.inline.hpp. I'm going to do the follow-up. Or is there a better alternative? Sorry and thanks.

Loading

@stefank
Copy link
Member

@stefank stefank commented Mar 4, 2021

Just so that I understand. Did you really mean safepoint.hpp and not safefetch.hpp? I assume this all has to do with the fact that safefetch.hpp is going to include threadWXSetter.hpp, which you are going to change to threadWXSsetter.inline.hpp, because it includes thread.inline.hpp. If that's the case then I think the easy fix is to just rename safefetch.hpp to safefetch.inline.hpp.

I don't think that will be problematic, or defeat the purpose of this patch. The previous change from stubRoutines.hpp to stubRoutines.inline.hpp has already been successfully been removed from your #2200 patch. If you create a new PR with a safefetch.hpp to safefetch.inline.hpp rename, I can review it as a trivial change and you will be able to push it immediately.

Or did I misunderstand anything? Maybe you could point me to the problematic files?

Loading

@AntonKozlov
Copy link
Member Author

@AntonKozlov AntonKozlov commented Mar 5, 2021

Just so that I understand. Did you really mean safepoint.hpp and not safefetch.hpp?

Definitely safefetch.hpp. Sorry, it seems I was thinking also about another problem at that moment :)

I assume this all has to do with the fact that safefetch.hpp is going to include threadWXSetter.hpp, which you are going to change to threadWXSsetter.inline.hpp, because it includes thread.inline.hpp. If that's the case then I think the easy fix is to just rename safefetch.hpp to safefetch.inline.hpp.

This is correct. My main concern was the noise I'm creating in the git repository, but I also think this is the best way in this situation. Thanks for confirmation! The bug is JDK-8263068 and PR is #2844.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants