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.

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

Group target_module, path and file arguments #575

Merged
merged 2 commits into from
Aug 12, 2023

Conversation

EFanZh
Copy link
Contributor

@EFanZh EFanZh commented Aug 7, 2023

This is the first step of bring optimizations from #569 into the log crate.

This PR contains two commits:

  • The first commit moves all private APIs into a single __private_api module so that it is easier to implement more elaborated optimizations.
  • The second commit splits &(target, module_path, file, line) argument into two arguments: &(target, module_path, file) and line. 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:

opt-level lto codegen-units original size optimized size diff
3 off 1 14211752 14199464 -12288
3 off 16 16739048 16739048 0
3 thin 1 15170120 15166024 -4096
3 thin 16 17566376 17566376 0
3 fat 1 13859336 13859336 0
3 fat 16 15231496 15231496 0
"z" off 1 11451048 11438760 -12288
"z" off 16 13531848 13511368 -20480
"z" thin 1 12307048 12298856 -8192
"z" thin 16 13699720 13695624 -4096
"z" fat 1 9951752 9935368 -16384
"z" fat 16 10476040 10459656 -16384

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Have you also looked at the performance difference, if any?

@EFanZh EFanZh marked this pull request as ready for review August 7, 2023 16:20
@EFanZh
Copy link
Contributor Author

EFanZh commented Aug 7, 2023

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 Cargo.toml:

criterion = "*"
log = "*"
log_optimized = { git = "ssh://[email protected]/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.

Copy link

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Amazing!

Thank you for this PR and testing this on cargo-binstall!

@EFanZh
Copy link
Contributor Author

EFanZh commented Aug 12, 2023

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?

Copy link
Contributor

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for working on this @EFanZh!

@KodrAus KodrAus merged commit 838920c into rust-lang:master Aug 12, 2023
14 checks passed
@EFanZh EFanZh deleted the group-target-module-path-and-file branch August 13, 2023 06:21
@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants