Single-pass ASCII lower/upper case conversion

[fereidani]

Current algorithm is cloning whole string/vector once, then rereads and rewrites it in-place once again which is sub-optimal and waste of CPU cycles and cache.
This one creates it in a single-pass while copying the data from the original vector.
In my benchmarks this one is about 1.2x-2x to 10x(KBs long strings) faster.

[rustbot]

r? @JohnTitor

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[joshtriplett]

@bors r+

[rust-bors]

📌 Commit 3d3bf13 has been approved by joshtriplett

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 10. This pull request will be tested once the tree is reopened.

Reason for tree closure: manually handling queue due to backlog

[JonathanBrouwer]

@rust-timer build 4c170e1

[JonathanBrouwer]

^ Expected a false positive, all PRs in the rollup have this same result

[rust-timer]

Finished benchmarking commit (4c170e1): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 1.9%]	11
Regressions ❌ (secondary)	1.5%	[0.3%, 2.6%]	18
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	0.5%	[-0.8%, 1.9%]	12

Max RSS (memory usage)

Results (primary 0.9%, secondary 20.2%)

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

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	26.4%	[23.1%, 29.8%]	12
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-4.7%	[-6.6%, -3.0%]	3
All ❌✅ (primary)	0.9%	[-0.6%, 2.4%]	2

Cycles

Results (primary 0.9%, secondary 8.9%)

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

	mean	range	count
Regressions ❌ (primary)	1.1%	[0.4%, 2.3%]	8
Regressions ❌ (secondary)	14.0%	[0.5%, 30.4%]	18
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-1.2%	[-4.3%, -0.4%]	9
All ❌✅ (primary)	0.9%	[-0.7%, 2.3%]	9

Binary size

Results (primary 0.6%, secondary 1.0%)

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

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.1%, 2.6%]	19
Regressions ❌ (secondary)	1.0%	[0.1%, 2.0%]	2
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-0.6%, 2.6%]	22

Bootstrap: 489.825s -> 489.295s (-0.11%)
Artifact size: 391.14 MiB -> 391.33 MiB (0.05%)

[fereidani]

Is this a false positive or caused by my PR?

[JonathanBrouwer]

This result is a little bit more extreme than on other PRs but given all the noise it's hard to trust the numbers...

Posted a revert to see the real result, which I expect to be neutral (in which case I'll close the revert): #160703

[panstromek]

Based on the result from the revert, this is not a false positive, but the impact is probably not from running the code itself, but from compiling it. You can see that in detailed results, there's a lot more queries related to generics, which is probably because this PR changes few plain function calls into iterator calls. It's also visible in depgrah size and metadata size increases.

[JonathanBrouwer]

Given that the compiletime is slower, but the function itself is faster (since the function was benchmarked by the author), I'd lean towards not reverting and accepting the regression.

I'm a bit surprised that this single function can cause such a significant regression tho

[panstromek]

Yea, it's somewhat surprising. I may be wrong, though. bitmaps also regressed and I doubt that it calls some ascii functions, its detailed results are mostly the same, so maybe that one gets a perf hit in some cases when this is called in the compiler (I don't really know how much is this function used in the compiler, though).

On the other hand html5ever is html parser, so I guess that one could call it a lot, that result at least intuitively makes sense.

I would also lean towards not reverting, but it'd be nice to investigate a bit what's going on. I might look at it today if kids allow me to 😄

[fereidani]

The ascii.rs for is_ascii does something similar but it switches its strategy with const_eval_select:

const_eval_select!(
    @capture { s: &[u8] } -> bool:
    if const {
        is_ascii_simple(s)
    } else {
        ...complex strategy
    }
)

I think we can do the same for this function too for further improvements.

Original code was using this with const-hack.

    pub const fn make_ascii_lowercase(&mut self) {
        // FIXME(const-hack): We would like to simply iterate using `for` loops but this isn't currently allowed in constant expressions.
        let mut i = 0;
        while i < self.len() {
            let byte = &mut self[i];
            byte.make_ascii_lowercase();
            i += 1;
        }
    }

I wonder if we do Vec::with_capacity(N) then get a mutable pointer and directly copy to target using the same const-hack and then unsafe set_len to N, regression goes away or not.

I can send a PR, just let me know.

[panstromek]

I'd avoid const_eval_select at all costs unless we really need it 😆 It's only needed when making allocating functions const, so we don't need it here.

Either way, it doesn't seem to me that the problem is in the function. If I just put the two versions side by side in godbolt, the new one clearly looks better. It's actually almost identical except that the new doesn't have a call to memcpy. If there really is a runtime performance problem, it would probably be some interaction with other optimizations when this is inlined to some caller.