Contributor

rust-highfive commented on Oct 30

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

bugadani commented on Oct 30

cc @est31

varkor commented on Nov 16

Apologies for taking so long; this slipped under my radar. I'm not familiar with this code, so I'm assigning to @Mark-Simulacrum based on git blame.

r? @Mark-Simulacrum

bugadani commented on Nov 20

@Mark-Simulacrum I've done what I could. I've addressed your comments and reverted some changes based on those comments. For example, changes to write_from_iter are no longer part of this PR, as they aren't necessary any more.

Mark-Simulacrum commented on Nov 21

@bors try @rust-timer queue

This should just be a win (or roughly neutral), but let's check. r=me otherwise.

rust-timer commented on Nov 21

Awaiting bors try build completion

bors commented on Nov 21

Trying commit 43b83b3 with merge aa21b48...

bors commented on Nov 21

Try build successful - checks-actions
Build commit: aa21b48 (aa21b48c4d75183274998a446bae14e98053637a)

rust-timer commented on Nov 21

rust-timer commented 29 days ago

Finished benchmarking try commit (aa21b48): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

bugadani commented 29 days ago

Interesting. I wonder if I see the results correctly - an increase in instruction counts, but in the same time, looking at the detailed results, a decrease in times? For example, this report belongs to a 0.5% instruction count regression, but shows a -0.3% total time diff: https://perf.rust-lang.org/detailed-query.html?commit=aa21b48c4d75183274998a446bae14e98053637a&base_commit=c9c57fadc47c8ad986808fc0a47479f6d2043453&benchmark=match-stress-enum-check&run_name=incr-unchanged

I would say that this is perhaps a slight regression in instruction counts, but overall a win on cycles -- https://perf.rust-lang.org/compare.html?start=c9c57fadc47c8ad986808fc0a47479f6d2043453&end=aa21b48c4d75183274998a446bae14e98053637a&stat=cycles%3Au on non-incremental benchmarks. I am inclined to land it based on these performance results; it does not seem like an obvious win yet, but it does seem like something we should be doing to try and avoid potential problems for the cases optimized here.

Ok, r=me with commits squashed

bors commented 29 days ago

Commit e93a463 has been approved by Mark-Simulacrum

Note that the 50% there is a wall clock measurement of 6ms, which is almost certainly noise. Cycle measurements frequently have noise in the 5% range on clean benchmarks; for incremental unchanged benchmarks they can be even noisier since there's less overall cycles on those.

I am not particularly worried.

est31 commented 29 days ago

@Mark-Simulacrum thanks for the info!

bors commented 29 days ago

Testing commit e93a463 with merge 432d116...

bors commented 29 days ago

Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 432d116 to master...

rust-highfive commented 29 days ago

Toolstate changed by #78569!

Tested on commit 432d116.
Direct link to PR: #78569

rls on linux: test-pass → test-fail (cc @Xanewok).