Skip to content

tracing: remove dispatcher::has_been_set check in macros #868

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

Jump to bottom
Merged
merged 1 commit into from
Jul 31, 2020
Merged

tracing: remove dispatcher::has_been_set check in macros #868

merged 1 commit into from
Jul 31, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 31, 2020 •
edited
Loading

This PR removes the dispatcher::has_been_set check from the
level_enabled! macro. We now check the static max level prior to
checking anything else, which avoids any atomic loads for callsites
disabled by the static max level. Also, checking the value of
LevelFilter::current should now be essentially equivalent to the
dispatcher::has_been_set check, since the default global max
LevelFilter is OFF when no dispatcher has been set. This means that
rather than doing two memory accesses if a callsite is disabled by the
global max level (one for the dispatcher set flag, and one for the max
level), we only do one memory access.

The has_been_set flag is also consumed by the log integration, for
determining whether to emit log records if "log-always" is not
enabled. We could probably replace that with a check if
LevelFilter::current() is OFF, but I dunno if this is necessary just
to remove that function. Also, we can't remove it from tracing-core,
since we want tracing-core to be backwards-compatible with previous
versions of tracing.

Signed-off-by: Eliza Weisman [email protected]

@hawkw
tracing: remove dispatcher::has_been_set check in macros
23f8651 
This PR removes the `dispatcher::has_been_set` check from the
`level_enabled!` macro. We now check the static max level prior to
checking anything else, which avoids any atomic loads for callsites
disabled by the static max level. Also, checking the value of
`LevelFilter::current` should now be essentially equivalent to the
`dispatcher::has_been_set` check, since the default global max
`LevelFilter` is `OFF` when no dispatcher has been set. This means that
rather than doing *two* memory accesses if a callsite is disabled by the
global max level (one for the dispatcher set flag, and one for the max
level), we only do one memory access.

The `has_been_set` flag is also consumed by the `log` integration, for
determining whether to emit `log` records if "log-always" is not
enabled. We *could* probably replace that with a check if
`LevelFilter::current()` is `OFF`, but I dunno if this is necessary just
to remove that function. Also, we can't remove it from `tracing-core`,
since we want `tracing-core` to be backwards-compatible with previous
versions of `tracing`.

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw added crate/tracing Related to the `tracing` crate kind/perf Performance improvements labels Jul 31, 2020
@hawkw hawkw requested a review from a team as a code owner July 31, 2020 16:56
@hawkw hawkw mentioned this pull request Jul 31, 2020
Merged
@hawkw hawkw merged commit 110d32e into master Jul 31, 2020
hawkw added a commit that referenced this pull request Jul 31, 2020
@hawkw
tracing: prepare to release 0.1.18
337e2ab 
Fixed

- Fixed a bug where `LevelFilter::OFF` (and thus also the
  `static_max_level_off`  feature flag) would enable *all* traces,
  rather than *none* (#853)
- **log**: Fixed `tracing` macros and `Span`s not checking
  `log::max_level` before emitting `log` records (#870)

Changed

- **macros**: Macros now check the global max level
  (`LevelFilter::current`) before the per-callsite cache when
  determining if a span or event is enabled. This significantly improves
  performance in some use cases (#853)
- **macros**: Simplified the code generated by macro expansion
  significantly, which may improve compile times and/or `rustc`
  optimizatation of surrounding code (#869, #869)
- **macros**: Macros now check the static max level before checking any
  runtime filtering, improving performance when a span or event is
  disabled by a `static_max_level_XXX` feature flag (#868)
- `LevelFilter` is now a re-export of the `tracing_core::LevelFilter`
  type, it can now be used interchangably with the versions in
  `tracing-core` and `tracing-subscriber` (#853)
- Significant performance improvements when comparing `LevelFilter`s and
  `Level`s (#853)
- Updated the minimum `tracing-core` dependency to 0.1.12 (#853)

Added

- **macros**: Quoted string literals may now be used as field names, to
  allow fields whose names are not valid Rust identifiers (#790)
- **docs**: Several documentation improvements (#850, #857, #841)
- `LevelFilter::current()` function, which returns the highest level
  that any subscriber will enable (#853)
- `Subscriber::max_level_hint` optional trait method, for setting the
  value returned by `LevelFilter::current()` (#853)

Thanks to new contributors @cuviper, @ethanboxx, @ben0x539, @dignati,
@colelawrence, and @rbtcollins for helping out with this release!

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw mentioned this pull request Jul 31, 2020
Merged
hawkw added a commit that referenced this pull request Jul 31, 2020
@hawkw
tracing: prepare to release 0.1.18 (#873)
c9f8b6d 
### Fixed

- Fixed a bug where `LevelFilter::OFF` (and thus also the
  `static_max_level_off`  feature flag) would enable *all* traces,
  rather than *none* (#853)
- **log**: Fixed `tracing` macros and `Span`s not checking
  `log::max_level` before emitting `log` records (#870)

### Changed

- **macros**: Macros now check the global max level
  (`LevelFilter::current`) before the per-callsite cache when
  determining if a span or event is enabled. This significantly improves
  performance in some use cases (#853)
- **macros**: Simplified the code generated by macro expansion
  significantly, which may improve compile times and/or `rustc`
  optimizatation of surrounding code (#869, #869)
- **macros**: Macros now check the static max level before checking any
  runtime filtering, improving performance when a span or event is
  disabled by a `static_max_level_XXX` feature flag (#868)
- `LevelFilter` is now a re-export of the `tracing_core::LevelFilter`
  type, it can now be used interchangably with the versions in
  `tracing-core` and `tracing-subscriber` (#853)
- Significant performance improvements when comparing `LevelFilter`s and
  `Level`s (#853)
- Updated the minimum `tracing-core` dependency to 0.1.12 (#853)

### Added

- **macros**: Quoted string literals may now be used as field names, to
  allow fields whose names are not valid Rust identifiers (#790)
- **docs**: Several documentation improvements (#850, #857, #841)
- `LevelFilter::current()` function, which returns the highest level
  that any subscriber will enable (#853)
- `Subscriber::max_level_hint` optional trait method, for setting the
  value returned by `LevelFilter::current()` (#853)

Thanks to new contributors @cuviper, @ethanboxx, @ben0x539, @dignati,
@colelawrence, and @rbtcollins for helping out with this release!

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Jul 31, 2020
@hawkw
trace: roll tracing and tracing-subscriber dependencies
732964b 
This picks up upstream changes tokio-rs/tracing#853,
tokio-rs/tracing#868, and tokio-rs/tracing#869 which improve performance
in some use cases. The overhead removed by these changes may already be
amortized enough in the proxy that it's not a problem, but it seems
worth picking up regardless.
@hawkw hawkw mentioned this pull request Jul 31, 2020
Merged
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Aug 4, 2020
@hawkw
trace: roll tracing and tracing-subscriber dependencies (#615)
342ab79 
This picks up upstream changes tokio-rs/tracing#853,
tokio-rs/tracing#868, and tokio-rs/tracing#869 which improve performance
in some use cases. The overhead removed by these changes may already be
amortized enough in the proxy that it's not a problem, but it seems
worth picking up regardless.


Signed-off-by: Eliza Weisman <[email protected]>
@jplatte jplatte deleted the eliza/check-static-first branch June 3, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky davidbarsky approved these changes

Assignees
No one assigned
Labels
crate/tracing Related to the `tracing` crate kind/perf Performance improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hawkw @davidbarsky