Skip to content

Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. #77264

Merged
bors merged 3 commits intorust-lang:masterfrom
fusion-engineering-forks:skip-local-stdio
Oct 3, 2020
Merged

Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. #77264
bors merged 3 commits intorust-lang:masterfrom
fusion-engineering-forks:skip-local-stdio

Conversation

@m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Sep 27, 2020 •
edited
Loading

The thread local LOCAL_STDOUT and LOCAL_STDERR are only used by the test crate to capture output from tests when running them in the same process in differen threads. However, every program will check these variables on every print, even outside of testing.

This involves allocating a thread local key, and registering a thread local destructor. This can be somewhat expensive.

This change keeps a global flag (LOCAL_STREAMS) which will be set to true when either of these local streams is used. (So, effectively only in test and benchmark runs.) When this flag is off, these thread locals are not even looked at and therefore will not be initialized on the first output on every thread, which also means no thread local destructors will be registered.

Together with #77154, this should make output a little bit more efficient.

@rust-highfive
Copy link
Contributor

rust-highfive commented Sep 27, 2020

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 Sep 27, 2020
@m-ou-se
Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used.
07fd17f 
The thread local LOCAL_STDOUT and LOCAL_STDERR are only used by the test
crate to capture output from tests when running them in the same process
in differen threads. However, every program will check these variables
on every print, even outside of testing.

This involves allocating a thread local key, and registering a thread
local destructor. This can be somewhat expensive.

This change keeps a global flag (LOCAL_STREAMS) which will be set to
true when either of these local streams is used. (So, effectively only
in test and benchmark runs.) When this flag is off, these thread locals
are not even looked at and therefore will not be initialized on the
first output on every thread, which also means no thread local
destructors will be registered.
@m-ou-se
Relax memory ordering of LOCAL_STREAMS and document it.
ed3ead0
@m-ou-se
Optimize set_{panic,print}(None).
de597fc
@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 27, 2020

cc @fasterthanlime whose blogpost inspired this change

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 1, 2020

@rustbot modify labels: +T-libs +A-io

@rustbot rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 1, 2020
dtolnay
dtolnay approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Awesome!

@dtolnay
Copy link
Member

dtolnay commented Oct 1, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 1, 2020

📌 Commit de597fc has been approved by dtolnay

@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 Oct 1, 2020
@eddyb
Copy link
Member

eddyb commented Oct 1, 2020

You could use a thread-local Cell<bool>, that won't register a destructor, at least on cfg(target_thread_local) platforms.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 1, 2020

@eddyb Considering that these LOCAL_* streams are only used in tests/benchmarks, I think a global flag makes sense here. In all regular programs, this flag will be permanently off. In tests/benchmarks, the local streams are used on pretty much every thread. And a global flag is efficient on all platforms, not only those supporting #[thread_local].

@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Oct 2, 2020
@jonas-schievink jonas-schievink mentioned this pull request Oct 2, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
@bors
Auto merge of rust-lang#77470 - jonas-schievink:rollup-9a2hulp, r=jon…
6ebad43 
…as-schievink

Rollup of 8 pull requests

Successful merges:

 - rust-lang#75377 (Fix Debug implementations of some of the HashMap and BTreeMap iterator types)
 - rust-lang#76107 (Write manifest for MAJOR.MINOR channel to enable rustup convenience)
 - rust-lang#76745 (Move Wrapping<T> ui tests into library)
 - rust-lang#77182 (Add missing examples for Fd traits)
 - rust-lang#77251 (Bypass const_item_mutation if const's type has Drop impl)
 - rust-lang#77264 (Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. )
 - rust-lang#77421 (Revert "resolve: Avoid "self-confirming" import resolutions in one more case")
 - rust-lang#77452 (Permit ty::Bool in const generics for v0 mangling)

Failed merges:

r? `@ghost`
@bors bors merged commit 01ca829 into rust-lang:master Oct 3, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 3, 2020
@m-ou-se m-ou-se deleted the skip-local-stdio branch November 23, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels

A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

7 participants

@m-ou-se @rust-highfive @dtolnay @bors @eddyb @joshtriplett @rustbot