Skip to content

Commit

Permalink
The current ban regexp is not optimize and killed my varnish server t…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@bastnic
bastnic committed Apr 21, 2021
1 parent 8463285 commit 5d29b8c
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 33 deletions.
12 changes: 8 additions & 4 deletions src/HttpCache/VarnishPurger.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
final class VarnishPurger implements PurgerInterface
{
private const DEFAULT_VARNISH_MAX_HEADER_LENGTH = 8000;
private const REGEXP_PATTERN = '(%s)($|\,)';

private $clients;
private $maxHeaderLength;
Expand All @@ -35,7 +36,7 @@ final class VarnishPurger implements PurgerInterface
public function __construct(array $clients, int $maxHeaderLength = self::DEFAULT_VARNISH_MAX_HEADER_LENGTH)
{
$this->clients = $clients;
$this->maxHeaderLength = $maxHeaderLength;
$this->maxHeaderLength = $maxHeaderLength - mb_strlen(self::REGEXP_PATTERN) + 2; // 2 for %s
}

/**
Expand All @@ -60,8 +61,9 @@ private function determineTagsPerHeader(array $escapedTags, string $glue): int
* header length by the largest tag (minus the glue length)
*/
$tagsize = max(array_map('mb_strlen', $escapedTags));
$gluesize = \strlen($glue);

return (int) floor($this->maxHeaderLength / ($tagsize + \strlen($glue))) ?: 1;
return (int) floor(($this->maxHeaderLength + $gluesize) / ($tagsize + $gluesize)) ?: 1;
}

/**
Expand All @@ -85,10 +87,12 @@ private function purgeRequest(array $iris)
{
// Create the regex to purge all tags in just one request
$parts = array_map(static function ($iri) {
return sprintf('(^|\,)%s($|\,)', preg_quote($iri));
// here we should remove the prefix as it's not discriminent and cost a lot to compute
return preg_quote($iri);
}, $iris);

foreach ($this->chunkRegexParts($parts) as $regex) {
$regex = sprintf(self::REGEXP_PATTERN, $regex);
$this->banRegex($regex);
}
}
Expand All @@ -108,7 +112,7 @@ private function chunkRegexParts(array $parts): iterable
return;
}

$concatenatedParts = sprintf('(%s)', implode(")\n(", $parts));
$concatenatedParts = implode("\n", $parts);

if (\strlen($concatenatedParts) <= $this->maxHeaderLength) {
yield str_replace("\n", '|', $concatenatedParts);
Expand Down
57 changes: 28 additions & 29 deletions tests/HttpCache/VarnishPurgerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,26 @@ class VarnishPurgerTest extends TestCase
public function testPurge()
{
$clientProphecy1 = $this->prophesize(ClientInterface::class);
$clientProphecy1->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/foo($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy1->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '((^|\,)/foo($|\,))|((^|\,)/bar($|\,))']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy1->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo)($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy1->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo|/bar)($|\,)']])->willReturn(new Response())->shouldBeCalled();

$clientProphecy2 = $this->prophesize(ClientInterface::class);
$clientProphecy2->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/foo($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy2->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '((^|\,)/foo($|\,))|((^|\,)/bar($|\,))']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy2->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo)($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy2->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo|/bar)($|\,)']])->willReturn(new Response())->shouldBeCalled();

$clientProphecy3 = $this->prophesize(ClientInterface::class);
$clientProphecy3->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/foo($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy3->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/bar($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy3->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo)($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy3->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/bar)($|\,)']])->willReturn(new Response())->shouldBeCalled();

$clientProphecy4 = $this->prophesize(ClientInterface::class);
$clientProphecy4->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/foo($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy4->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(^|\,)/bar($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy4->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/foo)($|\,)']])->willReturn(new Response())->shouldBeCalled();
$clientProphecy4->request('BAN', '', ['headers' => ['ApiPlatform-Ban-Regex' => '(/bar)($|\,)']])->willReturn(new Response())->shouldBeCalled();

$purger = new VarnishPurger([$clientProphecy1->reveal(), $clientProphecy2->reveal()]);
$purger->purge(['/foo']);
$purger->purge(['/foo' => '/foo', '/bar' => '/bar']);

$purger = new VarnishPurger([$clientProphecy3->reveal(), $clientProphecy4->reveal()], 5);
$purger = new VarnishPurger([$clientProphecy3->reveal(), $clientProphecy4->reveal()], 12);
$purger->purge(['/foo' => '/foo', '/bar' => '/bar']);
}

Expand Down Expand Up @@ -118,58 +118,57 @@ public function provideChunkHeaderCases()
yield 'one iri' => [
50,
['/foo'],
['(^|\,)/foo($|\,)'],
['(/foo)($|\,)'],
];

yield 'few iris' => [
50,
['/foo', '/bar'],
['((^|\,)/foo($|\,))|((^|\,)/bar($|\,))'],
['(/foo|/bar)($|\,)'],
];

yield 'iris to generate a header with exactly the maximum length' => [
56,
22,
['/foo', '/bar', '/baz'],
['((^|\,)/foo($|\,))|((^|\,)/bar($|\,))|((^|\,)/baz($|\,))'],
['(/foo|/bar|/baz)($|\,)'],
];

yield 'iris to generate a header with exactly the maximum length and a smaller one' => [
37,
17,
['/foo', '/bar', '/baz'],
[
'((^|\,)/foo($|\,))|((^|\,)/bar($|\,))',
'(^|\,)/baz($|\,)',
'(/foo|/bar)($|\,)',
'(/baz)($|\,)',
],
];

yield 'with last iri too long to be part of the same header' => [
50,
35,
['/foo', '/bar', '/some-longer-tag'],
[
'((^|\,)/foo($|\,))|((^|\,)/bar($|\,))',
'(^|\,)/some\-longer\-tag($|\,)',
'(/foo|/bar)($|\,)',
'(/some\-longer\-tag)($|\,)',
],
];

yield 'iris to have five headers' => [
50,
25,
['/foo/1', '/foo/2', '/foo/3', '/foo/4', '/foo/5', '/foo/6', '/foo/7', '/foo/8', '/foo/9', '/foo/10'],
[
'((^|\,)/foo/1($|\,))|((^|\,)/foo/2($|\,))',
'((^|\,)/foo/3($|\,))|((^|\,)/foo/4($|\,))',
'((^|\,)/foo/5($|\,))|((^|\,)/foo/6($|\,))',
'((^|\,)/foo/7($|\,))|((^|\,)/foo/8($|\,))',
'((^|\,)/foo/9($|\,))|((^|\,)/foo/10($|\,))',
'(/foo/1|/foo/2)($|\,)',
'(/foo/3|/foo/4)($|\,)',
'(/foo/5|/foo/6)($|\,)',
'(/foo/7|/foo/8)($|\,)',
'(/foo/9|/foo/10)($|\,)',
],
];

yield 'with varnish default limit' => [
8000,
array_fill(0, 1000, '/foo'),
array_fill(0, 3000, '/foo'),
[
implode('|', array_fill(0, 421, '((^|\,)/foo($|\,))')),
implode('|', array_fill(0, 421, '((^|\,)/foo($|\,))')),
implode('|', array_fill(0, 158, '((^|\,)/foo($|\,))')),
sprintf('(%s)($|\,)', implode('|', array_fill(0, 1598, '/foo'))),
sprintf('(%s)($|\,)', implode('|', array_fill(0, 1402, '/foo'))),
],
];
}
Expand Down

0 comments on commit 5d29b8c

Please sign in to comment.