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

Speedup Build::clone: Use Arc instead of String/PathBuf/OsString #782

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Speedup Build::clone: Use Arc instead of String/PathBuf/OsString #782

merged 1 commit into from
Feb 2, 2023

Conversation

NobodyXu
Copy link
Collaborator

TBF, I'm not sure how much this will help real-world usage.

I submit this PR based on the observation that the APIs of Build accepts &str, &Path and &OsStr, so converting them to an Arc<T> should be as cheap as String/PathBuf/OsString given that build-overrides is compiled without optimization, so the compiler cannot simply omit these heap allocation (and even with optimization enabled -O3, it's hard to do so).

While Arc::drop is indeed due to atomic operations, I think this is mostly going to be ok given that the heap deallocation is likely to be more expensive than that since build-overrides uses platform-default allocator, which is often not the most optimized.

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

…ring`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@thomcc
Copy link
Member

thomcc commented Feb 2, 2023

I'm not sure this is worth the trouble, TBH... How often does it get cloned? I suppose it doesn't hurt much.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Feb 2, 2023

I'm not sure this is worth the trouble, TBH... How often does it get cloned? I suppose it doesn't hurt much.

I'm also not sure about this, but the change here shall have minimal performance impact on code that does not use Build::clone, that's why I submit the PR even I'm not sure.

But feel free to close it if you feel like there's too much to review and not much gain.

@thomcc
Copy link
Member

thomcc commented Feb 2, 2023

The review is easy but it causes churn on other PRs. That's the big downside. That said I suppose it's fine.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks harmless, as mentioned.

@thomcc thomcc merged commit d247957 into rust-lang:main Feb 2, 2023
@NobodyXu NobodyXu deleted the speedup-cloning-Build branch February 2, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants