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

Use less allocations in recording Display and Error values #1988

Merged
merged 2 commits into from
Mar 16, 2022

Conversation

hrxi
Copy link
Contributor

@hrxi hrxi commented Mar 12, 2022

Use the fact that the Debug implementation of DisplayValue
corresponds to the Display implementation of the wrapped type. This
means that the string allocation of the Displayed type can be avoided
entirely.

Use the fact that the `Debug` implementation of `DisplayValue`
corresponds to the `Display` implementation of the wrapped type. This
means that the string allocation of the `Display`ed type can be avoided
entirely.
@hrxi hrxi requested review from hawkw and carllerche as code owners March 12, 2022 20:23
@hawkw
Copy link
Member

hawkw commented Mar 12, 2022

I don't believe this actually avoids any allocations, as format_args! doesn't allocate a String, it just returns fmt::Arguments that implements Debug; it's format! that allocates a string.

But, avoiding the macros is a bit nicer when they're not necessary, so I would be fine making this change.

@hrxi
Copy link
Contributor Author

hrxi commented Mar 13, 2022

You're right, I was thinking of format! when I made this change. Should I adjust the commit message?

@davidbarsky
Copy link
Member

You're right, I was thinking of format! when I made this change. Should I adjust the commit message?

We can change the commit message when we merge + squash—don't worry about that.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Going to go ahead and merge this --- although it doesn't actually reduce allocations, I think it's a bit nicer to avoid the unnecessary use of format_args! macros.

@hawkw
Copy link
Member

hawkw commented Mar 16, 2022

huh, seems like the nightly compiler ICEs on this PR for some reason --- i wonder if that happens on main, too? https://github.com/tokio-rs/tracing/runs/5573114704?check_suite_focus=true

@hrxi
Copy link
Contributor Author

hrxi commented Mar 16, 2022

@hawkw Current nightly is broken, CI runs also fail on other repos with that ICE.

@hawkw hawkw merged commit 989fb62 into tokio-rs:master Mar 16, 2022
hawkw pushed a commit that referenced this pull request Mar 18, 2022
This branch removes some unnecessary uses of the `format_args!`
and `write!` macros in `tracing-core`. Using `fmt::Display::fmt` and
similar rather than the macros may be slightly more efficient.

Co-authored-by: David Barsky <[email protected]>
hawkw pushed a commit that referenced this pull request Mar 22, 2022
This branch removes some unnecessary uses of the `format_args!`
and `write!` macros in `tracing-core`. Using `fmt::Display::fmt` and
similar rather than the macros may be slightly more efficient.

Co-authored-by: David Barsky <[email protected]>
hawkw added a commit that referenced this pull request Apr 1, 2022
# 0.1.24 (April 1, 2022)

This release fixes a bug where setting `NoSubscriber` as the local
default would not locally disable the current global default subscriber.

### Fixed

- Setting `NoSubscriber` as the local default now correctly disables the
  global default subscriber ([#2001])
- Fixed compilation warnings with the "std" feature disabled ([#2022])

### Changed

- Removed unnecessary use of `write!` and `format_args!` macros
  ([#1988])

[#1988]: #1988
[#2001]: #2001
[#2022]: #2022
hawkw added a commit that referenced this pull request Apr 1, 2022
# 0.1.24 (April 1, 2022)

This release fixes a bug where setting `NoSubscriber` as the local
default would not locally disable the current global default subscriber.

### Fixed

- Setting `NoSubscriber` as the local default now correctly disables the
  global default subscriber ([#2001])
- Fixed compilation warnings with the "std" feature disabled ([#2022])

### Changed

- Removed unnecessary use of `write!` and `format_args!` macros
  ([#1988])

[#1988]: #1988
[#2001]: #2001
[#2022]: #2022
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.24 (April 1, 2022)

This release fixes a bug where setting `NoSubscriber` as the local
default would not locally disable the current global default subscriber.

### Fixed

- Setting `NoSubscriber` as the local default now correctly disables the
  global default subscriber ([tokio-rs#2001])
- Fixed compilation warnings with the "std" feature disabled ([tokio-rs#2022])

### Changed

- Removed unnecessary use of `write!` and `format_args!` macros
  ([tokio-rs#1988])

[tokio-rs#1988]: tokio-rs#1988
[tokio-rs#2001]: tokio-rs#2001
[tokio-rs#2022]: tokio-rs#2022
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.

3 participants