[FrameworkBundle] Dont store cache misses on warmup #40645

Nyholm · 2021-03-30T22:05:01Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #38694
License	MIT
Doc PR	symfony/symfony-docs#15172

When we are warming the annotation cache, we are reading all annotation into an ArrayAdapter. When we are done we move the values to a PhpArrayAdapter to store them in a file.

@Seldaek found out that when you are using a custom constraint with a Symfony\Component\Validator\Constraints\Callback, there is a strange error like:

Can use "yield from" only with arrays and Traversables

That is because the Closure in the Symfony\Component\Validator\Constraints\Callback cannot be serialised and saved to cache. But since the ArrayAdapter is also storing misses as null, the null values are understood as real values.

When all values are moved to the PhpArrayAdapter and we ask the cache for a value (via Doctrine's CacheProvider), it will return null as a value instead of false as a cache miss. And null is not something one could "yield from".

src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/AnnotationsCacheWarmerTest.php

Nyholm · 2021-03-31T06:27:34Z

@carsonbot could you find me a reviewer please?

carsonbot · 2021-03-31T06:28:24Z

I'm sorry. I could not find any suitable reviewer.

Nyholm · 2021-03-31T06:29:10Z

Oh. I guess it was too long ago @dmaicher wrote this class =)

David, do you mind having a look at this PR?

Seldaek · 2021-03-31T06:34:22Z

Thanks for the quick fix :)

If I understand correctly though, this means using lambdas like that will prevent caching of the resolved config and it'll reload on every request needing it. I guess it's thus worth avoiding lambda callbacks..

Nyholm · 2021-03-31T06:36:13Z

Yes, that is correct.

Nyholm · 2021-03-31T07:27:38Z

I updated the docs with this observation: symfony/symfony-docs#15172

nicolas-grekas · 2021-03-31T08:04:49Z

Storing misses is an important part of the performance provided by system caches.
We should look for another solution if possible.

Nyholm · 2021-03-31T08:09:56Z

Yes, but these "misses" are interpreted as "hits". A "hit" with null is not the same as a miss. There should never be "null" stored in this cache.

I have two alternative solutions. The first would be to find a way to serialize a \Closure. The second one is to throw an exception on cache warmup when you fail to save a value in cache.

nicolas-grekas · 2021-03-31T08:49:35Z

The test case passes even without the change if I didn't mess up :)

Yes, but these "misses" are interpreted as "hits". A "hit" with null is not the same as a miss. There should never be "null" stored in this cache.

For system caches, aka the ones sourced by the code itself, we can do this - cache misses.
We built this logic because it saves CPU cycles, eg you don't want to read the annotations again on methods that don't have any annotation.

I have two alternative solutions. The first would be to find a way to serialize a \Closure. The second one is to throw an exception on cache warmup when you fail to save a value in cache.

Why a closure ends up in the cache? That's the main question I have here indeed. An annotation should not lead to a closure because then of course it cannot be cached - while the source is a plain descriptive annotation.

The second solution would be a major regression actually.

Can you try this patch instead? It tries to not store misses for failures-to-save().

--- a/src/Symfony/Component/Cache/Traits/ArrayTrait.php
+++ b/src/Symfony/Component/Cache/Traits/ArrayTrait.php
@@ -145,6 +145,7 @@ trait ArrayTrait
             try {
                 $serialized = serialize($value);
             } catch (\Exception $e) {
+                unset($this->values[$key]);
                 $type = \is_object($value) ? \get_class($value) : \gettype($value);
                 $message = sprintf('Failed to save key "{key}" of type %s: ', $type).$e->getMessage();
                 CacheItem::log($this->logger, $message, ['key' => $key, 'exception' => $e]);

Nyholm · 2021-03-31T08:58:35Z

The test case passes even without the change if I didn't mess up :)

You did mess up =)

❯ ./phpunit src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer 
#!/usr/bin/env php
PHPUnit 9.5.3 by Sebastian Bergmann and contributors.

Testing /src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer
....F..............                                               19 / 19 (100%)

Time: 00:00.091, Memory: 16.00 MB

There was 1 failure:

1) Symfony\Bundle\FrameworkBundle\Tests\CacheWarmer\AnnotationsCacheWarmerTest::testWarmupRemoveCacheMisses
Failed asserting that actual size 3 matches expected size 2.

/src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/AnnotationsCacheWarmerTest.php:149

Why a closure ends up in the cache?

Because we cache an object of Symfony\Component\Validator\Constraint\Callback. That object is given a closure by the user in its constructor.

Can you try this patch instead? It tries to not store misses for failures-to-save().

I think that would work. But the end result would be the same. We would reread the annotations at runtime.
Note that we still cache the empty array. Which means "we have read the annotations, and there weren't any".

nicolas-grekas · 2021-03-31T09:08:52Z

You did mess up =)

I confirm :P

Because we cache an object of Symfony\Component\Validator\Constraint\Callback. That object is given a closure by the user in its constructor.

I just read the doc PR, nice one.

I think that would work. But the end result would be the same. We would reread the annotations at runtime.

True, but we would still store misses, and this is important for the other system caches.

Note that we still cache the empty array. Which means "we have read the annotations, and there weren't any".

OK great, then I suppose other system caches need this. Annotations are not the only system caches, but the current patch changes the behavior for all of them.

… (Nyholm) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Validator] Add warning about closure not being cachable Follow up from symfony/symfony#40645 The annotation cache will not work with `Closure`. If I understand correctly, it is not possible to have external dependencies in your custom validator AND get your annotation cached. You have to chose one or the other. Commits ------- ebddb78 [Validator] Add warning about closure not being cachable

Nyholm · 2021-03-31T12:00:21Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php

+        $values = array_filter($values, function ($val) {
+            return $val !== null;
+        });
+
        // make sure we don't cache null values
-        parent::warmUpPhpArrayAdapter($phpArrayAdapter, array_filter($values));
+        parent::warmUpPhpArrayAdapter($phpArrayAdapter, $values);


array_filter($values) will also remove everything that is empty.

https://3v4l.org/0m3In

There is no test for this code and I am not sure why null should be removed. I can only assume it is because of the callback =)

Nyholm · 2021-03-31T12:04:06Z

I was wrong patching AbstractPhpFileCacheWarmer. What I really wanted to do is to only modify the behaviour of the AnnotationCacheWarmer.

Can you try this patch instead? It tries to not store misses for failures-to-save().

This is not reliable. If a user does something like this, the bug will occur again.

$i = $cache->getItem('foo');
$i->set('data');
$cache->save($i); // Save failed for some reason.

// ...
$cache->getItem('foo');

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php

Nyholm · 2021-03-31T19:18:52Z

The PR is updated

…pter (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Cache] skip storing failure-to-save as misses in ArrayAdapter | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #38694 | License | MIT | Doc PR | - In addition to #40645 Commits ------- ba66987 [Cache] skip storing failure-to-save as misses in ArrayAdapter

dmaicher · 2021-04-01T08:05:19Z

Oh. I guess it was too long ago @dmaicher wrote this class =)

David, do you mind having a look at this PR?

I think my work on that was related to #23558

But now I see you changed the PR anyway 😊

nicolas-grekas · 2021-04-01T10:18:01Z

Thank you @Nyholm.

carsonbot added Status: Needs Review Bug FrameworkBundle labels Mar 30, 2021

carsonbot added this to the 4.4 milestone Mar 30, 2021

Nyholm force-pushed the 4.4-annotation-cache-warmer branch from 512e889 to 3cec54f Compare March 30, 2021 22:31

Nyholm mentioned this pull request Mar 30, 2021

SF 5.2 doctrine/persistence #38694

Closed

Nyholm commented Mar 30, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/AnnotationsCacheWarmerTest.php Show resolved Hide resolved

Nyholm mentioned this pull request Mar 31, 2021

[Validator] Add warning about closure not being cachable symfony/symfony-docs#15172

Merged

Nyholm commented Mar 31, 2021

View reviewed changes

nicolas-grekas approved these changes Mar 31, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AnnotationsCacheWarmer.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 31, 2021

nicolas-grekas mentioned this pull request Mar 31, 2021

[Cache] skip storing failure-to-save as misses in ArrayAdapter #40655

Merged

chalasr approved these changes Apr 1, 2021

View reviewed changes

[FrameworkBundle] Dont store cache misses on warmup

27a22b3

nicolas-grekas force-pushed the 4.4-annotation-cache-warmer branch from d249500 to 27a22b3 Compare April 1, 2021 10:17

nicolas-grekas merged commit cc7d126 into symfony:4.4 Apr 1, 2021

Nyholm deleted the 4.4-annotation-cache-warmer branch April 6, 2021 09:47

This was referenced May 1, 2021

Release v4.4.22 #41017

Merged

Release v5.2.7 #41018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Dont store cache misses on warmup #40645

[FrameworkBundle] Dont store cache misses on warmup #40645

Nyholm commented Mar 30, 2021 •

edited

Loading

Nyholm commented Mar 31, 2021

carsonbot commented Mar 31, 2021

Nyholm commented Mar 31, 2021

Seldaek commented Mar 31, 2021

Nyholm commented Mar 31, 2021

Nyholm commented Mar 31, 2021

nicolas-grekas commented Mar 31, 2021 •

edited

Loading

Nyholm commented Mar 31, 2021 •

edited

Loading

nicolas-grekas commented Mar 31, 2021 •

edited

Loading

Nyholm commented Mar 31, 2021

nicolas-grekas commented Mar 31, 2021

Nyholm Mar 31, 2021

Nyholm commented Mar 31, 2021

Nyholm commented Mar 31, 2021

dmaicher commented Apr 1, 2021

nicolas-grekas commented Apr 1, 2021

[FrameworkBundle] Dont store cache misses on warmup #40645

[FrameworkBundle] Dont store cache misses on warmup #40645

Conversation

Nyholm commented Mar 30, 2021 • edited Loading

Nyholm commented Mar 31, 2021

carsonbot commented Mar 31, 2021

Nyholm commented Mar 31, 2021

Seldaek commented Mar 31, 2021

Nyholm commented Mar 31, 2021

Nyholm commented Mar 31, 2021

nicolas-grekas commented Mar 31, 2021 • edited Loading

Nyholm commented Mar 31, 2021 • edited Loading

nicolas-grekas commented Mar 31, 2021 • edited Loading

Nyholm commented Mar 31, 2021

nicolas-grekas commented Mar 31, 2021

Nyholm Mar 31, 2021

Choose a reason for hiding this comment

Nyholm commented Mar 31, 2021

Nyholm commented Mar 31, 2021

dmaicher commented Apr 1, 2021

nicolas-grekas commented Apr 1, 2021

Nyholm commented Mar 30, 2021 •

edited

Loading

nicolas-grekas commented Mar 31, 2021 •

edited

Loading

Nyholm commented Mar 31, 2021 •

edited

Loading

nicolas-grekas commented Mar 31, 2021 •

edited

Loading