Skip to content

Alloc String::retain optimization#150067

Open
fereidani wants to merge 3 commits intorust-lang:mainfrom
fereidani:string_retain
Open

Alloc String::retain optimization#150067
fereidani wants to merge 3 commits intorust-lang:mainfrom
fereidani:string_retain

Conversation

@fereidani
Copy link
Copy Markdown
Contributor

@fereidani fereidani commented Dec 16, 2025

View all comments

Hi again!
This uses the exact same algorithm as my other PR: #149784

Technically it should improve performance for String::retain too, But let's see what bors thinks.

@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 Dec 16, 2025
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Dec 16, 2025

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

@ZuseZ4
Copy link
Copy Markdown
Member

ZuseZ4 commented Dec 16, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

rust-bors Bot added a commit that referenced this pull request Dec 16, 2025
Alloc `String::retain` optimization
@rust-bors

This comment has been minimized.

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

rust-bors Bot commented Dec 16, 2025

☀️ Try build successful (CI)
Build commit: 2621d01 (2621d0177c40d6c9165ad462e5826558b57ea69a, parent: 31010ca61c3ff019e1480dda0a7ef16bd2bd51c0)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (2621d01): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible 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)

Results (primary -2.1%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (primary 68.5%, secondary -2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

Results (primary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 479.038s -> 478.434s (-0.13%)
Artifact size: 390.32 MiB -> 390.32 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 16, 2025
@Mark-Simulacrum
Copy link
Copy Markdown
Member

r? joboet

Probably makes sense to have the same reviewer for both.

@rustbot rustbot assigned joboet and unassigned Mark-Simulacrum Dec 28, 2025
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Dec 28, 2025

joboet is not on the review rotation at the moment.
They may take a while to respond.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 3, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jan 4, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 4, 2026
Copy link
Copy Markdown
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

I think this could be even faster by avoiding some UTF-8 reencoding...

View changes since this review

Comment thread library/alloc/src/string.rs Outdated
Comment thread library/alloc/src/string.rs Outdated
Comment thread library/alloc/src/string.rs Outdated
@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Jan 5, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@fereidani fereidani requested a review from joboet January 7, 2026 11:46
@rustbot rustbot 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 Jan 7, 2026
@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented Jan 7, 2026

@bors try @rust-timer queue

@rust-timer
Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion.

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

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jan 7, 2026

⌛ Trying commit 590ba93 with merge 886f85e

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/20782421026

rust-bors Bot added a commit that referenced this pull request Jan 7, 2026
Alloc `String::retain` optimization
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 7, 2026
Comment on lines +1671 to +1673
let string_ptr = ptr::NonNull::from(&mut *self);
let data_ptr = self.vec.as_mut_ptr();
let mut chars = self.char_indices();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unsound – data_ptr and string_ptr are both invalidated by the call to char_indices, as that creates a mutable reference to self and the str.

// Critical section starts here, at least one character is going to be removed.
let mut g = PanicGuard { s: string_ptr, read, write };
// Slower write-path
while let Some((read, ch)) = chars.next() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You'll thus need to create the CharIndices iterator on every iteration.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 7, 2026
@joboet
Copy link
Copy Markdown
Member

joboet commented Jan 7, 2026

@bors try cancel
Since this will need a rerun once fixed anyway.

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented Jan 7, 2026

@SpriteOvO
Copy link
Copy Markdown
Member

@fereidani Hi, ping from triage team. This PR has been inactive for a while. There are still some review comments that need to be addressed. Would you like to proceed with this PR? Thanks.

@fereidani
Copy link
Copy Markdown
Contributor Author

@SpriteOvO @joboet Please excuse the delay. My country is currently at war, and our government has cut off internet access for our people for over two months now. Now that I have a somewhat workable connection, I’ll try to complete the PR within the next few days. Thank you for your understanding.

@SpriteOvO
Copy link
Copy Markdown
Member

@fereidani Oh, so sorry to hear that. No need to hurry at all here. Feel free to come back to this when you have time, staying safe first.

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

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-perf Status: Waiting on a perf run to be completed. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants