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

Try to use a trait for calling log impl #675

Closed
wants to merge 1 commit into from
Closed

Try to use a trait for calling log impl #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 33 additions & 47 deletions src/__private_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,57 +52,43 @@ impl Log for GlobalLogger {
}
}

// Split from `log` to reduce generics and code size
fn log_impl<L: Log>(
logger: L,
args: Arguments,
level: Level,
&(target, module_path, loc): &(&str, &'static str, &'static Location),
kvs: Option<&[(&str, Value)]>,
) {
#[cfg(not(feature = "kv"))]
if kvs.is_some() {
panic!("key-value support is experimental and must be enabled using the `kv` feature")
pub trait LogExt: Log {
fn do_enabled(&self, level: Level, target: &str) -> bool {
self.enabled(&Metadata::builder().level(level).target(target).build())
}

let mut builder = Record::builder();

builder
.args(args)
.level(level)
.target(target)
.module_path_static(Some(module_path))
.file_static(Some(loc.file()))
.line(Some(loc.line()));

#[cfg(feature = "kv")]
builder.key_values(&kvs);

logger.log(&builder.build());
}

pub fn log<'a, K, L>(
logger: L,
args: Arguments,
level: Level,
target_module_path_and_loc: &(&str, &'static str, &'static Location),
kvs: K,
) where
K: KVs<'a>,
L: Log,
{
log_impl(
logger,
args,
level,
target_module_path_and_loc,
kvs.into_kvs(),
)
fn do_log<'a>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we’re going to do it this way these names should probably be much more mangled, like __log_private_api_log. Similarly, the name of the trait should also be ugly so it doesn’t look like something you’re meant to use directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I'd first check whether this way work (so that all the zero-size logger can benefit). And rename later.

&self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This forces caller to pass a non-ZST reference, which causes the ZST optimization to lose effect.

Copy link
Contributor Author

@tisonkun tisonkun Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that the final fn log(&self, ..) and fn enabled(&self, ..) method take a reference also.

So the optimization happens only in the caller-side if we take the ownership, as you described in #576 (comment)?

Copy link
Contributor

@EFanZh EFanZh Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. You need to make sure that the arguments passed to the outermost function are ZSTs. That’s why the original generic wrapper function exists.

Copy link
Contributor Author

@tisonkun tisonkun Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Then I'd close this PR as not work.

Although, I wonder if write! use $dst.write_fmt that accepts a reference already, is the ZST optimization significant? Or a net win?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The binary size reduction depends on the number of log macro calls, it may have different impact on different projects. I remember seeing observable binary size reduction in one of my projects with a lot of logs with this optimization.

args: Arguments,
level: Level,
&(target, module_path, loc): &(&str, &'static str, &'static Location),
kvs: impl KVs<'a>,
) {
let kvs = kvs.into_kvs();

#[cfg(not(feature = "kv"))]
if kvs.is_some() {
panic!("key-value support is experimental and must be enabled using the `kv` feature")
}

let mut builder = Record::builder();

builder
.args(args)
.level(level)
.target(target)
.module_path_static(Some(module_path))
.file_static(Some(loc.file()))
.line(Some(loc.line()));

#[cfg(feature = "kv")]
builder.key_values(&kvs);

self.log(&builder.build());
}
}

pub fn enabled<L: Log>(logger: L, level: Level, target: &str) -> bool {
logger.enabled(&Metadata::builder().level(level).target(target).build())
}
impl<T: Log> LogExt for T {}

#[track_caller]
pub fn loc() -> &'static Location<'static> {
Expand Down
64 changes: 27 additions & 37 deletions src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ macro_rules! log {
// log!(logger: my_logger, target: "my_target", Level::Info, "a {} event", "log");
(logger: $logger:expr, target: $target:expr, $lvl:expr, $($arg:tt)+) => ({
$crate::__log!(
logger: $crate::__log_logger!($logger),
logger: $logger,
target: $target,
$lvl,
$($arg)+
Expand All @@ -86,7 +86,7 @@ macro_rules! log {
// log!(logger: my_logger, Level::Info, "a log event")
(logger: $logger:expr, $lvl:expr, $($arg:tt)+) => ({
$crate::__log!(
logger: $crate::__log_logger!($logger),
logger: $logger,
target: $crate::__private_api::module_path!(),
$lvl,
$($arg)+
Expand All @@ -96,7 +96,7 @@ macro_rules! log {
// log!(target: "my_target", Level::Info, "a log event")
(target: $target:expr, $lvl:expr, $($arg:tt)+) => ({
$crate::__log!(
logger: $crate::__log_logger!(__log_global_logger),
logger: $crate::__private_api::GlobalLogger,
target: $target,
$lvl,
$($arg)+
Expand All @@ -106,7 +106,7 @@ macro_rules! log {
// log!(Level::Info, "a log event")
($lvl:expr, $($arg:tt)+) => ({
$crate::__log!(
logger: $crate::__log_logger!(__log_global_logger),
logger: $crate::__private_api::GlobalLogger,
target: $crate::__private_api::module_path!(),
$lvl,
$($arg)+
Expand All @@ -121,8 +121,9 @@ macro_rules! __log {
(logger: $logger:expr, target: $target:expr, $lvl:expr, $($key:tt $(:$capture:tt)? $(= $value:expr)?),+; $($arg:tt)+) => ({
let lvl = $lvl;
if lvl <= $crate::STATIC_MAX_LEVEL && lvl <= $crate::max_level() {
$crate::__private_api::log(
$logger,
use $crate::__private_api::LogExt;

$logger.do_log(
$crate::__private_api::format_args!($($arg)+),
lvl,
&($target, $crate::__private_api::module_path!(), $crate::__private_api::loc()),
Expand All @@ -135,8 +136,9 @@ macro_rules! __log {
(logger: $logger:expr, target: $target:expr, $lvl:expr, $($arg:tt)+) => ({
let lvl = $lvl;
if lvl <= $crate::STATIC_MAX_LEVEL && lvl <= $crate::max_level() {
$crate::__private_api::log(
$logger,
use $crate::__private_api::LogExt;

$logger.do_log(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to refer to the trait directly: LogExt::do_log(...), otherwise if the logger has a do_log method that is not a trait method, it will be called here.

$crate::__private_api::format_args!($($arg)+),
lvl,
&($target, $crate::__private_api::module_path!(), $crate::__private_api::loc()),
Expand Down Expand Up @@ -166,13 +168,13 @@ macro_rules! error {
// error!(logger: my_logger, target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
// error!(logger: my_logger, target: "my_target", "a {} event", "log")
(logger: $logger:expr, target: $target:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), target: $target, $crate::Level::Error, $($arg)+)
$crate::log!(logger: $logger, target: $target, $crate::Level::Error, $($arg)+)
});

// error!(logger: my_logger, key1 = 42, key2 = true; "a {} event", "log")
// error!(logger: my_logger, "a {} event", "log")
(logger: $logger:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), $crate::Level::Error, $($arg)+)
$crate::log!(logger: $logger, $crate::Level::Error, $($arg)+)
});

// error!(target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
Expand Down Expand Up @@ -205,13 +207,13 @@ macro_rules! warn {
// warn!(logger: my_logger, target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
// warn!(logger: my_logger, target: "my_target", "a {} event", "log")
(logger: $logger:expr, target: $target:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), target: $target, $crate::Level::Warn, $($arg)+)
$crate::log!(logger: $logger, target: $target, $crate::Level::Warn, $($arg)+)
});

// warn!(logger: my_logger, key1 = 42, key2 = true; "a {} event", "log")
// warn!(logger: my_logger, "a {} event", "log")
(logger: $logger:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), $crate::Level::Warn, $($arg)+)
$crate::log!(logger: $logger, $crate::Level::Warn, $($arg)+)
});

// warn!(target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
Expand Down Expand Up @@ -253,13 +255,13 @@ macro_rules! info {
// info!(logger: my_logger, target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
// info!(logger: my_logger, target: "my_target", "a {} event", "log")
(logger: $logger:expr, target: $target:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), target: $target, $crate::Level::Info, $($arg)+)
$crate::log!(logger: $logger, target: $target, $crate::Level::Info, $($arg)+)
});

// info!(logger: my_logger, key1 = 42, key2 = true; "a {} event", "log")
// info!(logger: my_logger, "a {} event", "log")
(logger: $logger:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), $crate::Level::Info, $($arg)+)
$crate::log!(logger: $logger, $crate::Level::Info, $($arg)+)
});

// info!(target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
Expand Down Expand Up @@ -293,13 +295,13 @@ macro_rules! debug {
// debug!(logger: my_logger, target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
// debug!(logger: my_logger, target: "my_target", "a {} event", "log")
(logger: $logger:expr, target: $target:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), target: $target, $crate::Level::Debug, $($arg)+)
$crate::log!(logger: $logger, target: $target, $crate::Level::Debug, $($arg)+)
});

// debug!(logger: my_logger, key1 = 42, key2 = true; "a {} event", "log")
// debug!(logger: my_logger, "a {} event", "log")
(logger: $logger:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), $crate::Level::Debug, $($arg)+)
$crate::log!(logger: $logger, $crate::Level::Debug, $($arg)+)
});

// debug!(target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
Expand Down Expand Up @@ -337,13 +339,13 @@ macro_rules! trace {
// trace!(logger: my_logger, target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
// trace!(logger: my_logger, target: "my_target", "a {} event", "log")
(logger: $logger:expr, target: $target:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), target: $target, $crate::Level::Trace, $($arg)+)
$crate::log!(logger: $logger, target: $target, $crate::Level::Trace, $($arg)+)
});

// trace!(logger: my_logger, key1 = 42, key2 = true; "a {} event", "log")
// trace!(logger: my_logger, "a {} event", "log")
(logger: $logger:expr, $($arg:tt)+) => ({
$crate::log!(logger: $crate::__log_logger!($logger), $crate::Level::Trace, $($arg)+)
$crate::log!(logger: $logger, $crate::Level::Trace, $($arg)+)
});

// trace!(target: "my_target", key1 = 42, key2 = true; "a {} event", "log")
Expand Down Expand Up @@ -391,22 +393,22 @@ macro_rules! trace {
macro_rules! log_enabled {
// log_enabled!(logger: my_logger, target: "my_target", Level::Info)
(logger: $logger:expr, target: $target:expr, $lvl:expr) => ({
$crate::__log_enabled!(logger: $crate::__log_logger!($logger), target: $target, $lvl)
$crate::__log_enabled!(logger: $logger, target: $target, $lvl)
});

// log_enabled!(logger: my_logger, Level::Info)
(logger: $logger:expr, $lvl:expr) => ({
$crate::__log_enabled!(logger: $crate::__log_logger!($logger), target: $crate::__private_api::module_path!(), $lvl)
$crate::__log_enabled!(logger: $logger, target: $crate::__private_api::module_path!(), $lvl)
});

// log_enabled!(target: "my_target", Level::Info)
(target: $target:expr, $lvl:expr) => ({
$crate::__log_enabled!(logger: $crate::__log_logger!(__log_global_logger), target: $target, $lvl)
$crate::__log_enabled!(logger: $crate::__private_api::GlobalLogger, target: $target, $lvl)
});

// log_enabled!(Level::Info)
($lvl:expr) => ({
$crate::__log_enabled!(logger: $crate::__log_logger!(__log_global_logger), target: $crate::__private_api::module_path!(), $lvl)
$crate::__log_enabled!(logger: $crate::__private_api::GlobalLogger, target: $crate::__private_api::module_path!(), $lvl)
});
}

Expand All @@ -415,24 +417,12 @@ macro_rules! log_enabled {
macro_rules! __log_enabled {
// log_enabled!(logger: my_logger, target: "my_target", Level::Info)
(logger: $logger:expr, target: $target:expr, $lvl:expr) => {{
use $crate::__private_api::LogExt;

let lvl = $lvl;
lvl <= $crate::STATIC_MAX_LEVEL
&& lvl <= $crate::max_level()
&& $crate::__private_api::enabled($logger, lvl, $target)
}};
}

// Determine the logger to use, and whether to take it by-value or by reference

#[doc(hidden)]
#[macro_export]
macro_rules! __log_logger {
(__log_global_logger) => {{
$crate::__private_api::GlobalLogger
}};

($logger:expr) => {{
&($logger)
&& $logger.do_enabled(lvl, $target)
}};
}

Expand Down