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

Reduce monomorphisation bloat in small_c_string #121101

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Reduce monomorphisation bloat in small_c_string #121101

merged 3 commits into from
Feb 19, 2024

Conversation

GnomedDev
Copy link
Contributor

This is a code path usually next to an FFI call, so taking the dyn slowdown for the 1159 llvm-line (fat lto, codegen-units 1, release build) drop in my testing program t2fanrd is worth it imo.

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 14, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2024

The Miri subtree was changed

cc @rust-lang/miri

@saethlin
Copy link
Member

x test miri --bless is supposed to work I think? Did it not? If it didn't work, we should have an issue for that.

I'm curious if this drops the size of build artifacts. The design sounds like a good tradeoff even if the size decrease is not very significant.
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 14, 2024
@bors
Auto merge of rust-lang#121101 - GnomedDev:dyn-small-c-string, r=<try>
5447954 
Reduce monomorphisation bloat in small_c_string

This is a code path usually next to an FFI call, so taking the `dyn` slowdown for the 1159 llvm-line (fat lto, codegen-units 1, release build) drop in my testing program [t2fanrd](https://github.com/GnomedDev/t2fanrd) is worth it imo.
@bors
Copy link
Contributor

bors commented Feb 14, 2024

⌛ Trying commit b862af2 with merge 5447954...

@RalfJung
Copy link
Member

x test miri --bless is supposed to work I think?

Yeah, it should. In fact ./x.py test miri --stage 0 --bless should work, which saves a bunch of time by avoiding a rebuild.

@saethlin
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 15, 2024

⌛ Trying commit 8f5dc71 with merge 5008444...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2024
@bors
Auto merge of rust-lang#121101 - GnomedDev:dyn-small-c-string, r=<try>
5008444 
Reduce monomorphisation bloat in small_c_string

This is a code path usually next to an FFI call, so taking the `dyn` slowdown for the 1159 llvm-line (fat lto, codegen-units 1, release build) drop in my testing program [t2fanrd](https://github.com/GnomedDev/t2fanrd) is worth it imo.
@bors
Copy link
Contributor

bors commented Feb 15, 2024

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5008444): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

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

Max RSS (memory usage)

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

Cycles

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

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.0% [0.0%, 0.0%] 4
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 34
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [0.0%, 0.0%] 4

Bootstrap: 636.575s -> 635.882s (-0.11%)
Artifact size: 306.16 MiB -> 306.11 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 15, 2024
@rustbot rustbot added the O-unix Operating system: Unix-like label Feb 17, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GnomedDev
Copy link
Contributor Author

@saethlin I tried ./x test miri --bless but it seems to have produced the diff just commited, which seems wrong for this PR... any other idea or should I open an issue. My config.toml is library profile.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GnomedDev
Copy link
Contributor Author

Okay, turns out --stage 0 is not an optional optimisation, it's required for bless to work.

@bors
Copy link
Contributor

bors commented Feb 18, 2024

📌 Commit 217e9c9 has been approved by Nilstrieb

It is now in the queue for this repository.

@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 Feb 18, 2024
@klensy
Copy link
Contributor

klensy commented Feb 18, 2024

What exact size diff with this PR for your examples?

@GnomedDev
Copy link
Contributor Author

@klensy With the settings in the PR description, testing the master branch at the time of branch off (eeeb021) vs this PR.
cstr-before.txt
cstr.txt

@bors
Copy link
Contributor

bors commented Feb 18, 2024

⌛ Testing commit 217e9c9 with merge b306f2d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
@bors
Auto merge of rust-lang#121101 - GnomedDev:dyn-small-c-string, r=Nils…
b306f2d 
…trieb

Reduce monomorphisation bloat in small_c_string

This is a code path usually next to an FFI call, so taking the `dyn` slowdown for the 1159 llvm-line (fat lto, codegen-units 1, release build) drop in my testing program [t2fanrd](https://github.com/GnomedDev/t2fanrd) is worth it imo.
@klensy
Copy link
Contributor

klensy commented Feb 18, 2024

@klensy With the settings in the PR description, testing the master branch at the time of branch off (eeeb021) vs this PR. cstr-before.txt cstr.txt

I mean. resulting file size diff, not llvm-lines.

@GnomedDev
Copy link
Contributor Author

Without stripping, binary size for t2fanrd goes from 626,664 bytes to 625,688 bytes with this PR.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 18, 2024

💔 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 Feb 18, 2024
@rustbot rustbot added the O-solid Operating System: SOLID label Feb 18, 2024
@GnomedDev
Copy link
Contributor Author

@Nilstrieb rebased and did more find/replace for the last couple invocations, can I get another approval?

@Noratrieb
Copy link
Member

random platforms failing, as expected^^

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2024

📌 Commit dbb15fb has been approved by Nilstrieb

It is now in the queue for this repository.

@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 Feb 18, 2024
@bors
Copy link
Contributor

bors commented Feb 18, 2024

⌛ Testing commit dbb15fb with merge 6122397...

@bors
Copy link
Contributor

bors commented Feb 19, 2024

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 6122397 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2024
@bors bors merged commit 6122397 into rust-lang:master Feb 19, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 19, 2024
@GnomedDev GnomedDev deleted the dyn-small-c-string branch February 19, 2024 00:52
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6122397): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

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

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.1%] 6
Regressions ❌
(secondary)		 0.0% [0.0%, 0.0%] 34
Improvements ✅
(primary)		 -0.5% [-0.5%, -0.5%] 4
Improvements ✅
(secondary)		 -0.5% [-0.5%, -0.4%] 29
All ❌✅ (primary) -0.1% [-0.5%, 0.1%] 10

Bootstrap: 641.709s -> 640.336s (-0.21%)
Artifact size: 308.85 MiB -> 308.82 MiB (-0.01%)

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

@RalfJung RalfJung RalfJung left review comments

@Noratrieb Noratrieb Noratrieb left review comments

Assignees

@Noratrieb Noratrieb

Labels
merged-by-bors This PR was explicitly merged by bors. O-hermit Operating System: Hermit O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface 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.78.0
Development

Successfully merging this pull request may close these issues.
10 participants
@GnomedDev @rustbot @rust-log-analyzer @saethlin @rust-timer @bors @RalfJung @Noratrieb @klensy @Mark-Simulacrum