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 <[email protected]>

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<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> {
@@ -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 {
@@ -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(),
             },
         })
     }

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<String>,
-    fields: FilterVec<field::Match>,
+    fields: Vec<field::Match>,
     pub(crate) target: Option<String>,
     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::<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")
@@ -244,7 +243,7 @@ impl Default for Directive {
             level: LevelFilter::OFF,
             target: None,
             in_span: None,
-            fields: FilterVec::new(),
+            fields: Vec::new(),
         }
     }
 }

tracing-subscriber/src/filter/env/mod.rs

@@ -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"))))]
@@ -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",
+        );
+    }
 }

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::<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) {
@@ -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]
@@ -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]
@@ -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",
+        );
+    }
 }