speed up `String::push` and `String::insert` #124810

lincot · 2024-05-06T16:39:28Z

Addresses the concerns described in #116235.

The performance gain comes mainly from avoiding temporary buffers.

Complex pattern matching in encode_utf8 (introduced in #67569) has been simplified to a comparison and an exhaustive match in the encode_utf8_raw_unchecked helper function. It takes a slice of MaybeUninit<u8> because otherwise we'd have to construct a normal slice to uninitialized data, which is not desirable, I guess.

Several functions still have that unneeded zeroing, but a single instruction is not that important, I guess.

@rustbot label T-libs C-optimization A-str

rustbot · 2024-05-06T16:39:35Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

library/core/src/char/methods.rs

library/alloc/src/string.rs

library/core/src/char/methods.rs

scottmcm

I had a variety of thoughts; let me know what you think.

Also, is there anything here for which it would make sense to have a codegen test to confirm what's happening? Or some other test to help confirm it's better?

lincot · 2024-05-13T20:52:24Z

A codegen check for the absence of memcpy would be nice, since the original String::push has one.

bors · 2024-06-12T19:58:18Z

@lincot: 🔑 Insufficient privileges: not in try users

bors · 2024-06-22T19:02:36Z

☔ The latest upstream changes (presumably #116113) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-07-10T18:13:27Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

9511918

bors · 2024-07-17T06:13:44Z

☔ The latest upstream changes (presumably #127840) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-09-19T07:21:04Z

☔ The latest upstream changes (presumably #130511) made this pull request unmergeable. Please resolve the merge conflicts.

lincot · 2024-10-06T17:06:50Z

The proposed implementation uses get_unchecked_mut, which cannot be used in char::encode_utf8, which is now const. Also, get_unchecked_mut actually has a cost when running with debug assertions. So I am reverting to using pointers. The codegen of String::push seems to be unchanged: godbolt.

tgross35

It looks like your perf job never got run, could you rebase?

library/alloc/src/string.rs

library/core/src/char/methods.rs

tgross35 · 2024-12-19T23:08:48Z

library/core/src/char/methods.rs

+#[doc(hidden)]
+#[inline]
+#[cfg_attr(bootstrap, rustc_allow_const_fn_unstable(const_mut_refs))]
+pub const unsafe fn encode_utf8_raw_unchecked(code: u32, dst: *mut u8) {


dst could probably still be a &mut [u8] in this signature, call .as_mut_ptr() within this function. Then you can add a debug_assert!(dst.len() >= len).

I believe &mut [u8] cannot point to uninitialized memory according to safety guidelines, which is why I used &mut [MaybeUninit<u8>] first. If a slice is necessary, we can revert to using &mut [MaybeUninit<u8>] and .as_mut_ptr().

tests/codegen/string-push.rs

tgross35 · 2024-12-19T23:13:03Z

@rustbot author

lincot · 2024-12-20T21:07:23Z

@rustbot review

bors · 2025-01-24T05:00:39Z

☔ The latest upstream changes (presumably #135947) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2025-02-02T22:13:31Z

☔ The latest upstream changes (presumably #136448) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2025-02-20T02:53:11Z

☔ The latest upstream changes (presumably #137290) made this pull request unmergeable. Please resolve the merge conflicts.

tgross35 · 2025-02-20T22:20:32Z

@scottmcm are you able to review this? I could probably take a look if not.

tgross35 · 2025-02-20T22:20:55Z

I'll at least get this going

@bors try @rust-timer queue

…ng-insert, r=<try> speed up `String::push` and `String::insert` Addresses the concerns described in rust-lang#116235. The performance gain comes mainly from avoiding temporary buffers. Complex pattern matching in `encode_utf8` (introduced in rust-lang#67569) has been simplified to a comparison and an exhaustive `match` in the `encode_utf8_raw_unchecked` helper function. It takes a slice of `MaybeUninit<u8>` because otherwise we'd have to construct a normal slice to uninitialized data, which is not desirable, I guess. Several functions still have that [unneeded zeroing](https://rust.godbolt.org/z/5oKfMPo7j), but a single instruction is not that important, I guess. `@rustbot` label T-libs C-optimization A-str

bors · 2025-02-20T22:22:07Z

⌛ Trying commit 9cf92e5 with merge 26f0bba...

bors · 2025-02-21T00:21:45Z

☀️ Try build successful - checks-actions
Build commit: 26f0bba (26f0bbac902496404cadab55a28b41a2cafaa9c4)

rust-timer · 2025-02-21T05:20:53Z

Finished benchmarking commit (26f0bba): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.2%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.3%]	3
Improvements ✅ (secondary)	-0.4%	[-0.8%, -0.3%]	11
All ❌✅ (primary)	-0.2%	[-0.6%, 0.2%]	6

Max RSS (memory usage)

Results (primary 3.2%)

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)	7.5%	[1.2%, 14.0%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.3%	[-6.4%, -4.2%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.2%	[-6.4%, 14.0%]	6

Cycles

Results (primary 2.4%, secondary -2.2%)

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)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Binary size

Results (primary -0.1%, secondary -0.1%)

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.7%]	17
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.2%, -0.0%]	29
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	37
All ❌✅ (primary)	-0.1%	[-1.2%, 0.7%]	46

Bootstrap: 774.477s -> 773.266s (-0.16%)
Artifact size: 361.04 MiB -> 361.02 MiB (-0.00%)

rustbot assigned scottmcm May 6, 2024

scottmcm reviewed May 13, 2024

View reviewed changes

library/core/src/char/methods.rs Outdated Show resolved Hide resolved

scottmcm reviewed May 13, 2024

View reviewed changes

library/alloc/src/string.rs Show resolved Hide resolved

scottmcm reviewed May 13, 2024

View reviewed changes

library/core/src/char/methods.rs Outdated Show resolved Hide resolved

scottmcm requested changes May 13, 2024

View reviewed changes

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 May 13, 2024

cuviper mentioned this pull request May 14, 2024

Remove the branches from len_utf8 #125129

Closed

This comment has been minimized.

Sign in to view

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Jul 10, 2024

lincot force-pushed the speed-up-string-push-and-string-insert branch from 9511918 to 89fa55e Compare July 10, 2024 19:08

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Jul 10, 2024

lincot force-pushed the speed-up-string-push-and-string-insert branch from 89fa55e to 2cb20b3 Compare August 6, 2024 19:00

tgross35 reviewed Dec 19, 2024

View reviewed changes

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 Dec 19, 2024

lincot force-pushed the speed-up-string-push-and-string-insert branch from 9462d92 to a2effde Compare December 20, 2024 15:16

This comment has been minimized.

Sign in to view

lincot requested a review from tgross35 December 20, 2024 17:31

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 Dec 20, 2024

lincot force-pushed the speed-up-string-push-and-string-insert branch from 49bd467 to b7f3a92 Compare February 2, 2025 17:31

lincot force-pushed the speed-up-string-push-and-string-insert branch from b7f3a92 to 1297cd0 Compare February 18, 2025 19:33

lincot added 2 commits February 20, 2025 19:54

speed up String::push and String::insert

42a56df

add a codegen check for absence of memcpy in String::push

9cf92e5

lincot force-pushed the speed-up-string-push-and-string-insert branch from 1297cd0 to 9cf92e5 Compare February 20, 2025 16:54

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 20, 2025

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

speed up `String::push` and `String::insert` #124810

speed up `String::push` and `String::insert` #124810

lincot commented May 6, 2024

rustbot commented May 6, 2024

scottmcm left a comment

lincot commented May 13, 2024

This comment has been minimized.

bors commented Jun 12, 2024

This comment has been minimized.

bors commented Jun 22, 2024

rustbot commented Jul 10, 2024

bors commented Jul 17, 2024

bors commented Sep 19, 2024

lincot commented Oct 6, 2024

tgross35 left a comment

tgross35 Dec 19, 2024

lincot Dec 20, 2024

tgross35 commented Dec 19, 2024

This comment has been minimized.

This comment has been minimized.

lincot commented Dec 20, 2024

bors commented Jan 24, 2025

bors commented Feb 2, 2025

bors commented Feb 20, 2025

tgross35 commented Feb 20, 2025

tgross35 commented Feb 20, 2025

This comment has been minimized.

bors commented Feb 20, 2025

bors commented Feb 21, 2025

This comment has been minimized.

rust-timer commented Feb 21, 2025

speed up String::push and String::insert #124810

Are you sure you want to change the base?

speed up String::push and String::insert #124810

Conversation

lincot commented May 6, 2024

rustbot commented May 6, 2024

scottmcm left a comment

Choose a reason for hiding this comment

lincot commented May 13, 2024

This comment has been minimized.

bors commented Jun 12, 2024

This comment has been minimized.

bors commented Jun 22, 2024

rustbot commented Jul 10, 2024

bors commented Jul 17, 2024

bors commented Sep 19, 2024

lincot commented Oct 6, 2024

tgross35 left a comment

Choose a reason for hiding this comment

tgross35 Dec 19, 2024

Choose a reason for hiding this comment

lincot Dec 20, 2024

Choose a reason for hiding this comment

tgross35 commented Dec 19, 2024

This comment has been minimized.

This comment has been minimized.

lincot commented Dec 20, 2024

bors commented Jan 24, 2025

bors commented Feb 2, 2025

bors commented Feb 20, 2025

tgross35 commented Feb 20, 2025

tgross35 commented Feb 20, 2025

This comment has been minimized.

bors commented Feb 20, 2025

bors commented Feb 21, 2025

This comment has been minimized.

rust-timer commented Feb 21, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

speed up `String::push` and `String::insert` #124810

speed up `String::push` and `String::insert` #124810