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 std::panic::Location to get log location information #599

Conversation

pwoolcoc
Copy link

No description provided.

@pwoolcoc
Copy link
Author

At some point I deleted the fork that I used to open #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.

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.

Can you remove the line! and file! macros?

@Thomasdezeeuw
Copy link
Collaborator

@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 pwoolcoc force-pushed the use-std-panic-location-for-file-and-line-info branch from d3f2eee to 1dc2526 Compare November 27, 2023 14:58
@pwoolcoc
Copy link
Author

Can you remove the line! and file! macros?

Done!

src/__private_api.rs Outdated Show resolved Hide resolved
@EFanZh
Copy link
Contributor

EFanZh commented Nov 28, 2023

@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.

@pwoolcoc pwoolcoc force-pushed the use-std-panic-location-for-file-and-line-info branch from 1dc2526 to 54a2103 Compare November 28, 2023 13:59
@EFanZh
Copy link
Contributor

EFanZh commented Nov 29, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

EFanZh commented Nov 29, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

EFanZh commented Dec 3, 2023

@Thomasdezeeuw I have submitted an issue here: rust-lang/rust#118557.

@brunowonka
Copy link

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
Copy link
Collaborator

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
Copy link
Contributor

KodrAus commented Jun 25, 2024

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!

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.

5 participants