Optimize varnish ban regexp #4231
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
bastnic
changed the title
bastnic
force-pushed
the
feature/perf-regexp-ban
branch
3 times, most recently
from
61ba6e7
to
9d29b58
Compare
| ]; | ||
|
|
||
| yield 'iris to generate a header with exactly the maximum length and a smaller one' => [ | ||
| 37, | ||
| ['/foo', '/bar', '/baz'], | ||
| [ | ||
| '((^|\,)/foo($|\,))|((^|\,)/bar($|\,))', | ||
| '(^|\,)/baz($|\,)', | ||
| '(/foo|/bar)($|\,)', |
There was a problem hiding this comment.
I think something needs to be changed in the splitting logic: the 2 segments would now fit the 37 characters limit if merged.
There was a problem hiding this comment.
hi @julienfalque, I'm fixing tests right now.
bastnic
force-pushed
the
feature/perf-regexp-ban
branch
4 times, most recently
from
3f5ab8a
to
483632f
Compare
julienfalque
approved these changes
Apr 21, 2021
dunglas
approved these changes
Apr 21, 2021
bastnic
force-pushed
the
feature/perf-regexp-ban
branch
2 times, most recently
from
9a52951
to
5d29b8c
Compare
|
@dunglas FYI xkey is already abandoned by varnish which prefers ykey https://docs.varnish-software.com/varnish-cache-plus/vmods/ykey/. Migrating to x/ykey is not "free" as you need the paid version of varnish (maybe I'm wrong on this one). This PR an optimization for what we already provide in apip. |
|
xkey still works with the free varnish and won't be removed soon |
soyuka
approved these changes
Apr 25, 2021
…o 100% cpu. Several reasons, but let's explain how that work. This is a question of > rq/s * ban/s * size of the cache tags header * complexity of the regexp I've a high number of requests and bans per seconds. The response has a lot of tags. But I only ban one ressource at a time, so the regexp was not that complex at first look. Varnish stores all ban in a (deduplicated) list. Each cached resource has a pointer to the last ban checked. If you add bans, when you call a cached resources, it will check against all the new bans and store the last ones. The number of regexp analysis was high, but I can change my traffic, so I can only adjust the header itself or the regexp. I looked at the regexp first > ((^|,)\/pim\/v1\/variants($|,))|((^|,)\/pim\/v1\/variants\/e3615f0c-a957-4a85-88b9-8db30d5c5c02($|,))|((^|,)\/pim\/v1\/models\/bebdee7b-fb53-448b-b79e-1bcf0dbcf050($|,)) Step 1: checking the begining of the resources is not useful if tags are only handled by apip. > (\/pim\/v1\/variants($|,))|(\/pim\/v1\/variants\/e3615f0c-a957-4a85-88b9-8db30d5c5c02($|,))|(\/pim\/v1\/models\/bebdee7b-fb53-448b-b79e-1bcf0dbcf050($|,)) Stepe 2: regrouping the end tag at the end is better. > (\/pim\/v1/variants|\/pim\/v1/variants\/e3615f0c-a957-4a85-88b9-8db30d5c5c02|\/pim\/v1/models\/bebdee7b-fb53-448b-b79e-1bcf0dbcf050)($|,) Step 3: remove the prefix from the regexp > (/variants|\/variants\/e3615f0c-a957-4a85-88b9-8db30d5c5c02|\/models\/bebdee7b-fb53-448b-b79e-1bcf0dbcf050)($|,) Step 4: remove the prefix from the source header Bench: +--------------+---------+-------+ | subject | mean | diff | +--------------+---------+-------+ | benchCurrent | 1.612μs | 7.91x | | benchStep1 | 0.575μs | 2.82x | | benchStep2 | 0.504μs | 2.47x | | benchStep3 | 0.492μs | 2.41x | | benchStep4 | 0.204μs | 1.00x | +--------------+---------+-------+ This PR only goes to step 2.
soyuka
force-pushed
the
feature/perf-regexp-ban
branch
from
5d29b8c
to
c443491
Compare
|
thanks @bastnic |
|
Yes xkey isn't maintained by Varnish.com (the company) anymore but is still maintained by the community as part of the Varnish modules project (I recently contributed to it and the patch has been merged). |
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.
The current ban regexp is not optimize and killed my varnish server to 100% cpu.
Several reasons, but let's explain how that work. This is a question of
I've a high number of requests and bans per seconds. The response has a lot of tags.
But I only ban one resource at a time, so the regexp was not that complex at first look.
Varnish stores all ban in a (deduplicated) list. Each cached resource has a pointer to the last ban checked.
If you add bans, when you call a cached resources, it will check against all the new bans and store the last ones.
The number of regexp analysis was high, but I can change my traffic, so I can only adjust the header itself or the regexp.
I looked at the regexp first
Step 1: checking the begining of the resources is not useful if tags are only handled by apip.
Stepe 2: regrouping the end tag at the end is better.
Step 3: remove the prefix from the regexp
Step 4: remove the prefix from the source header
Bench:
This PR only goes to step 2.
On my production, I published Step3 today at 12:15: