Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override clone_from method for PathBuf and OsString #84615

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

a1phyr
Copy link
Contributor

@a1phyr a1phyr commented Apr 27, 2021

This was not the case before because #[derive(Clone)] do not do it.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 27, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 27, 2021

What is the goal of this change? Does it improve performance at all? If not I don't see much point to it.

@a1phyr
Copy link
Contributor Author

a1phyr commented Apr 28, 2021

The default implementation of clone_from is

fn clone_from(&mut self, source: &Self) {
    *self = source.clone();
}

In case of an OsString and a PathBuf, it drops the old buffer and allocates a new one, which defeats the purpose of this method. Overriding it allows to reuse the old buffer. Currently this method is already overridden for Vec and String.

Example comparison between String and PathBuf: playground.

@jyn514
Copy link
Member

jyn514 commented Apr 28, 2021

Overriding it allows to reuse the old buffer.

I don't understand why that's the case? You're still calling clone on the buffer.

@a1phyr
Copy link
Contributor Author

a1phyr commented Apr 28, 2021

You're still calling clone on the buffer.

No, the new implementation calls self.inner.clone_from(&source.inner), which ultimately calls Vec::clone_from, which reuses the buffer. This is what String does today (and it works, cf playground link above).

@jyn514
Copy link
Member

jyn514 commented Apr 28, 2021

Ok, I found why I was confused: this reuses the allocation of the destination, not the source: https://doc.rust-lang.org/nightly/src/alloc/vec/mod.rs.html#2337-2366

LGTM but I'm not on T-libs and I'd prefer someone else to take a look.

@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 28, 2021
@Mark-Simulacrum
Copy link
Member

Seems likely pretty harmless but we also likely don't want to do this for everything as there is some compile-time overhead to their existence I suspect.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 28, 2021

📌 Commit 4a8671a has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2021
@a1phyr
Copy link
Contributor Author

a1phyr commented Apr 28, 2021

Seems likely pretty harmless but we also likely don't want to do this for everything as there is some compile-time overhead to their existence I suspect.

Yes, for derives #27939 was closed because of compile time. For individual items in std, I don't think there is a noticeable change, especially for downstream users.

@bors
Copy link
Contributor

bors commented Apr 28, 2021

⌛ Testing commit 4a8671a with merge 50ca3ac...

@bors
Copy link
Contributor

bors commented Apr 29, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 50ca3ac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 29, 2021
@bors bors merged commit 50ca3ac into rust-lang:master Apr 29, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 29, 2021
@a1phyr a1phyr deleted the clone_from_pathbuf_osstring branch March 12, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants