Improve the performance of CachedIdentifiersExtractor #1257
Conversation
|
hmm tests failed though |
|
I'm on it |
|
Could we not do the same for all |
| @@ -33,6 +33,7 @@ | |||
| private $cacheItemPool; | |||
| private $propertyAccessor; | |||
| private $decorated; | |||
| private $memoryCache = []; | |||
There was a problem hiding this comment.
Naming... See #1256 (comment)
dunglas
force-pushed
the
improve_cached_identifiers
branch
from
649d81d
to
4d4b5c4
Compare
|
I've tried something but I would like a review @soyuka. I'm not sure I respect your initial intent. |
dunglas
force-pushed
the
improve_cached_identifiers
branch
from
4d4b5c4
to
b68cbbf
Compare
| $cacheIsHit = false; | ||
| $keys = $this->getKeys($item, function ($item) use (&$identifiers) { | ||
| return $identifiers = $this->decorated->getIdentifiersFromItem($item); | ||
| }); |
There was a problem hiding this comment.
Using a reference to an unknown variable and declaring it in a closure looks cool to me haha 😅
|
@meyerbaptiste this one is slightly different and more complex then metadata classes. Initial PR #997 |
|
Yeah I agree we have a lot of repetition in those "cached" classes, finally it's always the same:
Do you have an idea to remove the duplicated code (only diff is the injected decorator and the cache key prefix)? |
|
Is it safe for us to use |
|
In the case of |
|
|
|
We can probably go one step further for this class: it should be possible to cache (in a memory cache) the results as well. |
| $this->assertEquals(['id' => 1], $identifiersExtractor->getIdentifiersFromItem($dummy)); | ||
| $expectedResult = ['id' => 1]; | ||
| $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy)); | ||
| $this->assertEquals($expectedResult, $identifiersExtractor->getIdentifiersFromItem($dummy), 'Trigger the local cache'); |
There was a problem hiding this comment.
How does this test verify that we are triggering the local cache? Also, it should not matter in a unit test.
There was a problem hiding this comment.
He's hacking coverage to please @Simperfit 😆
There was a problem hiding this comment.
Actually, this second call is crucial to test correct behaviour, i.e. that we return the correct value when cache is used. But it's not testing whether we're triggering the local cache.
There was a problem hiding this comment.
Oh wait, we already have different test cases for that... Lol
There was a problem hiding this comment.
Not really for the coverage. You're right @teohhanhui, it doesn't verify that the local cache actually works (it's very hard to test reliably), however it tests if the local cache doesn't introduce a bug.
For instance, if some one contribute this patch:
-return $this->memoryCache[$resourceClass];
+return $this->memoryCache[$resourceC];The test will detect the problem. Without this line, it will not.
There was a problem hiding this comment.
@dunglas Doesn't testSecondPass already do that?
There was a problem hiding this comment.
Good catch. testSecondPass should be update to bypass the local cache IMO (by creating a new instance).
There was a problem hiding this comment.
$localCache is unambiguous. #preach
|
@dunglas I don't think that caching values is worth it. Also cache invalidation will be an issue. This is already improving performances a lot because we avoid calling the whole metadataFactory thing from |
…tifiers Improve the performance of CachedIdentifiersExtractor
Follows #1256.