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

refactor: replace format! with concat! for string literals #2695

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Integral-Tech
Copy link

@Integral-Tech Integral-Tech commented Dec 11, 2024

  • Although format! macro is very powerful, it introduces overhead of memory allocation on the heap compared to &str. Therefore, when format! macro is unnecessary, we should avoid using it.

@ricvelozo
Copy link

I think the format macro already optimizes static strings (like the env! ones).

See: https://blog.m-ou.se/format-args/#small-steps

runtime/src/debug/basic.rs Outdated Show resolved Hide resolved
@Integral-Tech
Copy link
Author

Integral-Tech commented Dec 21, 2024

I think the format macro already optimizes static strings (like the env! ones).

See: https://blog.m-ou.se/format-args/#small-steps

They are different. concat!() constructs the &str during compilation, which is still faster.

@ricvelozo
Copy link

I think the format macro already optimizes static strings (like the env! ones).

See: https://blog.m-ou.se/format-args/#small-steps

They are different. concat!() constructs the &str during compilation, which is still faster.

Read the link I wrote.

@Integral-Tech
Copy link
Author

Integral-Tech commented Dec 21, 2024

@ricvelozo I have read your link, it says format_args()! can inline static strings.
However, format!() is equivalent to fmt::format(format_args!()). The function fmt::format still involves heap allocation.
If you don't like using concat!(), format_args!().as_str() can be used instead to avoid heap allocation.

@ricvelozo
Copy link

@ricvelozo I have read your link, it says format_args()! can inline static strings.
However, format!() is equivalent to fmt::format(format_args!()). The function fmt::format still involves heap allocation.
If you don't like using concat!(), format_args!().as_str() can be used instead to avoid heap allocation.

Like I said in the review, you are using the ToString trait, and that allocates.

@Integral-Tech
Copy link
Author

Integral-Tech commented Dec 21, 2024

@ricvelozo I have just removed the change that involves .to_string() call :)

@ricvelozo
Copy link

@ricvelozo I have just removed the change involves .to_string() call :)

Nice! 👍🏻

@Integral-Tech
Copy link
Author

@ricvelozo Could you please approve the workflow run?

@ricvelozo
Copy link

@ricvelozo Could you please approve the workflow run?

Needs Héctor's approval. Eventually he will review.

@Koranir
Copy link
Contributor

Koranir commented Dec 21, 2024

All these usecases convert the given input into a PathBuf anyways, which does allocate and copy if made with static string slice data. PathBufs don't allocate if created from a String, so this PR doesn't really change anything to do with allocation amounts.

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.

3 participants