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

Restructure std::rt #89011

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Restructure std::rt #89011

merged 8 commits into from
Sep 29, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Sep 16, 2021

These changes should reduce binary size slightly while at the same slightly improving performance of startup, thread spawning and std::thread::current(). I haven't verified if the compiler is able to optimize some of these cases already, but at least for some others the compiler is unable to do these optimizations as they slightly change behavior in cases where program startup would crash anyway by omitting a backtrace and panic location.

I can remove 6f6bb16 if preferred.

@bjorn3
Merge sys_common::rt into rt
6f6bb16
@bjorn3
Remove an allocation from rt::init
af7eede 
Previously the thread name would first be heap allocated and then
re-allocated to add a nul terminator. Now it will be heap allocated only
once with nul terminator added form the start.
@bjorn3
Optimize ThreadInfo::with
f78cd44 
The RefCell is now borrowed exactly once. In addition a code sequence
that contains an unwrap that is guaranteed to never panic at runtime is
replaced with get_or_insert_with, which makes the intended behavior
clearer and will not emit code to panic even without optimizations.
@bjorn3
Use const {} for the THREAD_INFO thread local
a8bb3bc 
This makes accesses to it cheaper
@bjorn3
Remove unused function
1ad44b2
@bjorn3
Replace a couple of asserts with rtassert! in rt code
cb14269 
This replaces a couple of panic locations with hard aborts. The panics
can't be catched by the user anyway in these locations.
@bjorn3
Merge two THREAD_INFO.with and following RefCell borrow
5e7f641 
This is a bit faster
@bjorn3 bjorn3 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 16, 2021
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 16, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 16, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 16, 2021
@bors
Copy link
Contributor

bors commented Sep 16, 2021

⌛ Trying commit 5e7f641 with merge 15b3d2ed0e2de00152c24e976f78374d1c1beb14...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 16, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Sep 16, 2021

⌛ Trying commit 37608c7 with merge e84914f65292e8404d89a72f04f1bb753f39a77f...

@bors
Copy link
Contributor

bors commented Sep 16, 2021

☀️ Try build successful - checks-actions
Build commit: e84914f65292e8404d89a72f04f1bb753f39a77f (e84914f65292e8404d89a72f04f1bb753f39a77f)

@rust-timer
Copy link
Collaborator

Queued e84914f65292e8404d89a72f04f1bb753f39a77f with parent 2b5ddf3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e84914f65292e8404d89a72f04f1bb753f39a77f): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

  • Moderate improvement in instruction counts (up to -1.6% on incr-patched: b9b3e592dd cherry picked builds of style-servo)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 16, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Sep 17, 2021 •
edited
Loading

Binary sizes for fn main() {}:

config before this PR after this PR diff (bytes)
- 3_639_256 3_627_968 -11_288
- (debuginfo stripped) 360_976 361_024 +48
- (fully stripped) 297_216 297_288 +72
-Copt-level=3 3_638_112 3_626_824 -11_288
-Copt-level=3 (debuginfo stripped) 360_128 360_176 +48
-Copt-level=3 (fully stripped) 297_216 297_288 +72
-Copt-level=3 -Clto 1_581_104 1_568_992 -12_112
-Copt-level=3 -Clto (debuginfo stripped) 321_648 317_600 -4048
-Copt-level=3 -Clto (fully stripped) 276_736 272_712 -4024

When stripping binaries, but not doing LTO, this increases binary size by up to 72 bytes, but in all other cases this reduces binary size by up to 12kb without stripping or 4kb with stripping.

@klensy
Copy link
Contributor

klensy commented Sep 17, 2021

Diff with -Cprefer-dynamic should be bigger perhaps (in % of size change).

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 18, 2021

I think in that case only the size of libstd.so would change and not the actual executable.

@bjorn3 bjorn3 mentioned this pull request Sep 18, 2021
Merged
dtolnay
dtolnay reviewed Sep 29, 2021
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.

This PR is labelled T-libs-api but it's not immediately clear to me why. Is there an externally observable API tradeoff that we'll need to reach a consensus on? Otherwise this seems like T-libs's business.

(Haven't gotten a chance to review yet.)

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 29, 2021

I meant libs-impl, not libs-api. My bad.

@bjorn3 bjorn3 removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 29, 2021
@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 29, 2021
dtolnay
dtolnay approved these changes Sep 29, 2021
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.

Looks good. :) Thank you for nicely splitting the commits.

@dtolnay
Copy link
Member

dtolnay commented Sep 29, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2021

📌 Commit 37608c7 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 Sep 29, 2021
@bors
Copy link
Contributor

bors commented Sep 29, 2021

⌛ Testing commit 37608c7 with merge 1149193...

@bors
Copy link
Contributor

bors commented Sep 29, 2021

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 1149193 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2021
@bors bors merged commit 1149193 into rust-lang:master Sep 29, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 29, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1149193): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2021
@matthiaskrgr
Rollup merge of rust-lang#89068 - bjorn3:restructure_rt2, r=joshtriplett
6c5aa76 
Restructure std::rt (part 2)

A couple more cleanups on top of rust-lang#89011

Blocked on rust-lang#89011
@bjorn3 bjorn3 deleted the restructure_rt branch March 14, 2022 12:14
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
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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bjorn3 @rust-highfive @rust-timer @bors @rust-log-analyzer @klensy @dtolnay @rustbot