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

EFanZh commented

2022-08-18 22:06:45 -05:00

(Migrated from github.com)

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.

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

2022-08-19 02:51:52 -05:00

(Migrated from github.com)

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

EFanZh commented

2022-08-19 04:34:20 -05:00

(Migrated from github.com)

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

2022-08-19 04:37:52 -05:00

(Migrated from github.com)

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.

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

2022-08-19 06:21:22 -05:00

(Migrated from github.com)

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

`__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

2022-08-31 05:13:41 -05:00

(Migrated from github.com)

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

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

KodrAus commented

2023-04-13 05:48:07 -05:00

(Migrated from github.com)

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

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

❤️ 1

Thomasdezeeuw commented

2024-03-31 13:19:31 -05:00

(Migrated from github.com)

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

EFanZh commented

2024-03-31 22:05:49 -05:00

(Migrated from github.com)

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.

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.

👍 1

Rows
Columns

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