Conversation
28e8c60 to
207e484
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Optimize `Extend for String`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (3f3acdf): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.2%, secondary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.701s -> 471.964s (0.06%) |
207e484 to
b6110e5
Compare
This comment has been minimized.
This comment has been minimized.
|
Interesting perf results. Let's see what happens when we specialize on array and Vec iterators... @Urgau: Could we do another perf run please? I hope this is ok for you @lolbinarycat since it takes from #148604, #148604 (comment) and #149694 |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Optimize `Extend for String`
This comment has been minimized.
This comment has been minimized.
|
💥 Test timed out after |
|
I would appreciate a |
Co-authored-by: binarycat <binarycat@envs.net>
b6110e5 to
9961b10
Compare
|
Could we run perf again? The previous attempt failed after the try build timed out. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Optimize `Extend for String`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ed1b0fb): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.0%, secondary -3.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.708s -> 474.648s (0.62%) |
| // SAFETY: | ||
| // - `val` is a valid &str, so `val.as_ptr()` is valid | ||
| // for `val.len()` bytes and properly initialized. | ||
| // - `spare` points to valid spare capacity in the Vec |
There was a problem hiding this comment.
This is unsound – it's possible to pass the same string multiple times (or overlapping strings), in which case this sum will saturate. But in that case, the capacity will not suffice here.
There was a problem hiding this comment.
reserve will abort the program with an OOM error in that case, no?
There was a problem hiding this comment.
Yes, esp since no single allocation can be isize::MAX bytes large, reserve is guaranteed to fail if the sum saturates (in this case it will panic when it tries to calculate the new layout of the allocation, not OOM).
| let chunk = match iter.next_chunk::<8>() { | ||
| Ok(chunk) => chunk.into_iter(), | ||
| Err(partial_chunk) => { | ||
| repeat = false; | ||
| partial_chunk | ||
| } | ||
| }; |
There was a problem hiding this comment.
Is this necessary for performance? Otherwise, I'd rather you use a loop and return instead of testing repeat.
|
Reminder, once the PR becomes ready for a review, use |
|
note: #149694 has been accepted. |
|
☔ The latest upstream changes (presumably #149694) made this pull request unmergeable. Please resolve the merge conflicts. |
Implements the idea in #148604 (comment).
Enhancement to #149694.
Could we get a perf run?