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.

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

subscriber: use Vec instead of BTreeSet in DirectiveSet #580

Merged
merged 3 commits into from
Feb 13, 2020
Merged

subscriber: use Vec instead of BTreeSet in DirectiveSet #580

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14ac8c4
kill btreeset
hawkw Feb 12, 2020
5f98ca4
Use binary_search
samschlegel Feb 12, 2020
64e275d
undo accidental change
samschlegel Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions tracing-subscriber/src/filter/env/directive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,7 @@ use super::super::level::{self, LevelFilter};
use super::{field, FieldMap, FilterVec};
use lazy_static::lazy_static;
use regex::Regex;
use std::{
cmp::Ordering,
collections::btree_set::{self, BTreeSet},
error::Error,
fmt,
iter::FromIterator,
str::FromStr,
};
use std::{cmp::Ordering, error::Error, fmt, iter::FromIterator, str::FromStr};
use tracing_core::{span, Metadata};

/// A single filtering directive.
Expand Down Expand Up @@ -45,7 +38,7 @@ pub(crate) type Statics = DirectiveSet<StaticDirective>;

#[derive(Debug, PartialEq)]
pub(crate) struct DirectiveSet<T> {
directives: BTreeSet<T>,
directives: Vec<T>,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we (eventually) want to introduce some kind of adaptive approach, where we use a vec up to a certain size, and switch to a set/map once we are larger than that size? Although, since we basically consume this data by iterating over it rather that via lookups, that probably doesn't make a difference.

Copy link
Member

Choose a reason for hiding this comment

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

actually, now that i think about it a bit, i think that if we want to go full niche data structures brain wizard, the ideal structure would be some kind of trie/prefix tree that's keyed by module path segments, so we don't have to iterate. but, even in that world, iteration is probably still faster if we have <10 directives.

anyway, just idle thoughts! this looks good for now!

max_level: LevelFilter,
}

Expand Down Expand Up @@ -134,7 +127,7 @@ impl Directive {
directives: impl IntoIterator<Item = Directive>,
) -> (Dynamics, Statics) {
// TODO(eliza): this could be made more efficient...
let (dyns, stats): (BTreeSet<Directive>, BTreeSet<Directive>) =
let (dyns, stats): (Vec<Directive>, Vec<Directive>) =
directives.into_iter().partition(Directive::is_dynamic);
let statics = stats
.into_iter()
Expand Down Expand Up @@ -391,15 +384,15 @@ impl<T> DirectiveSet<T> {
self.directives.is_empty()
}

pub(crate) fn iter(&self) -> btree_set::Iter<'_, T> {
pub(crate) fn iter(&self) -> std::slice::Iter<'_, T> {
self.directives.iter()
}
}

impl<T: Ord> Default for DirectiveSet<T> {
fn default() -> Self {
Self {
directives: BTreeSet::new(),
directives: Vec::new(),
max_level: LevelFilter::OFF,
}
}
Expand All @@ -420,7 +413,10 @@ impl<T: Match + Ord> DirectiveSet<T> {
if *level > self.max_level {
self.max_level = level.clone();
}
let _ = self.directives.replace(directive);
match self.directives.binary_search(&directive) {
Ok(i) => self.directives[i] = directive,
Err(i) => self.directives.insert(i, directive),
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions tracing-subscriber/src/registry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ use tracing_core::{field::FieldSet, span::Id, Metadata};
mod extensions;
#[cfg(feature = "registry")]
mod sharded;
#[cfg(feature = "registry")]
mod stack;
pub(crate) mod stack;
samschlegel marked this conversation as resolved.
Show resolved Hide resolved

pub use extensions::{Extensions, ExtensionsMut};
#[cfg(feature = "registry")]
Expand Down