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

subscriber: rm crossbeam-utils, make parking_lot opt-in #348

Merged
merged 6 commits into from Sep 13, 2019

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Sep 13, 2019

Motivation

The tracing-subscriber currently depends on both the crossbeam-util
crate (for its ShardedLock type) and the parking_lot crate for it's
RwLock implementation. The Rust standard library also provides a
RwLock implementation, so these external dependencies are not strictly
necessary. In the past, using these crates was anecdotally observed to
result in a performance improvement, but the tradeoff is adding
additonal crates (and their transitive dependencies) to the user's
dependency tree.

Solution

This branch removes the crossbeam-util dependency, and makes the
parking_lot dependency opt-in. The explicit use of
parking_lot::RwLock with a wrapper type that abstracts over the
parking_lot::RwLock and std::sync::RwLock types to provide a
matching API. This allows the parking_lot feature flag to
transparently replace the use of std::sync::RwLock with parking_lot,
rather than making it required for other features.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from jonhoo, tobz, yaahc and a team September 13, 2019 18:05
@hawkw hawkw self-assigned this Sep 13, 2019
@github-actions github-actions bot added the crate/subscriber Related to the `tracing-subscriber` crate label Sep 13, 2019
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Sep 13, 2019

Switching from crossbeam-util::ShardedLock to std::sync::RwLock in EnvFilter doesn't seem to have a noticeable impact on the quick benchmarks I added in ade986b --- the dynamic/multithreaded benchmark (which actually exercises locking in the filter) actually shows a slight performance improvement on this branch vs master (from 15us to 12us), though this may not be significant.

@hawkw hawkw merged commit 86946f6 into master Sep 13, 2019
@hawkw hawkw deleted the eliza/rm-subscriber-deps branch September 13, 2019 20:43
hawkw added a commit that referenced this pull request Sep 13, 2019
Fixed:

- `EnvFilter` ignoring directives with targets that are the same number
  of characters (#333)
- `EnvFilter` failing to properly apply filter directives to events
  generated from `log` records by`tracing-log` (#344)

Changed:

- Renamed `Filter` to `EnvFilter`, deprecated `Filter` (#339)
- Renamed "filter" feature flag to "env-filter", deprecated "filter" (#339)
- `FmtSubscriber` now defaults to enabling only the `INFO` level and
  above when a max level filter or `EnvFilter` is not set (#336)
- Made `parking_lot` dependency an opt-in feature flag (#348)

Added:

- `EnvFilter::add_directive` to add new directives to filters after they
  are constructed (#334)
- `fmt::Builder::with_max_level` to set a global level filter for a
  `FmtSubscriber` without requiring the use of `EnvFilter` (#336)
- `Layer` implementation for `LevelFilter` (#336)
- `EnvFilter` now implements `fmt::Display` (#329)

Removed:

- Removed dependency on `crossbeam-util` (#348)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants