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.

Sign up for GitHub

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

Jump to bottom

Use fcntl(fd, F_GETFD) to detect if standard streams are open #96837

Merged
merged 3 commits into from
Jun 10, 2022
Merged

Use fcntl(fd, F_GETFD) to detect if standard streams are open #96837

merged 3 commits into from
Jun 10, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented May 8, 2022 •
edited
Loading

In the previous implementation, if the standard streams were open,
but the RLIMIT_NOFILE value was below three, the poll would fail
with EINVAL:

ERRORS: EINVAL The nfds value exceeds the RLIMIT_NOFILE value.

Switch to the existing fcntl based implementation to avoid the issue.

Fixes #96621.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 8, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2022
@joshtriplett
Copy link
Member

LGTM.

@bors r+

@bors
Copy link
Contributor

bors commented May 8, 2022

📌 Commit ba84ead159472666303de68104dce86ddc9b815c has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2022
@the8472
Copy link
Member

the8472 commented May 8, 2022

This costs 2 more syscalls at program startup, which adds up to a few microseconds I guess? Not the end of the world but still slower than the previous implementation.

@joshtriplett
Copy link
Member

joshtriplett commented May 10, 2022 •
edited
Loading

@the8472 I would be happy to merge a version that tries poll and falls back to fcntl iff poll returns EINVAL.

In this context, I felt like it made sense to merge the new test and the code to make that test pass. A subsequent PR can add the fast-path and ensure the test still passes.

@bors
Copy link
Contributor

bors commented May 11, 2022

⌛ Testing commit ba84ead159472666303de68104dce86ddc9b815c with merge 23bc2980951e5f1056dbb81d60e8478833cd1e48...

@bors
Copy link
Contributor

bors commented May 11, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 11, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tmiasko
Use fcntl(fd, F_GETFD) to detect if standard streams are open
e0a53ed 
In the previous implementation, if the standard streams were open,
but the RLIMIT_NOFILE value was below three, the poll would fail
with EINVAL:

> ERRORS: EINVAL The nfds value exceeds the RLIMIT_NOFILE value.

Switch to the existing fcntl based implementation to avoid the issue.
@tmiasko
Copy link
Contributor Author

tmiasko commented May 11, 2022

  • Disabled checking of a compiler stderr in the test.

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 11, 2022

📌 Commit e0a53ed has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2022
@joshtriplett
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 11, 2022
@the8472
Copy link
Member

the8472 commented May 11, 2022

This was discussed this in today's libs meeting. To keep the amount of unavoidable syscalls at program startup small we'd prefer to keep in the poll as fast path and only fall back to fcntl when it returns the error indicating that the rlimit was hit.

Since this PR mostly removes code it wouldn't make much sense to add it back with different control flow in a separate PR.

@the8472 the8472 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2022
@the8472
Copy link
Member

the8472 commented Jun 4, 2022

I have done the changes and pushed them to tmiasko's branch.

@the8472
keep using poll as fast path and only use fcntl as fallback
d3465a8 
this minimizes the amount of syscalls performed during startup
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2022

📌 Commit 4ae6f9fd98f4db113241711b11278cd27746036d has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2022
@the8472
Copy link
Member

the8472 commented Jun 9, 2022

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 9, 2022
@the8472
Copy link
Member

the8472 commented Jun 9, 2022

Converted the ifs to a match, I assume that's trivial enough that it won't need another review.

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Jun 9, 2022

📌 Commit 2e62fda has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2022
@bors
Copy link
Contributor

bors commented Jun 10, 2022

⌛ Testing commit 2e62fda with merge ec55c61...

@bors
Copy link
Contributor

bors commented Jun 10, 2022

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing ec55c61 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 10, 2022
@bors bors merged commit ec55c61 into rust-lang:master Jun 10, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 10, 2022
@bors bors mentioned this pull request Jun 10, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ec55c61): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 2.6% 2.6% 1
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.6% 2.6% 1

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -2.8% -2.8% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) -2.8% -2.8% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@tmiasko tmiasko deleted the stdio-fcntl branch June 13, 2022 11:56
@Logarithmus
Copy link
Contributor

@tmiasko the test added by your PR fails on my machine #99382
Probably because glibc can't be linked statically?

@spastorino
Copy link
Member

The test added in this PR now forces to install glibc.static in nixos. See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/process.2Fnofile-limit.2Ers.20test.20failing.20on.20nixos

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

@klensy klensy klensy left review comments

@the8472 the8472 the8472 left review comments

@joshtriplett joshtriplett Awaiting requested review from joshtriplett

Assignees

@joshtriplett joshtriplett

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Standard file descriptor checks cause abort on startup with strict rlimit