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

Use Location::caller() for file and line info #633

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Jun 25, 2024

Rework of #599. I've followed the suggestion in #599 (comment) to pass a panic::Location to our non-inlined private log function in the hope it will avoid bloating callsites.

@@ -341,10 +341,6 @@
#![warn(missing_docs)]
#![deny(missing_debug_implementations, unconditional_recursion)]
#![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
// When compiled for the rustc compiler itself we want to make sure that this is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #632

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

LGTM.

I do wondering what this does to the binary size, but I'm personally not that affected by it, so I have no setup to test is properly.

@EFanZh
Copy link
Contributor

EFanZh commented Jun 26, 2024

I haven’t tested it on a real project, but based on this assembly comparison, I think this PR will cause some binary size bloat.

@KodrAus
Copy link
Contributor Author

KodrAus commented Jun 26, 2024

@EFanZh I think most changes we can make are likely to impact binary size in one way or another and we don't have the CI infrastructure to measure this so am hesitant to block this change purely based on the possible increase. It sets a precedent that blocks pretty much any future work.

Having said that, I do think binary size is a useful metric for us to track, so if this is really important to you then I think we'd happily accept a PR that introduced a CI job and test project to track how binary size changes. That way we can actually evaluate the impact of changes as they come through.

@EFanZh
Copy link
Contributor

EFanZh commented Jun 26, 2024

Currently, binary size is less important to me than before, I just want to provide some possibly useful information, in case someone else is interested or affected by it.

As for the CI job, I think it will need a dedicated data server for storing the binary size history data, which I think it is a too big project for me.

@Thomasdezeeuw
Copy link
Collaborator

As for the CI job, I think it will need a dedicated data server for storing the binary size history data, which I think it is a too big project for me.

We could use GitHub Actions cache for that: https://github.com/actions/cache. But totally fair if you don't want to take that on.

I think we're good to merge then.

@EFanZh
Copy link
Contributor

EFanZh commented Jun 26, 2024

For Rust version 1.79+, with inline const and Location::caller being stabilized, we might be able to employ the same optimization: https://godbolt.org/z/8Gn4Ycne8.

But I am not sure whether #[track_caller] would still work.

@KodrAus
Copy link
Contributor Author

KodrAus commented Jun 26, 2024

As for the CI job, I think it will need a dedicated data server for storing the binary size history data, which I think it is a too big project for me.

Oh yes that would be a lot. I was imagining something less sophisticated. Just a test that generates a project with 1,000 log statements in it, compiles and strips it, and asserts the resulting binary is less than some constant value. If the size changed then we'd need to update that constant, giving us a history of when it changed and why. Something we can keep in mind for the future 🙂

@KodrAus KodrAus merged commit 46894ef into master Jun 26, 2024
28 checks passed
@KodrAus KodrAus deleted the feat/panic-info branch June 27, 2024 23:13
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