From 1dac314c6772d29cd18f3927e959cee7bef26d8e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 15 Sep 2021 11:44:11 -0700 Subject: [PATCH] subscriber: don't use SmallVecs for filter fields (#1568) ## 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 --- tracing-subscriber/src/filter/directive.rs | 16 +++---- .../src/filter/env/directive.rs | 13 +++--- tracing-subscriber/src/filter/env/mod.rs | 34 +++++++++++--- tracing-subscriber/src/filter/targets.rs | 45 ++++++++++++++----- 4 files changed, 76 insertions(+), 32 deletions(-) diff --git a/tracing-subscriber/src/filter/directive.rs b/tracing-subscriber/src/filter/directive.rs index 61cc00a9ec..e95b999a97 100644 --- a/tracing-subscriber/src/filter/directive.rs +++ b/tracing-subscriber/src/filter/directive.rs @@ -13,14 +13,14 @@ pub struct ParseError { #[derive(Debug, PartialEq, Eq, Clone)] pub(crate) struct StaticDirective { pub(in crate::filter) target: Option, - pub(in crate::filter) field_names: FilterVec, + pub(in crate::filter) field_names: Vec, pub(in crate::filter) level: LevelFilter, } #[cfg(feature = "smallvec")] -pub(in crate::filter) type FilterVec = smallvec::SmallVec<[T; 8]>; +pub(crate) type FilterVec = smallvec::SmallVec<[T; 8]>; #[cfg(not(feature = "smallvec"))] -pub(in crate::filter) type FilterVec = Vec; +pub(crate) type FilterVec = Vec; #[derive(Debug, PartialEq, Clone)] pub(in crate::filter) struct DirectiveSet { @@ -129,7 +129,7 @@ impl DirectiveSet { impl StaticDirective { pub(in crate::filter) fn new( target: Option, - field_names: FilterVec, + field_names: Vec, level: LevelFilter, ) -> Self { Self { @@ -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, } } @@ -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` @@ -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(), }, }) } diff --git a/tracing-subscriber/src/filter/env/directive.rs b/tracing-subscriber/src/filter/env/directive.rs index 1cd3285696..fa281b06d0 100644 --- a/tracing-subscriber/src/filter/env/directive.rs +++ b/tracing-subscriber/src/filter/env/directive.rs @@ -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}, @@ -16,7 +15,7 @@ use tracing_core::{span, Level, Metadata}; #[cfg_attr(docsrs, doc(cfg(feature = "env-filter")))] pub struct Directive { in_span: Option, - fields: FilterVec, + fields: Vec, pub(crate) target: Option, pub(crate) level: LevelFilter, } @@ -216,12 +215,12 @@ impl FromStr for Directive { FIELD_FILTER_RE .find_iter(c.as_str()) .map(|c| c.as_str().parse()) - .collect::, _>>() + .collect::, _>>() }) - .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") @@ -244,7 +243,7 @@ impl Default for Directive { level: LevelFilter::OFF, target: None, in_span: None, - fields: FilterVec::new(), + fields: Vec::new(), } } } diff --git a/tracing-subscriber/src/filter/env/mod.rs b/tracing-subscriber/src/filter/env/mod.rs index f15ed13e23..3cf9a44350 100644 --- a/tracing-subscriber/src/filter/env/mod.rs +++ b/tracing-subscriber/src/filter/env/mod.rs @@ -118,11 +118,6 @@ thread_local! { type FieldMap = HashMap; -#[cfg(feature = "smallvec")] -type FilterVec = smallvec::SmallVec<[T; 8]>; -#[cfg(not(feature = "smallvec"))] -type FilterVec = Vec; - /// Indicates that an error occurred while parsing a `EnvFilter` from an /// environment variable. #[cfg_attr(docsrs, doc(cfg(all(feature = "env-filter", feature = "std"))))] @@ -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::().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", + ); + } } diff --git a/tracing-subscriber/src/filter/targets.rs b/tracing-subscriber/src/filter/targets.rs index 1bd16e8ee3..4d1bd6fd23 100644 --- a/tracing-subscriber/src/filter/targets.rs +++ b/tracing-subscriber/src/filter/targets.rs @@ -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::::default()); + assert_eq!(dirs[0].field_names, Vec::::new()); assert_eq!(dirs[1].target, Some("common".to_string())); assert_eq!(dirs[1].level, LevelFilter::INFO); - assert_eq!(dirs[1].field_names, FilterVec::::default()); + assert_eq!(dirs[1].field_names, Vec::::new()); } fn expect_parse_level_directives(s: &str) { @@ -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::::default()); + assert_eq!(dirs[0].field_names, Vec::::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::::default()); + assert_eq!(dirs[1].field_names, Vec::::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::::default()); + assert_eq!(dirs[2].field_names, Vec::::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::::default()); + assert_eq!(dirs[3].field_names, Vec::::new()); assert_eq!(dirs[4].target, Some("crate3".to_string())); assert_eq!(dirs[4].level, LevelFilter::TRACE); - assert_eq!(dirs[4].field_names, FilterVec::::default()); + assert_eq!(dirs[4].field_names, Vec::::new()); assert_eq!(dirs[5].target, Some("crate2".to_string())); assert_eq!(dirs[5].level, LevelFilter::DEBUG); - assert_eq!(dirs[5].field_names, FilterVec::::default()); + assert_eq!(dirs[5].field_names, Vec::::new()); } #[test] @@ -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::::default()); + assert_eq!(dirs[0].field_names, Vec::::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::::default()); + assert_eq!(dirs[1].field_names, Vec::::new()); assert_eq!(dirs[2].target, Some("crate3".to_string())); assert_eq!(dirs[2].level, LevelFilter::OFF); - assert_eq!(dirs[2].field_names, FilterVec::::default()); + assert_eq!(dirs[2].field_names, Vec::::new()); assert_eq!(dirs[3].target, Some("crate2".to_string())); assert_eq!(dirs[3].level, LevelFilter::DEBUG); - assert_eq!(dirs[3].field_names, FilterVec::::default()); + assert_eq!(dirs[3].field_names, Vec::::new()); } #[test] @@ -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::().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", + ); + } }