Try to use a trait for calling log impl #675
Conversation
Signed-off-by: tison <wander4096@gmail.com>
| target_module_path_and_loc, | ||
| kvs.into_kvs(), | ||
| ) | ||
| fn do_log<'a>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agree. I'd first check whether this way work (so that all the zero-size logger can benefit). And rename later.
| $logger, | ||
| use $crate::__private_api::LogExt; | ||
|
|
||
| $logger.do_log( |
There was a problem hiding this comment.
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.
| kvs.into_kvs(), | ||
| ) | ||
| fn do_log<'a>( | ||
| &self, |
There was a problem hiding this comment.
This forces caller to pass a non-ZST reference, which causes the ZST optimization to lose effect.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
3 participants
This follows up #673 (review).
cc @EFanZh not sure if it creates some overhead or how can I check it.