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

8300088: [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE #17361

Closed
wants to merge 6 commits into from

Conversation

gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Jan 10, 2024

On current macOS (> 10.6) we can use RLIM_INFINITY for setrlimit(RLIMIT_NOFILE), even though the man page for setrlimit(2) claims otherwise.

The only wrinkle here is that some terminals (ksh) will crash with RLIM_INFINITY, because that value overflows int type, which they use internally, causing crash, so we work around that by using INT_MAX instead of RLIM_INFINITY.


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-8300088: [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17361

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

Using diff file

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

Webrev

Link to Webrev Comment

Sorry, something went wrong.

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 10, 2024

👋 Welcome back gziemski! 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
Copy link

openjdk bot commented Jan 10, 2024

@gerard-ziemski 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.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 10, 2024
@gerard-ziemski gerard-ziemski marked this pull request as ready for review January 10, 2024 21:58
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 10, 2024
@mlbridge
Copy link

mlbridge bot commented Jan 10, 2024

Webrevs

@jaikiran
Copy link
Member

Hello Gerard,

The only wrinkle here is that some terminals (ksh) will crash with RLIM_INFINITY

A note about that - that bug has been fixed in upstream ksh last year ksh93/ksh#591 and appears to have made it in the 1.0.6 release https://github.com/ksh93/ksh/releases/tag/v1.0.6 which was released 6 months back. Of course that doesn't mean all relevant setups/systems will be using this new ksh version and I believe it will take time for that fix to make it into recent releases (In fact, I think Apple has its own fork of ksh).

@gerard-ziemski
Copy link
Author

Hello Gerard,

The only wrinkle here is that some terminals (ksh) will crash with RLIM_INFINITY

A note about that - that bug has been fixed in upstream ksh last year ksh93/ksh#591 and appears to have made it in the 1.0.6 release https://github.com/ksh93/ksh/releases/tag/v1.0.6 which was released 6 months back. Of course that doesn't mean all relevant setups/systems will be using this new ksh version and I believe it will take time for that fix to make it into recent releases (In fact, I think Apple has its own fork of ksh).

Thank you for the note - good to hear that it has been fixed in new ksh, however, the workaround will need to stick for a long while now I think.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised structure looks good. Just some minor suggestions. Thanks.

src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
@gerard-ziemski
Copy link
Author

Revised structure looks good. Just some minor suggestions. Thanks.

Thank you for more feedback.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks

@openjdk
Copy link

openjdk bot commented Jan 13, 2024

@gerard-ziemski 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:

8300088: [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE

Reviewed-by: dholmes, fparain, dcubed

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

  • 52523d3: 8324050: Issue store-store barrier after re-materializing objects during deoptimization
  • df370d7: 8314329: AgeTable: add is_clear() & allocation spec, and relax assert to allow use of 0-index slot
  • 0d8543d: 8324065: Daylight saving information for Africa/Casablanca are incorrect
  • c9cacfb: 8323657: Compilation of snippet results in VerifyError at runtime with --release 9 (and above)
  • bde650f: 8322282: Incorrect LoaderConstraintTable::add_entry after JDK-8298468
  • be943a9: 8321984: IGV: Upgrade to Netbeans Platform 20
  • d3b2ac1: 8314186: runtime/8176717/TestInheritFD.java failed with "Log file was leaked"
  • 72f1990: 8323057: Recoverable errors may be reported before unrecoverable errors when annotation processing is skipped
  • c84af49: 8324129: C2: Remove some ttyLocker usages in preparation for JDK-8306767
  • fd37262: 8323748: RISC-V: Add Zfh probe code
  • ... and 173 more: https://git.openjdk.org/jdk/compare/aba19334eaeb46d37169cddeef929b13e050a60e...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.

➡️ 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 Pull request is ready to be integrated label Jan 13, 2024
@guw
Copy link

guw commented Jan 13, 2024

@gerard-ziemski Thank you very much for working on this. How does it work, will this change be backported to JDK 21 & 17 branches or will it be on main only?

@gerard-ziemski
Copy link
Author

@gerard-ziemski Thank you very much for working on this. How does it work, will this change be backported to JDK 21 & 17 branches or will it be on main only?

I believe that backport issues will need to be manually created for this purpose if you want them.

@dcubed-ojdk
Copy link
Member

@gerard-ziemski - if you think that this fix is appropriate to backport to JDK21u
(and JDK17u), then talk to @calvinccheung and get his opinion also since he works
on backports for the Runtime team.

@dcubed-ojdk
Copy link
Member

Just to be clear, Calvin works on the backports into the Oracle CPU releases
and not necessarily the OpenJDK21u or OpenJDK17u releases. Oracle folks
have sometimes done backports there, but that's not the typical flow...

@gerard-ziemski
Copy link
Author

@gerard-ziemski - if you think that this fix is appropriate to backport to JDK21u (and JDK17u), then talk to @calvinccheung and get his opinion also since he works on backports for the Runtime team.

Thank you Dan! I will check with Calvin...

Copy link
Contributor

@fparain fparain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of nit typos. Otherwise, thumbs up.
Don't forget to check for a copyright year update.

What's the current value of OPEN_MAX and INT_MAX
on macOS these days?

src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
src/hotspot/os/bsd/os_bsd.cpp Outdated Show resolved Hide resolved
@gerard-ziemski
Copy link
Author

LGTM

That you!

@gerard-ziemski
Copy link
Author

gerard-ziemski commented Jan 19, 2024

Just a couple of nit typos. Otherwise, thumbs up. Don't forget to check for a copyright year update.

What's the current value of OPEN_MAX and INT_MAX on macOS these days?

Thank you Dan for the feedback!

The current values on macOS Sonoma (14.2.1) appear to be:

OPEN_MAX: 10240
INT_MAX: 2147483647 (0111 1111 1111 1111 1111 1111 1111 1111)

@dcubed-ojdk
Copy link
Member

Does this: OPEN_MAX: 10240 mean that the JVM is limited to that few open
file descriptors on the macOS releases with that value? Just confirming my
understanding...

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thumbs up.

@gerard-ziemski
Copy link
Author

Does this: OPEN_MAX: 10240 mean that the JVM is limited to that few open file descriptors on the macOS releases with that value? Just confirming my understanding...

Unfortunately, that's the case currently.

@gerard-ziemski
Copy link
Author

Can I get final OK after comments fixes please?

@dcubed-ojdk
Copy link
Member

Can I get final OK after comments fixes please?
I already re-reviewed... @dholmes-ora and/or @fparain ...

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general rule for comments is that you either use sentences and so start with a capital and end with a period, or you just have free text. You should avoid starting with a capital but omitting the period; and vice-versa.

@gerard-ziemski
Copy link
Author

The general rule for comments is that you either use sentences and so start with a capital and end with a period, or you just have free text. You should avoid starting with a capital but omitting the period; and vice-versa.

Sure thing, since I am in that area, I might as well try to clean it up a bit.

Hopefully I got it right. Can you please take a look?

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed again. For those comments that already started with a capital
letter, I probably would have chosen to add a period at the end, but that's
a personal preference thing...

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

src/hotspot/os/bsd/os_bsd.cpp Show resolved Hide resolved
@gerard-ziemski
Copy link
Author

Reviewed again. For those comments that already started with a capital letter, I probably would have chosen to add a period at the end, but that's a personal preference thing...

Yeah, it was a judgement call on my part, but the logic I used was: if the line fits on a single line, then make it an in-line comment (no capital first letter, no period), otherwise capitalize and use period at the end.

@gerard-ziemski
Copy link
Author

Thank you very much for the feedback!

@gerard-ziemski
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Jan 23, 2024

Going to push as commit 3d82363.
Since your change was applied there have been 192 commits pushed to the master branch:

  • 2a01c79: 8324513: Inline ContiguousSpace::object_iterate_from
  • fbaaac6: 8314164: java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java fails intermittently in timeout
  • 791b427: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed
  • bcaad51: 8318228: RISC-V: C2 ConvF2HF
  • 5acd37f: 8324207: Serial: Remove Space::set_saved_mark_word
  • f5e6d11: 8324210: Serial: Remove unused methods in Generation
  • bcb340d: 8324286: Fix backsliding on use of nullptr instead of NULL
  • 3696765: 8323964: runtime/Thread/ThreadCountLimit.java fails intermittently on AIX
  • 5a74c2a: 8323438: Enhance assertions for Windows sync API failures
  • 52523d3: 8324050: Issue store-store barrier after re-materializing objects during deoptimization
  • ... and 182 more: https://git.openjdk.org/jdk/compare/aba19334eaeb46d37169cddeef929b13e050a60e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 23, 2024
@openjdk openjdk bot closed this Jan 23, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 23, 2024
@openjdk
Copy link

openjdk bot commented Jan 23, 2024

@gerard-ziemski Pushed as commit 3d82363.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

None yet

6 participants