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.

Sign up for GitHub

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

Jump to bottom

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

Open
EFanZh opened this issue Aug 19, 2022 · 8 comments
Open

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

EFanZh opened this issue Aug 19, 2022 · 8 comments

Comments

@EFanZh
Copy link
Contributor

EFanZh commented Aug 19, 2022

I tried to submit #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.
@EFanZh EFanZh changed the title Add an optional feature for trading lazy evaluation of log macro arguments for smaller binary size? Add an optional feature for trading lazy evaluation for smaller binary size? Aug 19, 2022
@Thomasdezeeuw
Copy link
Collaborator

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

@EFanZh
Copy link
Contributor Author

EFanZh commented Aug 19, 2022

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

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

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

KodrAus commented Aug 31, 2022

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

@KodrAus
Copy link
Contributor

KodrAus commented Apr 13, 2023

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

EFanZh pushed a commit to EFanZh/log that referenced this issue Jul 23, 2023
@NobodyXu
Replace simplelog with tracing_subscriber::fmt (rust-lang#525)
3841762 
* Disable feat log-always of dep tracing
* Add dep tracing-log 0.1.3 with no feat
* Add new dep tracing-appender v0.2.2
* Add dep tracing-subscriber 0.3.16 with feat fmt and json
* Fix `MainExit::report`: Do not use `log::{error, warn}`
  since `MainExit::report` might be called with no `log`ger, it can only
  use `println!` and `eprintln!`.
* Use `tracing_subscriber::fmt` instead of `simple_log`
* Rm unused dep simplelog from crates/bin
* Fix `BinstallError::report`: Avoid `log::{warn, error}`
   since they might be called after `tracing_appender::WorkerGuard` is
   dropped.
* Make tracing output more readable to end users
* Add new dep tracing-core v0.1.30
* Add new dep once_cell v1.16.0
* Refactor: Extract new mod `logging`
* Add new option `Args::json_output`
* Fix `MainExit::report`: Ignore io error
* Fix `BinstallError::report`: Ignore IO error

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@Thomasdezeeuw
Copy link
Collaborator

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

@EFanZh
Copy link
Contributor Author

EFanZh commented Apr 1, 2024 •
edited

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sfackler @EFanZh @Thomasdezeeuw @KodrAus