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

Reduce size of cc::Build and size of generated targets #1257

Merged
merged 7 commits into from
Nov 2, 2024
Merged

Conversation

NobodyXu
Copy link
Collaborator

@NobodyXu NobodyXu commented Nov 2, 2024

No description provided.

This would also reduce heap fragmentation.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Also add caching of parsing of the `TargetInfo` from cargo env.

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

/// Information specific to a `rustc` target.
///
/// See <https://doc.rust-lang.org/cargo/appendix/glossary.html#target>.
#[derive(Debug, PartialEq, Clone)]
pub(crate) struct TargetInfo {
pub(crate) struct TargetInfo<'a> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @madsmtm This is the optimization I was proposing, can you take a look and confirm this looks good please?

Copy link
Contributor

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Don't have a strong preference for either version (in favour is that matching on &str is nicer, against is that we have to maintain the list of struct fields).

};

#[derive(Debug)]
struct TargetInfoParserInner {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
struct TargetInfoParserInner {
struct ParsedTargetInfo {

(Or OwnedTargetInfo)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmm I think Inner suffix is ok, if we want to rename it we probably want to rename the other one as well.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Nov 2, 2024

against is that we have to maintain the list of struct fields

True we need that extra field maintenance, but that would make the generated binary smaller.

AFAIK std::borrow::Cow<'static, str> is 4 pointers long, and &str is only 2 pointers long.

Given that we use Cow a lot it's almost 2x smaller constant data, so I think it should be worth the extra effort.

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Nov 2, 2024

P.S. I think that some crates use cc-rs at runtime as a normal dep, so this should reduce their binary size.

@NobodyXu NobodyXu merged commit 9eb0c38 into main Nov 2, 2024
54 checks passed
@NobodyXu NobodyXu deleted the optimize branch November 2, 2024 05:01
@github-actions github-actions bot mentioned this pull request Nov 2, 2024
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.

2 participants