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: use of SmallVec in EnvFilter makes them quite large #1567

Closed
hawkw opened this issue Sep 15, 2021 · 2 comments
Closed

subscriber: use of SmallVec in EnvFilter makes them quite large #1567

hawkw opened this issue Sep 15, 2021 · 2 comments
Assignees
Labels
kind/bug Something isn't working

Comments

@hawkw
Copy link
Member

hawkw commented Sep 15, 2021

@hawkw I think we are experiencing stack overflow on windows as a result of this update.
The previous size_of EnvFilter was 200, but now it is 5420.

It was hard, but we captured the backtrace on overflow on windows:

Details

  thread 'main' has overflowed its stack
  STACK OVERFLOW:
     0: backtrace::backtrace::dbghelp::trace
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.60\src\backtrace\dbghelp.rs:98
     1: backtrace::backtrace::trace_unsynchronized
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.60\src\backtrace\mod.rs:66
     2: backtrace::backtrace::trace<closure-0>
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.60\src\backtrace\mod.rs:53
     3: backtrace::capture::Backtrace::create
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.60\src\capture.rs:176
     4: backtrace::capture::Backtrace::new
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\backtrace-0.3.60\src\capture.rs:140
     5: build_script_build::report_overflow
               at build.rs:54
     6: build_script_build::vectored_handler
               at build.rs:47
     7: RtlInitializeCriticalSectionAndSpinCount
     8: RtlWalkFrameChain
     9: KiUserExceptionDispatcher
    10: alloc::vec::spec_from_iter_nested::{{impl}}::from_iter<tracing_subscriber::filter::env::directive::Directive,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<core::str::iter::Sp
lit<char>, closure-0>, tracing_subscriber::filter::directive::
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\alloc\src\vec\spec_from_iter_nested.rs:17
    11: alloc::vec::spec_from_iter::{{impl}}::from_iter<tracing_subscriber::filter::env::directive::Directive,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<core::str::iter::Split<cha
r>, closure-0>, tracing_subscriber::filter::directive::ParseEr
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\alloc\src\vec\spec_from_iter.rs:33
    12: alloc::vec::{{impl}}::from_iter<tracing_subscriber::filter::env::directive::Directive,core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<core::str::iter::Split<char>, closure-0>,
tracing_subscriber::filter::directive::ParseError>>
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\alloc\src\vec\mod.rs:2449
    13: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::ResultShunt<core::iter::adapters::map::Map<core::str::iter::Split<char>, closure-0>, tracing_subscriber::filter::directive:
:ParseError>,alloc::vec::Vec<tracing_subscriber::filter::env::
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\iter\traits\iterator.rs:1748
    14: core::result::{{impl}}::from_iter::{{closure}}<tracing_subscriber::filter::env::directive::Directive,tracing_subscriber::filter::directive::ParseError,alloc::vec::Vec<tracing_subscriber::filter
::env::directive::Directive, alloc::alloc::Global>,core::iter:
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\result.rs:1625
    15: core::iter::adapters::process_results<core::iter::adapters::map::Map<core::str::iter::Split<char>, closure-0>,tracing_subscriber::filter::env::directive::Directive,tracing_subscriber::filter::d
irective::ParseError,closure-0,alloc::vec::Vec<tracing_subscri
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\iter\adapters\mod.rs:145
    16: core::result::{{impl}}::from_iter<tracing_subscriber::filter::env::directive::Directive,tracing_subscriber::filter::directive::ParseError,alloc::vec::Vec<tracing_subscriber::filter::env::direct
ive::Directive, alloc::alloc::Global>,core::iter::adapters::ma
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\result.rs:1625
    17: core::iter::traits::iterator::Iterator::collect<core::iter::adapters::map::Map<core::str::iter::Split<char>, closure-0>,enum$<core::result::Result<alloc::vec::Vec<tracing_subscriber::filter::en
v::directive::Directive, alloc::alloc::Global>, tracing_subscr
               at /rustc/a178d0322ce20e33eac124758e837cbd80a6f633\library\core\src\iter\traits\iterator.rs:1748
    18: tracing_subscriber::filter::env::EnvFilter::try_new<alloc::string::String*>
               at C:\Users\Administrator\.cargo\registry\src\github.com-1ecc6299db9ec823\tracing-subscriber-0.2.22\src\filter\env\mod.rs:174

Not sure if this is a bug in particular, because opting out of smallvec feature fixes the problem, but still this has broken our builds =(

Originally posted by @Veetaha in #1550 (comment)

@hawkw hawkw self-assigned this Sep 15, 2021
@hawkw hawkw added the kind/bug Something isn't working label Sep 15, 2021
hawkw added a commit that referenced this issue Sep 15, 2021
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).

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.

Fixes #1567
hawkw added a commit that referenced this issue Sep 15, 2021
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).

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.

Fixes #1567
hawkw added a commit that referenced this issue Sep 15, 2021
## 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>
@hawkw
Copy link
Member Author

hawkw commented Sep 15, 2021

Closed by #1568.

@hawkw hawkw closed this as completed Sep 15, 2021
@Veetaha
Copy link
Contributor

Veetaha commented Sep 16, 2021

Wow, thanks for such a quick fix, that's pretty cool! =)

davidbarsky pushed a commit that referenced this issue Nov 29, 2021
## 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>
hawkw added a commit that referenced this issue Mar 23, 2022
## 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>
hawkw added a commit that referenced this issue Mar 23, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
hawkw added a commit that referenced this issue Mar 24, 2022
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants