Skip to content

Commit

Permalink
subscriber: don't use SmallVecs for filter fields (#1568)
Browse files Browse the repository at this point in the history
## Motivation

The `DirectiveSet` type used in `EnvFilter` and `Targets` uses
`SmallVec` to store the filtering directives when the `SmallVec` feature
is enabled. This is intended to improve the performance of iterating
over small sets of directives, by avoiding a heap pointer dereference.

PR #1550 changed the directives themselves to also use `SmallVec` for
storing _field_ filters. This was intended to make the same optimization
for field filters. However, it had unintended consequences: an empty
`SmallVec` is an array of `T` items (plus metadata), while an empty
`Vec` is just a couple of words. Since _most_ filters don't have field
filters, this meant that we were suddenly using a lot more space to
store...nothing. This made `EnvFilter`s _much_ larger, causing problems
for some users (see #1567).

## Solution

This branch undoes the change to `SmallVec` for field name/value
filters. This takes the size of an `EnvFilter` from 5420 bytes back down
to 1272 bytes.

I also added some tests that just print the size of various `EnvFilter` and
`Targets` values. These don't make any assertions, but can be run for 
development purposes when making changes to these types.

Fixes #1567

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
hawkw committed Mar 24, 2022
1 parent 10b0c91 commit 3accdce
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 32 deletions.
16 changes: 8 additions & 8 deletions tracing-subscriber/src/filter/directive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ pub struct ParseError {
#[derive(Debug, PartialEq, Eq, Clone)]
pub(crate) struct StaticDirective {
pub(in crate::filter) target: Option<String>,
pub(in crate::filter) field_names: FilterVec<String>,
pub(in crate::filter) field_names: Vec<String>,
pub(in crate::filter) level: LevelFilter,
}

#[cfg(feature = "smallvec")]
pub(in crate::filter) type FilterVec<T> = smallvec::SmallVec<[T; 8]>;
pub(crate) type FilterVec<T> = smallvec::SmallVec<[T; 8]>;
#[cfg(not(feature = "smallvec"))]
pub(in crate::filter) type FilterVec<T> = Vec<T>;
pub(crate) type FilterVec<T> = Vec<T>;

#[derive(Debug, PartialEq, Clone)]
pub(in crate::filter) struct DirectiveSet<T> {
Expand Down Expand Up @@ -129,7 +129,7 @@ impl DirectiveSet<StaticDirective> {
impl StaticDirective {
pub(in crate::filter) fn new(
target: Option<String>,
field_names: FilterVec<String>,
field_names: Vec<String>,
level: LevelFilter,
) -> Self {
Self {
Expand Down Expand Up @@ -221,7 +221,7 @@ impl Default for StaticDirective {
fn default() -> Self {
StaticDirective {
target: None,
field_names: FilterVec::new(),
field_names: Vec::new(),
level: LevelFilter::ERROR,
}
}
Expand Down Expand Up @@ -288,7 +288,7 @@ impl FromStr for StaticDirective {

let mut split = part0.split("[{");
let target = split.next().map(String::from);
let mut field_names = FilterVec::new();
let mut field_names = Vec::new();
// Directive includes fields:
// * `foo[{bar}]=trace`
// * `foo[{bar,baz}]=trace`
Expand Down Expand Up @@ -326,12 +326,12 @@ impl FromStr for StaticDirective {
Ok(level) => Self {
level,
target: None,
field_names: FilterVec::new(),
field_names: Vec::new(),
},
Err(_) => Self {
target: Some(String::from(part0)),
level: LevelFilter::TRACE,
field_names: FilterVec::new(),
field_names: Vec::new(),
},
})
}
Expand Down
13 changes: 6 additions & 7 deletions tracing-subscriber/src/filter/env/directive.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::FilterVec;
pub(crate) use crate::filter::directive::{ParseError, StaticDirective};
pub(crate) use crate::filter::directive::{FilterVec, ParseError, StaticDirective};
use crate::filter::{
directive::{DirectiveSet, Match},
env::{field, FieldMap},
Expand All @@ -16,7 +15,7 @@ use tracing_core::{span, Level, Metadata};
#[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))]
pub struct Directive {
in_span: Option<String>,
fields: FilterVec<field::Match>,
fields: Vec<field::Match>,
pub(crate) target: Option<String>,
pub(crate) level: LevelFilter,
}
Expand Down Expand Up @@ -216,12 +215,12 @@ impl FromStr for Directive {
FIELD_FILTER_RE
.find_iter(c.as_str())
.map(|c| c.as_str().parse())
.collect::<Result<FilterVec<_>, _>>()
.collect::<Result<Vec<_>, _>>()
})
.unwrap_or_else(|| Ok(FilterVec::new()));
.unwrap_or_else(|| Ok(Vec::new()));
Some((span, fields))
})
.unwrap_or_else(|| (None, Ok(FilterVec::new())));
.unwrap_or_else(|| (None, Ok(Vec::new())));

let level = caps
.name("level")
Expand All @@ -244,7 +243,7 @@ impl Default for Directive {
level: LevelFilter::OFF,
target: None,
in_span: None,
fields: FilterVec::new(),
fields: Vec::new(),
}
}
}
Expand Down
34 changes: 29 additions & 5 deletions tracing-subscriber/src/filter/env/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,6 @@ thread_local! {

type FieldMap<T> = HashMap<Field, T>;

#[cfg(feature = "smallvec")]
type FilterVec<T> = smallvec::SmallVec<[T; 8]>;
#[cfg(not(feature = "smallvec"))]
type FilterVec<T> = Vec<T>;

/// Indicates that an error occurred while parsing a `EnvFilter` from an
/// environment variable.
#[cfg_attr(docsrs, doc(cfg(all(feature = "env-filter", feature = "std"))))]
Expand Down Expand Up @@ -713,4 +708,33 @@ mod tests {
assert_eq!(f1.statics, f2.statics);
assert_eq!(f1.dynamics, f2.dynamics);
}

#[test]
fn size_of_filters() {
fn print_sz(s: &str) {
let filter = s.parse::<EnvFilter>().expect("filter should parse");
println!(
"size_of_val({:?})\n -> {}B",
s,
std::mem::size_of_val(&filter)
);
}

print_sz("info");

print_sz("foo=debug");

print_sz(
"crate1::mod1=error,crate1::mod2=warn,crate1::mod2::mod3=info,\
crate2=debug,crate3=trace,crate3::mod2::mod1=off",
);

print_sz("[span1{foo=1}]=error,[span2{bar=2 baz=false}],crate2[{quux=\"quuux\"}]=debug");

print_sz(
"crate1::mod1=error,crate1::mod2=warn,crate1::mod2::mod3=info,\
crate2=debug,crate3=trace,crate3::mod2::mod1=off,[span1{foo=1}]=error,\
[span2{bar=2 baz=false}],crate2[{quux=\"quuux\"}]=debug",
);
}
}
45 changes: 33 additions & 12 deletions tracing-subscriber/src/filter/targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,11 +361,11 @@ mod tests {
assert_eq!(dirs.len(), 2, "\nparsed: {:#?}", dirs);
assert_eq!(dirs[0].target, Some("server".to_string()));
assert_eq!(dirs[0].level, LevelFilter::DEBUG);
assert_eq!(dirs[0].field_names, FilterVec::<String>::default());
assert_eq!(dirs[0].field_names, Vec::<String>::new());

assert_eq!(dirs[1].target, Some("common".to_string()));
assert_eq!(dirs[1].level, LevelFilter::INFO);
assert_eq!(dirs[1].field_names, FilterVec::<String>::default());
assert_eq!(dirs[1].field_names, Vec::<String>::new());
}

fn expect_parse_level_directives(s: &str) {
Expand All @@ -374,27 +374,27 @@ mod tests {

assert_eq!(dirs[0].target, Some("crate3::mod2::mod1".to_string()));
assert_eq!(dirs[0].level, LevelFilter::OFF);
assert_eq!(dirs[0].field_names, FilterVec::<String>::default());
assert_eq!(dirs[0].field_names, Vec::<String>::new());

assert_eq!(dirs[1].target, Some("crate1::mod2::mod3".to_string()));
assert_eq!(dirs[1].level, LevelFilter::INFO);
assert_eq!(dirs[1].field_names, FilterVec::<String>::default());
assert_eq!(dirs[1].field_names, Vec::<String>::new());

assert_eq!(dirs[2].target, Some("crate1::mod2".to_string()));
assert_eq!(dirs[2].level, LevelFilter::WARN);
assert_eq!(dirs[2].field_names, FilterVec::<String>::default());
assert_eq!(dirs[2].field_names, Vec::<String>::new());

assert_eq!(dirs[3].target, Some("crate1::mod1".to_string()));
assert_eq!(dirs[3].level, LevelFilter::ERROR);
assert_eq!(dirs[3].field_names, FilterVec::<String>::default());
assert_eq!(dirs[3].field_names, Vec::<String>::new());

assert_eq!(dirs[4].target, Some("crate3".to_string()));
assert_eq!(dirs[4].level, LevelFilter::TRACE);
assert_eq!(dirs[4].field_names, FilterVec::<String>::default());
assert_eq!(dirs[4].field_names, Vec::<String>::new());

assert_eq!(dirs[5].target, Some("crate2".to_string()));
assert_eq!(dirs[5].level, LevelFilter::DEBUG);
assert_eq!(dirs[5].field_names, FilterVec::<String>::default());
assert_eq!(dirs[5].field_names, Vec::<String>::new());
}

#[test]
Expand All @@ -418,19 +418,19 @@ mod tests {
assert_eq!(dirs.len(), 4, "\nparsed: {:#?}", dirs);
assert_eq!(dirs[0].target, Some("crate1::mod2".to_string()));
assert_eq!(dirs[0].level, LevelFilter::TRACE);
assert_eq!(dirs[0].field_names, FilterVec::<String>::default());
assert_eq!(dirs[0].field_names, Vec::<String>::new());

assert_eq!(dirs[1].target, Some("crate1::mod1".to_string()));
assert_eq!(dirs[1].level, LevelFilter::ERROR);
assert_eq!(dirs[1].field_names, FilterVec::<String>::default());
assert_eq!(dirs[1].field_names, Vec::<String>::new());

assert_eq!(dirs[2].target, Some("crate3".to_string()));
assert_eq!(dirs[2].level, LevelFilter::OFF);
assert_eq!(dirs[2].field_names, FilterVec::<String>::default());
assert_eq!(dirs[2].field_names, Vec::<String>::new());

assert_eq!(dirs[3].target, Some("crate2".to_string()));
assert_eq!(dirs[3].level, LevelFilter::DEBUG);
assert_eq!(dirs[3].field_names, FilterVec::<String>::default());
assert_eq!(dirs[3].field_names, Vec::<String>::new());
}

#[test]
Expand All @@ -456,4 +456,25 @@ mod tests {
crate3=5,crate3::mod2::mod1=0",
)
}

#[test]
fn size_of_filters() {
fn print_sz(s: &str) {
let filter = s.parse::<Targets>().expect("filter should parse");
println!(
"size_of_val({:?})\n -> {}B",
s,
std::mem::size_of_val(&filter)
);
}

print_sz("info");

print_sz("foo=debug");

print_sz(
"crate1::mod1=error,crate1::mod2=warn,crate1::mod2::mod3=info,\
crate2=debug,crate3=trace,crate3::mod2::mod1=off",
);
}
}

0 comments on commit 3accdce

Please sign in to comment.