subscriber: don't use SmallVecs for filter fields #1568
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added
kind/bug
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
force-pushed
the
eliza/unsmallerate-vecs
branch
from
1fc0552
to
d62b741
Compare
davidbarsky
approved these changes
Sep 15, 2021
hawkw
added a commit
that referenced
this pull request
Sep 16, 2021
# 0.2.23 (September 16, 2021) This release fixes a few bugs in the per-layer filtering API added in v0.2.21. ### Fixed - **env-filter**: Fixed excessive `EnvFilter` memory use ([#1568]) - **filter**: Fixed a panic that may occur in debug mode when using per-layer filters together with global filters ([#1569]) - Fixed incorrect documentation formatting ([#1572]) [#1568]: #1568 [#1569]: #1569 [#1572]: #1572
hawkw
added a commit
that referenced
this pull request
Sep 16, 2021
# 0.2.23 (September 16, 2021) This release fixes a few bugs in the per-layer filtering API added in v0.2.21. ### Fixed - **env-filter**: Fixed excessive `EnvFilter` memory use ([#1568]) - **filter**: Fixed a panic that may occur in debug mode when using per-layer filters together with global filters ([#1569]) - Fixed incorrect documentation formatting ([#1572]) [#1568]: #1568 [#1569]: #1569 [#1572]: #1572
davidbarsky
pushed a commit
that referenced
this pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
DirectiveSettype used inEnvFilterandTargetsusesSmallVecto store the filtering directives when theSmallVecfeatureis 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
SmallVecforstoring field filters. This was intended to make the same optimization
for field filters. However, it had unintended consequences: an empty
SmallVecis an array ofTitems (plus metadata), while an emptyVecis just a couple of words. Since most filters don't have fieldfilters, this meant that we were suddenly using a lot more space to
store...nothing. This made
EnvFilters much larger, causing problemsfor some users (see #1567).
Solution
This branch undoes the change to
SmallVecfor field name/valuefilters. This takes the size of an
EnvFilterfrom 5420 bytes back downto 1272 bytes.
I also added some tests that just print the size of various
EnvFilterandTargetsvalues. These don't make any assertions, but can be run fordevelopment purposes when making changes to these types.
Fixes #1567