search

rust-lang / log

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

Add an optional feature for trading lazy evaluation for smaller binary size? #525

Open EFanZh opened 2 years ago

EFanZh commented 2 years ago

I tried to submit https://github.com/rust-lang/log/pull/514 for reducing binary size, but realized that that change will break the current lazy evaluation behavior, so I closed that PR. But is is OK to add an optional feature that enables what the PR does? So users that need smaller binary size can enable this feature.

Thomasdezeeuw commented 2 years ago

How would you guarantee that all your dependencies can work without lazy evaluation?

EFanZh commented 2 years ago

For my use case, I only care about logs printed by my code, so how dependencies print their logs do not matter. Also, this feature is optional, so the user who enabled it should be responsible for any effect this feature causes. If somehow a dependency is broken by this feature, I will consider fixing upstream or replace it with another dependency.

Thomasdezeeuw commented 2 years ago

I don't think we can introduce a feature that breaks any code. Even if it's a feature. Because any crate can enable the feature, enabling it for all crates. This means if I have dependencies A and B, A enables the feature and B can't work with the feature, I can't use dependencies A and B together any more.

sfackler commented 2 years ago

__private_api_log_lit can be re-added in a correct manner by using https://doc.rust-lang.org/stable/std/fmt/struct.Arguments.html#method.as_str. The crate would either have to bump its MSRV to 1.52 or do some conditional compilation with a build script.

KodrAus commented 2 years ago

We already depend on rustversion so could use conditional compilation here pretty easily.

KodrAus commented 1 year ago

We’ve bumped past 1.52 now so could make use of Arguments::as_str.

Thomasdezeeuw commented 5 months ago

@EFanZh is this still something you're interested in?

EFanZh commented 5 months ago

Yes, I am still interested. But currently, Arguments::as_str is marked #[inline], not #[inline(always)]. With opt-level=z, the compiler is not always able to determine the result at compile time: https://godbolt.org/z/b6jMbedzb. Also, even if Arguments::as_str is marked #[inline(always)] at a future Rust version, log still needs to support older Rust versions, which requires us to either bump its MSRV, or employ some sort of conditional compilation that selectively enables the Arguments::as_str optimization.