Copy link

Contributor

rust-highfive commented 23 days ago

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

rust-timer commented 23 days ago

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors commented 23 days ago

Trying commit 9332ac3 with merge ee90cb2...

bors commented 23 days ago

Try build successful - checks-actions
Build commit: ee90cb2 (ee90cb22627d2f2c6828fc4316225d34b4b8bd1a)

rust-timer commented 23 days ago

rust-timer commented 23 days ago

Finished benchmarking try commit (ee90cb2): 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 label: +S-waiting-on-review -S-waiting-on-perf

yaahc commented 20 days ago •

So am I correct in understanding that the changes here don't represent direct optimizations, but instead stop these wrappers from ignoring clone_from impls of inner types, so if the inner type depends heavily on a custom clone_from for performance reasons these wrappers will not be in the way after this change? I was surprised that the perf test didn't show any real improvement on a change that's ostensibly all about improving perf but now I feel like I understand, assuming my assessment is correct.

If that's the case I'd be happy to accept this PR but I think it might be better to try to address this problem in the Clone derive itself, which would have much further reaching impact and would stop us from needing to do these small changes in the future.

a1phyr commented 19 days ago

So am I correct in understanding that the changes here don't represent direct optimizations, but instead stop these wrappers from ignoring clone_from impls of inner types, so if the inner type depends heavily on a custom clone_from for performance reasons these wrappers will not be in the way after this change?

Yes, a common example is an io::Cursor<Vec<u8>>. With this PR, usingclone_from avoids allocating a new buffer and dropping the old one.

I was surprised that the perf test didn't show any real improvement on a change that's ostensibly all about improving perf but now I feel like I understand, assuming my assessment is correct.

This optimization is a bit niche, to be fair. I don't think that rustc uses clone_from for these specific types a lot, so I didn't expect this PR to improve compile times. It could really improve some other programs though, and it has a very small cost.

If that's the case I'd be happy to accept this PR but I think it might be better to try to address this problem in the Clone derive itself, which would have much further reaching impact and would stop us from needing to do these small changes in the future.

It was proposed a long time ago (#27939), but it was closed due to the impact on compile times. With this PR, clone_from should be overridden for most of the remaining interesting types (except tuples, looks like I forgot them).

yaahc commented 17 days ago

It was proposed a long time ago (#27939), but it was closed due to the impact on compile times. With this PR, clone_from should be overridden for most of the remaining interesting types (except tuples, looks like I forgot them).

Aah, unfortunate. Thanks for checking the background.

@bors r+

bors commented 17 days ago

Commit 9332ac3 has been approved by yaahc

bors commented 17 days ago

Testing commit 9332ac3 with merge fb84a8750d1f6cd2bdb8a2b96d4fb67a6997616c...

rust-log-analyzer commented 17 days ago

The job aarch64-gnu failed! Check out the build log: (web) (plain)

bors commented 17 days ago

Test failed - checks-actions

a1phyr commented 16 days ago

Looks like a spurious network error.
@bors retry

bors commented 16 days ago

@a1phyr: Insufficient privileges: not in try users

Xanewok commented 16 days ago

warning: spurious network error (2 tries remaining): failed to connect to github.com: Connection refused; class=Os (2)
warning: spurious network error (1 tries remaining): failed to connect to github.com: Connection refused; class=Os (2)
warning: spurious network error (1 tries remaining): failed to connect to github.com: Connection refused; class=Os (2)

@bors retry

bors commented 16 days ago

Testing commit 9332ac3 with merge 3d31363338bc3a4db66237f5be10389cfd01201b...

bors commented 16 days ago

Test successful - checks-actions
Approved by: yaahc
Pushing 3d31363 to master...