Group target_module, path and file arguments#575
Conversation
Thomasdezeeuw
left a comment
There was a problem hiding this comment.
Changes LGTM. Have you also looked at the performance difference, if any?
|
OK, I have tested the performance using this code: use criterion::Criterion;
fn log_string_literal(c: &mut Criterion) {
let mut benchmark_group = c.benchmark_group("log string literal");
benchmark_group.bench_function("original", |b| b.iter(|| log::info!("abc")));
benchmark_group.bench_function("optimized", |b| b.iter(|| log_optimized::info!("abc")));
benchmark_group.finish();
}
criterion::criterion_group!(benches, log_string_literal);
criterion::criterion_main!(benches);with the following dependencies in criterion = "*"
log = "*"
log_optimized = { git = "ssh://git@github.com/EFanZh/log.git", branch = "group-target-module-path-and-file", package = "log" }On average, the original one takes 246.84 ps per iteration, and the optimized one takes 245.54 ps per iteration, I consider this difference insignificant. |
NobodyXu
left a comment
There was a problem hiding this comment.
Amazing!
Thank you for this PR and testing this on cargo-binstall!
|
Hi @KodrAus, I’d really appreciate it if you could take a look at this PR. I intend to do some binary size optimizations with ideas from #569. I plan to test and implement each idea with a separate PR, so if this PR gets accepted, I can move on to the next PR. Will these sort of optimizations be accepted? |
This is the first step of bring optimizations from #569 into the
logcrate.This PR contains two commits:
__private_apimodule so that it is easier to implement more elaborated optimizations.&(target, module_path, file, line)argument into two arguments:&(target, module_path, file)andline. The idea is that a module could have more than one log calls, so most likely, these log calls would have the same(target, module_path, file)triple, group them together will allow multiple log calls to reuse a same(target, module_path, file)instance, see https://godbolt.org/z/z7jW67Ece for comparison.For one of my internal projects, this PR will reduce the binary size by about 2%.
I also tested this PR on
cargo-binstall, this is the test result: