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.

Sign up for GitHub

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

Jump to bottom

Do more efficient matching of filter subjects in consumer #4864

Merged
merged 4 commits into from Dec 11, 2023
Merged

Do more efficient matching of filter subjects in consumer #4864

merged 4 commits into from Dec 11, 2023

Conversation

svenfoo
Copy link
Contributor

@svenfoo svenfoo commented Dec 8, 2023

Avoid tokenizing the message subject again for every filter subject that it is matched against. Also keep the tokenized version of the filter subjects in the subjectFilter struct.

Signed-off-by: Sven Neumann sven.neumann@holoplot.com

@svenfoo svenfoo requested a review from a team as a code owner December 8, 2023 09:56
@svenfoo
Copy link
Contributor Author

svenfoo commented Dec 8, 2023

Benchmark (from the first commit) before doing the actual changes:

goos: linux
goarch: amd64
pkg: github.com/nats-io/nats-server/v2/server
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
Benchmark____________ConsumerIsFilteredMatch/1_filter_subjects-8         	1000000000	         0.0000020 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/2_filter_subjects-8         	1000000000	         0.0000027 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/4_filter_subjects-8         	1000000000	         0.0000042 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/8_filter_subjects-8         	1000000000	         0.0000049 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/16_filter_subjects-8        	1000000000	         0.0000043 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/32_filter_subjects-8        	1000000000	         0.0000093 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/64_filter_subjects-8        	1000000000	         0.0000142 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/128_filter_subjects-8       	1000000000	         0.0000176 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/256_filter_subjects-8       	1000000000	         0.0000438 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/512_filter_subjects-8       	1000000000	         0.0000944 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/1024_filter_subjects-8      	1000000000	         0.0001221 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/nats-io/nats-server/v2/server	0.774s

After the changes:

goos: linux
goarch: amd64
pkg: github.com/nats-io/nats-server/v2/server
cpu: 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
Benchmark____________ConsumerIsFilteredMatch/1_filter_subjects-8         	1000000000	         0.0000030 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/2_filter_subjects-8         	1000000000	         0.0000025 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/4_filter_subjects-8         	1000000000	         0.0000029 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/8_filter_subjects-8         	1000000000	         0.0000033 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/16_filter_subjects-8        	1000000000	         0.0000038 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/32_filter_subjects-8        	1000000000	         0.0000046 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/64_filter_subjects-8        	1000000000	         0.0000058 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/128_filter_subjects-8       	1000000000	         0.0000102 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/256_filter_subjects-8       	1000000000	         0.0000256 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/512_filter_subjects-8       	1000000000	         0.0000330 ns/op	       0 B/op	       0 allocs/op
Benchmark____________ConsumerIsFilteredMatch/1024_filter_subjects-8      	1000000000	         0.0000626 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/nats-io/nats-server/v2/server	0.521s

@svenfoo
Copy link
Contributor Author

svenfoo commented Dec 8, 2023

We, at Holoplot, would like to see this merged as we would like to start using multiple filter subjects on consumers in our project. However we found that the performance degrades significantly with larger numbers of filter subjects.

@svenfoo
Copy link
Contributor Author

svenfoo commented Dec 8, 2023

I do not really embrace the way this is done, but I tried to follow the example of the existing code that does subject sub-matching.

I started working on a more holistic approach where I introduced a Subject type that holds a string and its tokenized form and then provides methods for matching etc. This quickly turned out to become a larger change, so I decided to try to get this much smaller change merged first. If there is interest I can clean up the work I've done so far and present it as a proposal for a refactoring that would potentially lead to some smaller performance wins.

derekcollison
derekcollison reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good! A couple of comments.

go.sum Outdated Show resolved Hide resolved
server/consumer.go Outdated Show resolved Hide resolved
server/consumer.go Outdated Show resolved Hide resolved
Jarema
Jarema reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice optimzation!

Just some small comments.

server/consumer_test.go Outdated Show resolved Hide resolved
server/consumer_test.go Outdated Show resolved Hide resolved
@svenfoo
Add test and benchmark for consumer.isFilteredMatch()
3b3e2ef 
Signed-off-by: Sven Neumann <sven.neumann@holoplot.com>
@svenfoo
Do more efficient matching of filter subjects in consumer
3786146 
Avoid tokenizing the message subject again for every filter
subject that it is matched against. Also keep the tokenized
version of the filter subjects in the subjectFilter struct.

Signed-off-by: Sven Neumann <sven.neumann@holoplot.com>
@svenfoo
Optimize stream.partitionUnique()
187a665 
Make use of the consumers' tokenized filter subjects.

Signed-off-by: Sven Neumann <sven.neumann@holoplot.com>
@svenfoo
Move newly added test and benchmark to jetstream_consumer_test.go
b8ac918 
Signed-off-by: Sven Neumann <sven.neumann@holoplot.com>
@svenfoo svenfoo mentioned this pull request Dec 11, 2023
Closed
Jarema
Jarema approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

derekcollison
derekcollison approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! - Thanks for the contribution!

@derekcollison derekcollison merged commit 601d1f5 into nats-io:main Dec 11, 2023
2 checks passed
@svenfoo svenfoo deleted the sven/consumer branch December 11, 2023 16:46
@wallyqs wallyqs mentioned this pull request Dec 11, 2023
Merged
wallyqs added a commit that referenced this pull request Dec 11, 2023
@wallyqs
Release v2.10.8 cherry picks (#4874)
064b7b2 
Includes the following changes:

- #4861
- #4862
- #4870
- #4869
- #4864
- #4868
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

@Jarema Jarema Jarema approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@svenfoo @derekcollison @Jarema