Alloc String::retain optimization

[fereidani]

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]

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]

@bors try @rust-timer queue

[rust-bors]

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

[rust-timer]

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%)

[Mark-Simulacrum]

r? joboet

Probably makes sense to have the same reviewer for both.

[rustbot]

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

[rustbot]

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.

[joboet]

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

View changes since this review

[SpriteOvO]

@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]

@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]

@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.

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

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

[fereidani]

Please excuse my delay. Let me know when to clean up the tree and make it a single commit.

[joboet]

I'm very glad to hear from you again! This looks nice and sound, I only have one minor suggestion.

View changes since this review

[fereidani]

Thank you my friend, I'm happy to be back, It's a privilege for me to have you as my reviewer.
I checked the assembly for both, for retain(|c| c != ASCII) current version is generating smaller x64 assembly and optimizes better as ch.len_utf8() provides more information about the length for LLVM which it can optimize.
but while reviewing the assembly/IR I found out that ptr::copy turns into a slow call qword ptr [rip + memmove@GOTPCREL] call which is not optimal.
I would suggest that we switch it to:

                // SAFETY: `read..read + ch_len` is in bounds and `g.write < read`.
                //
                // The match makes every copy length constant so LLVM expands
                // it inline; a dynamic-length `ptr::copy` lowers to a
                // `memmove` call per retained character.
                unsafe {
                    let src = ptr.add(read);
                    let dst = ptr.add(g.write);
                    match ch_len {
                        1 => ptr::copy(src, dst, 1),
                        2 => ptr::copy(src, dst, 2),
                        3 => ptr::copy(src, dst, 3),
                        _ => ptr::copy(src, dst, 4),
                    }
                }

It optimizes better and it is about 3.5x faster when match is optimizable.
I also checked core::char::encode_utf8_raw_unchecked and self.encode_utf8; they both generate sub-optimal assembly and recalculate character length, self.encode_utf8 also generates unnecessary panic unwind handler and can't prove code could not panic. there might be a optimization chance for other functions using same ptr::copy match optimization too but I haven't benchmarked it yet for the others.
I also tried to do a cleaner optimization using assert_unchecked(ch_len>=1 && ch_len<=4) and then single ptr::copy, but LLVM can't optimize that like the match.
But whatever you think suits the project better is fine with me. Please let me know what I should do.

[joboet]

Two nits, but apart from those this looks fine. I think we can defer the char length question to a later time. You might actually want to file an issue about the memcpy call, I think LLVM could do better here.

View changes since this review

[fereidani]

Thanks! I fixed the problems and squashed the commits into one commit.
I will try to find the time to report it to LLVM.

[joboet]

Thank you, looks great!
@bors r+

[rust-bors]

📌 Commit 6a24a6e has been approved by joboet

It is now in the queue for this repository.