-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8359827: Test runtime/Thread/ThreadCountLimit.java need loop increasing the limit #26401
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
Conversation
👋 Welcome back syan! A progress list of the required criteria for merging this PR into |
@sendaoYan 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:
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 50 new commits pushed to the
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 |
@sendaoYan The following label will be automatically applied to this pull request:
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. |
Webrevs
|
@sendaoYan |
FWIW we see no issue running this test, but we ensure we already have a high ulimit setting available in our test machines by default. |
![]() |
…t/hotspot/jtreg/resourcehogs/runtime/Thread/
Thanks your correction @dholmes-ora. |
Okay, but in that scenario what is it you are actually running out of? You are changing the test to suit the way you need to run it, but I'm not aware of anyone else reporting issues running this test. |
That's not the way ulimit should work in different sub-shells. What is the ulimit in the parent shell? I think the subshells are limited by the parent. |
I initially thought that ulimit shouldn't work like that in different sub-shells. But actually ulimit works in different sub-shells as unexpectedly.
The ulimit in the parent shell is unlimited. The first process "./create-thread" can create 5k threads shows that the parent shell has no limit. ![]() |
There is definitely something unexpected/odd about the behaviour of First, what does it even mean to use Second, I observe that with The use of I'm really not sure how to proceed here. The change you propose affects all platforms, but there is only an issue for you on Linux. |
Hi @dholmes-ora
I think it's not machine overloading, becasuse the setting of 'ulimit -u' on my machine is 'unlimited'. I can create 5000 threads many times, show as below: ![]()
I think the 'ulimit -u' in sub-shell take effect in the sub-shell only, it's temporary setting, it will not affect the parent shell.
It seems that the sub-shell with 'ulimit -u 4096' prefix will count all the user processes number. It's just my speculatation. That's why this test not suitable run with other tests simultaneous Anyway, I change this PR to use |
You can't just change the test to use docker! This is not a container test. We use special test tasks to run container tests in an environment where containers are enabled. |
I'm finding some of these statements to be contradictory to the problem being stated. If the ulimit setting only affects the sub-shell then it can't cause other concurrent tests to hit the limit and fail to create threads!
If the sub-shell counts all processes/threads belonging to the user and applies the new ulimit then that would make some sense. But again how does that then cause any problem in a different shell? |
Okey, I have revert the docker commit. |
Maybe some of my previous statements have caused some misunderstandings. |
Sorry - right I get it now. So basically with enough other activity on the machine all the 4096 process/thread capacity may have already been used up before the test can run. It isn't this test that is the "resource hog" as such. What we really want to do is more like One possible solution would be to loop so that if we get exitcode 1 then we retry with 4096*2 etc.
Took 5 loops to run on my system (ulimit -u 24576) when I had an unrestricted version of the test running which has about 20700 live threads. |
Thanks @dholmes-ora very much. I will verified this patch by ours CI. /contributor add @dholmes-ora |
@sendaoYan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :)
I will run this through our CI as well.
Testing in our CI was fine - test passes on first iteration with 4096 in all case. I ran this standalone and within tier5. |
Testing in our CI was fine - test passes on third iteration with 12288 in all case. I run this within all the jtreg tests. |
/issue JDK-8359827 |
@sendaoYan This issue is referenced in the PR title - it will now be updated. |
Thanks your patient reviews and suggestions @dholmes-ora. /integrate |
Going to push as commit fc80384.
Your commit was automatically rebased without conflicts. |
@sendaoYan Pushed as commit fc80384. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
@sendaoYan any hotspot related changes, including tests, require two reviews before integration. It is probably prudent to wait for any feedback from other CI maintainers before backporting this change. |
Sorry, I will pay attention next time. |
Hi all,
The test runtime/Thread/ThreadCountLimit.java was observed fails when run with other tests. The test start subprocess with shell prefix command
ulimit -u 4096
which intend to limite the usage of thread number. But this will cause test fails when this test run with other tests. I create a demo to demonstrate that.I start a java process which will create 5k threads, and then I can not start new java process with prefix
ulimit -u 4096
on the same machine.ManyThreads.java.txt
So it's necessary to make this test run sperately to make this test success.
Change has been verified locally, test-fix only, risk is low.
Progress
Issue
Reviewers
Contributors
<[email protected]>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26401/head:pull/26401
$ git checkout pull/26401
Update a local copy of the PR:
$ git checkout pull/26401
$ git pull https://git.openjdk.org/jdk.git pull/26401/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26401
View PR using the GUI difftool:
$ git pr show -t 26401
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26401.diff
Using Webrev
Link to Webrev Comment