Skip to content

Improve performance of the metadata system#1256

Merged
dunglas merged 1 commit into
api-platform:2.0from
dunglas:cache
Jul 19, 2017
Merged

Improve performance of the metadata system#1256
dunglas merged 1 commit into
api-platform:2.0from
dunglas:cache

Conversation

@dunglas

@dunglas dunglas commented Jul 19, 2017

Copy link
Copy Markdown
Member
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

By default, the Symfony's cache.system setup uses apcu. Our metadata component relies on this cache, however, as we retrieve the cache many time in a single request apcu_fetch is called many times and do a lot of IO. This fix introduce a local memory cache to avoid the IO operations.


private function generateCacheKey(string $resourceClass = Dummy::class)
{
return CachedResourceMetadataFactory::CACHE_KEY_PREFIX.md5(serialize([$resourceClass]));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@teohhanhui you had a good reason to serialize here? (I think it comes from you)

Do we really need the md5 thing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is necessary because the key must not exceed 64 chars (PSR-6). A long class name may exceed this.

@soyuka

soyuka commented Jul 19, 2017

Copy link
Copy Markdown
Member

Nice one!
Is there no added value when doing the same on the NameCollectionFactories?

We may want to add the same thing to CachedIdentifiersExtractor, IIRC it was called a lot too. IMO any micro optimization is good here.

/edit: Maybe CachedIdentifiersExtractor is only available in master.

@dunglas

dunglas commented Jul 19, 2017

Copy link
Copy Markdown
Member Author

I missed NameCollectionFactory. I'll add it. I'll do it for the identifier too.

@dunglas
dunglas merged commit db7634b into api-platform:2.0 Jul 19, 2017
@dunglas
dunglas deleted the cache branch July 19, 2017 17:58

private $cacheItemPool;
private $decorated;
private $memoryCache = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$memoryCache is not a very descriptive name... 😞

Perhaps $localCache and $localCacheKey. Descriptive is better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@teohhanhui however it's exactly what it is, a memory cache.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

APCu is also a (shared) memory cache. This is the local memory cache. 😆

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The word memory is just redundant and meaningless since all the variables are in memory anyway...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How about loaded ?

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
Improve performance of the metadata system
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants