search

rust-lang / log

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

Use `std::panic::Location` to get log location information #599

Closed pwoolcoc closed 4 weeks ago

pwoolcoc commented 7 months ago

At some point I deleted the fork that I used to open https://github.com/rust-lang/log/pull/410, so this is just the updated version of that PR. I have removed the feature gate for this, as it was suggested that since the MSRV is now 1.60.0, this feature should be unconditional.

Thomasdezeeuw commented 7 months ago

@EFanZh I know you are/were interested in reducing the code size of log, do you want to check this pr for code size changes?

pwoolcoc commented 7 months ago

Can you remove the line! and file! macros?

Done!

EFanZh commented 7 months ago

@EFanZh I know you are/were interested in reducing the code size of log, do you want to check this pr for code size changes?

@Thomasdezeeuw Yes, I think I can have the result in a few days after my comment being addressed.

EFanZh commented 7 months ago

I have tested this PR on my project, I see an observable size increasing of the result binary. It seems that currently the compiler can’t optimize this implementation well: https://godbolt.org/z/1E4fWKv34.

Thomasdezeeuw commented 7 months ago

I have tested this PR on my project, I see an observable size increasing of the result binary. It seems that currently the compiler can’t optimize this implementation well: https://godbolt.org/z/1E4fWKv34.

I tried adding #[inline], which makes it a little better, but it's still more code than the file! & line! variant.

I also tried inline the call to Location::caller, but it generates the same code: https://godbolt.org/#z:OYLghAFBqd5TKALEBjA9gEwKYFFMCWALugE4A0BIEAZgQDbYB2AhgLbYgDkAjF%2BTXRMiAZVQtGIHgBYBQogFUAztgAKAD24AGfgCsp5eiyahSAVyVFyKxqiIEh1ZpgDC6embZMQAJnLOAGQImbAA5TwAjbFIpHnIAB3QlYgcmNw8vXwSklKEgkPC2KJieOJtsO1SRIhZSInTPbz9yyqFq2qJ8sMjo2OsauobM5oHO4O6i3tKASmt0M1JUTi4AUi0AQQB6ACo19fizCIBqGiYjomxLCHp0UBOmCBWfADZHl7AuSxZ7VCPLCiOZgAzD5pmCjisAOwAIT2R3hRxuwDerzojDAkFm9HGGIgYOmKyBsI2UIAInttps9nsDsdTudLkRrrcQPcUSiPl8fn8iBRgaDwVDiesEYjsEREehfoTSTzMCAQPFjARUAqAlLvqkFeJ6IxSHjCcLRUj2TdUAA6NHYPGGKXm7EhPFgw3UyGkrhY7gAVn43i4OnI6G4ACULBKlPNFtgIT4gXxyERtB7yBAUBg2PEGNFKNR05m9SAmARgEgiPQAJ4CBgXUhKagRJPkCLBWrl7jx5usUjlgDyEV0FUTvH46Y4wh7TArjZwETMwBcEnodeH5BwbGMwEk/v4hFIg4IADdLo3sOoKmYLu3%2BMELvRG9iIqRW24cI3eQQ2FfZjQjMAlAA1AhsAAdx7eJmCvORhDECROBkKDFBUDRG30OIjBMEBzEsQwCAiOtMUDeJ7CEZcAFoXD%2BcsincUj1xMB1gFIo9a1SGUDy0I5yKOdAiNIxgj3oGUgX4dBmNIAgcHwqBWA4EBsEIYimHIA8JDMZYfC0Hw%2BDBaxsFsRSnCYVx3EaAxAnGQpigMRJkkUoZvDiGzciYLpLKmXT9KqUZ7IMFpFPaOpXJ6Ep%2Bg6HyylGILJhKWYIwWJYpE9LgfXIP0AyDLgjiwohfiLEsy3LI4IHwYgyBjONpn4IcdFmVMQHmIgDisKgIDzLNSFCdhlmy3Li1LCt%2BHk0rxPlOJBGgnU4NkcbELUTRt3IVDyGAp94i/QxvV9RsMp7C8mu4mgsrDXr8orIq3Azdryp4SqEyTHSAGsQC9LQNq4aR%2BE/F7Uu27h%2BDrV7quTOBYDTLA8AUsgc1oLMZOWeMZpgyR4Jm5Q5pQrJ0NMMMPP3RwIGcHygX8Iyoqsnw/CcuyTMyYmqdSMnegp3HWiYAL6hp7xib8ryOkZmJma%2BQZOZAbnIos4LfFBOZ4uWJ4gRWL1oV5FhUAegB9HU9UV8kSVjRXoWCB1sB1mlDnuE4szxTjCVwGNnk5GpuX%2BCEYThBFLHlRVlVVEB1XEAy0EXaI8UtK2CRJN1qT1hWlZVtXNeD0hTZjg2jfGFP9nN%2Bljet0jbcBEFXaND2iC9pUi19/3NUcIPdRD6Z7XGA1I919YzbpM4LiuJFWVOdkngdz4nZVHk%2BRBfFi/d%2BETUHmHGBtXP8RdVvo62XYNlpC3u6ZXu2TnwfHc1X5/nIflJ6FaexQlM0ISBWVPYVCuVTVDVA61huV5FBFZ5eZkLStIvO0S9nREldG3SkyY7zJS2gtDKoZLB/EjEscqPgqr3RTGDS6epoZtQLHlfqlY0Q1jrBABsC1OytkgpQ7sfYBx2EgqOZgRAJxTgWjOOcC5dTLnjGuDcW4Ay7n3EeZcAZTznkvCuG8el7y4SfN2F8ywAzvk/MOb8v4AJAVAuBP0CN5BIymghNGyEFr6D8FjTCOMHxSVmDxRSZEKJKCohEGidFNzBEYsxZIQg2IcS4nYvi2ABJCREmJCSx5YBwzkgpVIylVLqU0tpWKek8beAJkZcKJNMD82sjkamGQHLZFsgzCW0VfIpNZuzTJPM2jiwKJLCKYURaNMCqUqyN0ZZRl8ElFKaURLcCOpYE6hCiolRIKQVBt0ga1RQA1JquD0DYOiJ1WSXAepHAIQVQakMRpZERpNKQ015DGPmgGMxy1VrrWgb036XBdqNQvAdQZOUNl9QKudRZ%2BZoiTPQduHSSBsAsBwDEG0T1vrQI%2Bj9OBf1rAgEBvdJKaDyBfVen0wMMLpkpkQHVQgNAaDQ32bBQ5RikKnP4PQJAdY0KUtxTQIg5ZwJwsMJS08Ks7Fai4PwUglKsjcqULS%2BljLXp8tZU%2BdljhOU9NgelbgpICB4qOIBEC3z1mbLOgeJQzzhlbLun82YAKgW9EXtwSFKKoUys5bC%2BFeryBPR4FoV60DhLmv6Za6ZswvFamkEAA%3D%3D

EFanZh commented 7 months ago

I think the problem is that additional function calls somehow prevent the compiler from treating constant values as static values: https://godbolt.org/z/57K9hdEcc.

Thomasdezeeuw commented 7 months ago

I think the problem is that additional function calls somehow prevents the compiler from treating constant values as static values: https://godbolt.org/z/57K9hdEcc.

I don't really have a good solution for this. Maybe it's worth opening a discussion on rustc's Zullip? Or an issue in the rustc tracker?

EFanZh commented 7 months ago

@Thomasdezeeuw I have submitted an issue here: https://github.com/rust-lang/rust/issues/118557.

brunowonka commented 1 month ago

This seems useful for helpers marked with #[track_caller] that do logging internally to opt-in or out of having the caller location tagged in the log line. What would the apetite be for using Location:caller vs file!(), line!() conditionally based on an argument to the macros?

If there's interest I can try to get a PR up

Thomasdezeeuw commented 1 month ago

Maybe we can change the __private_api::log function to accept &std::panic::Location? Maybe that will not increase the binary size?

I'm not sure introducing complexity with a conditional would be the best way forward, but we can experiment with it.

KodrAus commented 4 weeks ago

I've re-opened this as #633 to get the ball rolling again. We can still find our way back to the discussion here if necessary.

Thanks for the original PR @pwoolcoc!