search

rust-lang / log

Logging implementation for Rust
https://docs.rs/log
Apache License 2.0
2.18k stars 252 forks source link

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

Closed KodrAus closed 3 months ago

KodrAus commented 3 months ago

Rework of #599. I've followed the suggestion in https://github.com/rust-lang/log/pull/599#issuecomment-2143472357 to pass a panic::Location to our non-inlined private log function in the hope it will avoid bloating callsites.

EFanZh commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 🙂