perf(allocator): remove overflow checks from String::from_strs_array_in

[overlookmotel]

Optimize String::from_strs_array_in to remove overflow checks in common cases.

&str has a maximum length of isize::MAX, so in many cases, if we communicate this constraint to the compiler, it can remove the overflow checks, as it can see that overflowing usize is impossible.

I was unsure if max length of &str was guaranteed, but wiser minds confirm it is: https://users.rust-lang.org/t/does-str-reliably-have-length-isize-max/126777

[overlookmotel]

• perf(allocator): remove overflow checks from String::from_strs_array_in #9650 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Performance Report

Merging #9650 will not alter performance

_{Comparing 03-10-perf_allocator_remove_overflow_checks_from_string_from_strs_array_in_ (89b6e4c) with main (a10ead8)}

Summary

✅ 39 untouched benchmarks

[Dunqing]

It looks like we don't even need to add any checks?

[Dunqing]

Merge activity

• Mar 11, 2:03 AM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Mar 11, 2:03 AM EDT: A user added this pull request to the Graphite merge queue.
• Mar 11, 2:09 AM EDT: A user merged this pull request with the Graphite merge queue.

[overlookmotel]

It looks like we don't even need to add any checks?

Yes I think we still do. We have to deal correctly with any possible input to the function.

Without overflow checks, this example would cause incorrect total_len calculation, and therefore writing out of bounds.

let s1 = "$_";
let s2 = get_long_string();
let s3 = get_long_string();
assert_eq!(s1.len(), 2);
assert_eq!(s2.len(), isize::MAX as usize);
assert_eq!(s3.len(), isize::MAX as usize);

// This does not panic in release mode without `checked_add`
let total_len = [s1, s2, s3].iter().fold(0usize, |total_len, s| total_len + s.len());
// Oh no!
assert_eq!(total_len, 0);

But... at least this change means that in our actual use cases, the compiler has enough information to prove that the checks aren't needed, and to remove them.

So we get complete safety no matter what the input is, but we don't have to pay for it - as long as only 1 of the input strings is dynamic, and the rest are static (so compiler knows their length).

[Dunqing]

Yes I think we still do. We have to deal correctly with any possible input to the function.

Yes, I agree!

Without overflow checks, this example would cause incorrect total_len calculation, and therefore writing out of bounds.

let s1 = "$_";
let s2 = get_long_string();
let s3 = get_long_string();
assert_eq!(s1.len(), 2);
assert_eq!(s2.len(), isize::MAX as usize);
assert_eq!(s3.len(), isize::MAX as usize);

// This does not panic in release mode without `checked_add`
let total_len = [s1, s2, s3].iter().fold(0usize, |total_len, s| total_len + s.len());
// Oh no!
assert_eq!(total_len, 0);

But... at least this change means that in our actual use cases, the compiler has enough information to prove that the checks aren't needed, and to remove them.

So we get complete safety no matter what the input is, but we don't have to pay for it - as long as only 1 of the input strings is dynamic, and the rest are static (so compiler knows their length).

The example case seems like it would not happen, as https://users.rust-lang.org/t/does-str-reliably-have-length-isize-max/126777/2 explained, the maximum allocatable allocation object cannot exceed isize::MAX; therefore, its impossible for total allocated string length to surpass isize::MAX either. Am I correct in understanding this?

[overlookmotel]

The example case seems like it would not happen, as https://users.rust-lang.org/t/does-str-reliably-have-length-isize-max/126777/2 explained, the maximum allocatable allocation object cannot exceed isize::MAX; therefore, its impossible for total allocated string length to surpass isize::MAX either. Am I correct in understanding this?

Yes, it's true that attempting to allocate more than isize::MAX bytes will fail. Vec::with_capacity_in will panic in this case.

But the potential problem here is before we allocate the Vec.

If we calculate total_len without checks and the calculation "overflows", then total_len can end up "wrapping around" back to 0.

So, if total_len is incorrectly calculated as 0, then we call Vec::with_capacity_in(0, allocator). That doesn't panic, and it doesn't allocate any memory either.

But we are assuming that we have allocated enough memory for all the strings, and we get the pointer to the Vec, and write the first string at that memory address. But we didn't allocate any memory! So we just wrote that string over unallocated memory. This may result in a segfault, or it may overwrite any other data in memory. Bad news!

In safe code, Rust will prevent you from doing this - it checks these things and panics if you try to do anything illegal, and it's extremely good at covering every conceivable edge case. But in "unsafe land", we we have told Rust "don't check anything, we already checked". So the responsibility for covering every possible case is on us.

I'm not sure if this makes any more sense?

[Dunqing]

Yes, it's true that attempting to allocate more than isize::MAX bytes will fail. Vec::with_capacity_in will panic in this case.

But the potential problem here is before we allocate the Vec.

If we calculate total_len without checks and the calculation "overflows", then total_len can end up "wrapping around" back to 0.

So, if total_len is incorrectly calculated as 0, then we call Vec::with_capacity_in(0, allocator). That doesn't panic, and it doesn't allocate any memory either.

But we are assuming that we have allocated enough memory for all the strings, and we get the pointer to the Vec, and write the first string at that memory address. But we didn't allocate any memory! So we just wrote that string over unallocated memory. This may result in a segfault, or it may overwrite any other data in memory. Bad news!

In safe code, Rust will prevent you from doing this - it checks these things and panics if you try to do anything illegal, and it's extremely good at covering every conceivable edge case. But in "unsafe land", we we have told Rust "don't check anything, we already checked". So the responsibility for covering every possible case is on us.

I'm not sure if this makes any more sense?

Thank you for explaining this again, that makes sense, I misunderstood that the allocated object can't be more than one https://doc.rust-lang.org/std/ptr/index.html#allocated-object