Make performance description of String::{insert,insert_str,remove} more precise #138538

hkBst · 2025-03-15T15:17:04Z

rustbot · 2025-03-15T15:17:08Z

rustbot has assigned @joboet.
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

joboet

I don't really see why anyone would need such precise numbers, it's not like you could correlate that with an exact cycle number or something.

Still, the old note is not correct. I recommend you adapt the old note to say something about the tail of the string needing to be shifted – that is probably the most helpful information to anyone investigating a performance issue in their code.

joboet · 2025-03-17T08:38:21Z

library/alloc/src/string.rs

-    /// This is an *O*(*n*) operation as it requires copying every element in the
-    /// buffer.
+    /// If there is space in `self` this will copy `self.len() - idx + string.len()` bytes,
+    /// otherwise will reallocate and copy `self.len() + string.len()` bytes.


This is incorrect – the reallocation case potentially copies self.capacity() bytes and then additionally self.len() - idx + string.len() bytes.

Right, I was a bit fuzzy about how the reallocation worked. Thanks for clearing that up! :D

I've reformulated this. Let me know what you think of it.

the8472 · 2025-03-17T08:52:39Z

Being too specific also has the downside that it's essentially specifying the implementation, thus constraining future implementation changes. What is documented generally is a guarantee.

hkBst · 2025-04-04T11:49:45Z

Being too specific also has the downside that it's essentially specifying the implementation, thus constraining future implementation changes. What is documented generally is a guarantee.

This is true. Would you prefer only documenting linear complexity in the total number of bytes (both the receiver and the insertion)?

hkBst · 2025-05-06T15:15:26Z

@rustbot ready

joboet

r=me with the commas removed

joboet · 2025-05-15T10:32:55Z

library/alloc/src/string.rs

+    /// Reallocates if `self.capacity()` is insufficient,
+    /// (which may involve copying all `self.capacity()` bytes).
+    /// Makes space at the given position for the insertion,
+    /// by moving the bytes, from the given position until the end, to new positions.


Suggested change

/// by moving the bytes, from the given position until the end, to new positions.

/// by moving the bytes from the given position until the end to new positions.

perhaps we can keep the last comma and remove only the first?

I rephrased this, so perhaps you'd like to take another look.

@joboet: ^^^

tgross35

Couple of small requests then I'd be happy to merge this. Please also rewrap doc comments to 100 chars.

tgross35 · 2025-06-16T04:43:33Z

library/alloc/src/string.rs

-    /// buffer.
+    /// Copies all bytes after the removed char to new positions.
+    ///
+    /// NB Calling this in a loop can result in quadratic behavior.


Could you replace "NB" with "Note that"? We've had some people be confused with "NB" before, and it reads a bit unusual in prose ("N.B." may be better, but not much reason not to just spell it out)

tgross35 · 2025-06-16T04:44:33Z

library/alloc/src/string.rs

+    /// Makes space for the insertion,
+    /// by copying all bytes of `&self[idx..]` to new positions.


A comma isn't needed before "by" - it's not a comma splice per se, but it's a short sentence and not really someplace you would pause speaking it out loud.

hkBst · 2025-06-16T12:18:24Z

Couple of small requests then I'd be happy to merge this. Please also rewrap doc comments to 100 chars.

I made the few improvements you requested, and removed an unnecessary line break.

tgross35

One small nit I missed then r=me

tgross35 · 2025-06-16T21:08:00Z

library/alloc/src/string.rs

+    /// Reallocates if `self.capacity()` is insufficient,
+    /// which may involve copying all `self.capacity()` bytes.
+    /// Makes space for the insertion by copying all bytes of `&self[idx..]` to new positions.


Sorry, one other thing - could you rewrap docs to 100 chars?

Alright, I've manually rewrapped this, but I'm not a fan.

Do you have a tool that produces this unusual wrapping?

Ah, looking closer I think this looks like these might be semantic line breaks - we've ~loosely talked about this before in a handful of scattered places. The general impression I've gotten is that SLBs make reading diffs slightly easier but make the resulting source harder/unnatural to read; since our source is read all the time, we prefer to cater to that and just wrap at 100. Also it's somewhat infrequent that a single clause is updated, usually changes are more substantial.

Hopefully one day rustfmt will just take care of this without intervention...

All good points, though I'm not sure I agree about which way is more readable.

Hopefully one day rustfmt will just take care of this without intervention...

That would be good.

Thanks for reviewing!

rustbot · 2025-06-17T06:04:17Z

⚠️ Warning ⚠️

This PR is based on an upstream commit that is 27 days old.

It's recommended to update your branch according to the rustc-dev-guide.

tgross35 · 2025-06-17T06:16:15Z

Thank you!

@bors r+ rollup

bors · 2025-06-17T06:16:18Z

📌 Commit 0348a4a has been approved by tgross35

It is now in the queue for this repository.

Rollup of 13 pull requests Successful merges: - #138538 (Make performance description of String::{insert,insert_str,remove} more precise) - #141946 (std: refactor explanation of `NonNull`) - #142216 (Miscellaneous RefCell cleanups) - #142542 (Manually invalidate caches in SimplifyCfg.) - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism) - #142570 (Reject union default field values) - #142584 (Handle same-crate macro for borrowck semicolon suggestion) - #142585 (Update books) - #142586 (Fold unnecessary `visit_struct_field_def` in AstValidator) - #142587 (Make sure to propagate result from `visit_expr_fields`) - #142595 (Revert overeager warning for misuse of `--print native-static-libs`) - #142598 (Set elf e_flags on ppc64 targets according to abi) - #142601 (Add a comment to `FORMAT_VERSION`.) r? `@ghost` `@rustbot` modify labels: rollup  [Create a similar rollup](https://bors.rust-lang.org/queue/rust?prs=138538,141946,142216,142542,142563,142570,142584,142585,142586,142587,142595,142598,142601)  try-job: dist-apple-various

Make performance description of String::{insert,insert_str,remove} more precise

Rollup of 10 pull requests Successful merges: - #138538 (Make performance description of String::{insert,insert_str,remove} more precise) - #141946 (std: refactor explanation of `NonNull`) - #142216 (Miscellaneous RefCell cleanups) - #142371 (avoid `&mut P<T>` in `visit_expr` etc methods) - #142377 (Try unremapping compiler sources) - #142517 (Windows: Use anonymous pipes in Command) - #142542 (Manually invalidate caches in SimplifyCfg.) - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism) - #142570 (Reject union default field values) - #142584 (Handle same-crate macro for borrowck semicolon suggestion) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 10 pull requests Successful merges: - #138538 (Make performance description of String::{insert,insert_str,remove} more precise) - #141946 (std: refactor explanation of `NonNull`) - #142216 (Miscellaneous RefCell cleanups) - #142371 (avoid `&mut P<T>` in `visit_expr` etc methods) - #142377 (Try unremapping compiler sources) - #142517 (Windows: Use anonymous pipes in Command) - #142542 (Manually invalidate caches in SimplifyCfg.) - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism) - #142570 (Reject union default field values) - #142584 (Handle same-crate macro for borrowck semicolon suggestion) r? `@ghost` `@rustbot` modify labels: rollup  [Create a similar rollup](https://bors.rust-lang.org/queue/rust?prs=138538,141946,142216,142371,142377,142517,142542,142563,142570,142584)  try-job: dist-aarch64-apple

Rollup of 13 pull requests Successful merges: - #138538 (Make performance description of String::{insert,insert_str,remove} more precise) - #141946 (std: refactor explanation of `NonNull`) - #142216 (Miscellaneous RefCell cleanups) - #142542 (Manually invalidate caches in SimplifyCfg.) - #142563 (Refine run-make test ignores due to unpredictable `i686-pc-windows-gnu` unwind mechanism) - #142570 (Reject union default field values) - #142584 (Handle same-crate macro for borrowck semicolon suggestion) - #142585 (Update books) - #142586 (Fold unnecessary `visit_struct_field_def` in AstValidator) - #142587 (Make sure to propagate result from `visit_expr_fields`) - #142595 (Revert overeager warning for misuse of `--print native-static-libs`) - #142598 (Set elf e_flags on ppc64 targets according to abi) - #142601 (Add a comment to `FORMAT_VERSION`.) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #138538 - hkBst:patch-4, r=tgross35 Make performance description of String::{insert,insert_str,remove} more precise

rustbot assigned joboet Mar 15, 2025

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 Mar 15, 2025

hkBst changed the title ~~Make performance of String::insert_str more precise~~ Make performance description of String::insert_str more precise Mar 17, 2025

joboet requested changes Mar 17, 2025

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 Mar 17, 2025

hkBst force-pushed the patch-4 branch from 0e7f01c to aa594fe Compare April 4, 2025 11:41

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 May 6, 2025

joboet approved these changes May 15, 2025

View reviewed changes

rust-cloud-vms bot force-pushed the patch-4 branch from aa594fe to cea703d Compare May 21, 2025 08:52

This comment has been minimized.

Sign in to view

hkBst changed the title ~~Make performance description of String::insert_str more precise~~ Make performance description of String::{insert,insert_str,remove} more precise May 21, 2025

rust-cloud-vms bot force-pushed the patch-4 branch from cea703d to 1172074 Compare May 21, 2025 08:56

hkBst requested a review from joboet May 21, 2025 08:56

tgross35 reviewed Jun 16, 2025

View reviewed changes

rust-cloud-vms bot force-pushed the patch-4 branch from 1172074 to 9d98be8 Compare June 16, 2025 12:12

This comment has been minimized.

Sign in to view

tgross35 approved these changes Jun 16, 2025

View reviewed changes

Make performance of String::insert_str more precise

0348a4a

rust-cloud-vms bot force-pushed the patch-4 branch from 9d98be8 to 0348a4a Compare June 17, 2025 06:04

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 Jun 17, 2025

workingjubilee mentioned this pull request Jun 17, 2025

Rollup of 13 pull requests #142613

Merged

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 17, 2025

Rollup merge of rust-lang#138538 - hkBst:patch-4, r=tgross35

679a1a7

Make performance description of String::{insert,insert_str,remove} more precise

workingjubilee mentioned this pull request Jun 17, 2025

Rollup of 10 pull requests #142614

Open

bors merged commit 346adce into rust-lang:master Jun 17, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 17, 2025

rust-timer added a commit that referenced this pull request Jun 17, 2025

Unrolled build for #138538

878001e

Rollup merge of #138538 - hkBst:patch-4, r=tgross35 Make performance description of String::{insert,insert_str,remove} more precise

	/// by moving the bytes, from the given position until the end, to new positions.
	/// by moving the bytes from the given position until the end to new positions.

		/// Makes space for the insertion,
		/// by copying all bytes of `&self[idx..]` to new positions.

Make performance description of String::{insert,insert_str,remove} more precise #138538

Make performance description of String::{insert,insert_str,remove} more precise #138538

Conversation

hkBst commented Mar 15, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 15, 2025

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the8472 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkBst commented Apr 4, 2025

Uh oh!

hkBst commented May 6, 2025

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

hkBst commented Jun 16, 2025

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jun 17, 2025

Uh oh!

tgross35 commented Jun 17, 2025

Uh oh!

bors commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

hkBst commented Mar 15, 2025 •

edited by rustbot

Loading

the8472 commented Mar 17, 2025 •

edited

Loading