Cache identifiers for better performance on getIriFromItem by soyuka · Pull Request #997 · api-platform/core

soyuka · 2017-03-17T16:21:43Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	Not yet (still experimenting)
Related tickets	#212
License	MIT
Doc PR	ø

Add `getIriFromItem` cache

The collection here has ~8k items, no relations. Dev mode active but caching with apcu is forced.

Blackfire before:

2.87s spent in getIriFromItem
2.35 s for getIdentifiersFromItem

Blackfire after:

898 ms spent in getIriFromItem
371 ms for getIdentifiersFromItem

When discussing my performance issue with @Simperfit we noticed that getIdentifiersFromItem was called a lot (previous method is getIriFromItem). This is called for every item in the collection.

Profiler:

Don't look at the overall time, only the time spent serializing :p.

before

after

In this PR, we cache the properties that are identifiers for the given class.

teohhanhui · 2017-03-20T07:05:10Z

I like the idea, but the implementation is wrong. We should decorate the IriConverter: CachedIriConverter.

soyuka · 2017-03-20T07:30:08Z

👍 indeed

dunglas · 2017-03-20T16:25:35Z

src/Bridge/Symfony/Routing/IriConverter.php

        $identifiers = [];
        $resourceClass = $this->getObjectClass($item);

+        $cacheKey = self::CACHE_KEY_PREFIX.md5(serialize([$resourceClass]));


Why not using $resourceClass directly as it's a string?

Indeed, copy paste you know 👯‍♂️

I made this in a hurry just to see if my first assumption was correct! I'll refactor this and decorate the IriConverter as suggested by @teohhanhui. Will be done this week!

soyuka · 2017-03-22T10:30:41Z

Okay so I did this with a decorator, but because what's slow is getIdentifiersFromItem (which is a private method) I'm not sure it's ideal. Indeed, I had to copy the code from IriConverter (trying to avoid BC break at the same time), therefore I see no point in decorating :/.

You can check the code to understand what I mean...

A solution would be to have a new service for getIdentifiersFromItem that we can cache without changing the IriConverter. Lmk if you've a better idea.

From my benchmarks, I know that we can still win a lot by caching the identifiers getters instead of calling the PropertyAccessor ($identifiers[$propertyName] = $item->{$accessor}();).

Some links (note that I'm using the normalizer dumper, 8000 items collection):

IriConverter without cache 8.36s
IriConverter with this PR cache 6.89s
IriConverter with direct getter instead of PropertyAccessor + cache 4.78s

teohhanhui · 2017-03-28T06:44:24Z

@soyuka Okay, since what we're caching is a private method, then decoration indeed makes no sense.

A solution would be to have a new service for getIdentifiersFromItem that we can cache without changing the IriConverter.

ItemIdentifiersResolverInterface? ItemIdentifiersExtractorInterface?

soyuka · 2017-03-28T07:17:10Z

ItemIdentifiersResolverInterface? Extract is a bit strong no?

Would get${ucfirst($propertyName)} be acceptable to retrieve identifiers instead of using the property accessor?

teohhanhui · 2017-03-28T07:21:47Z

I think "extract" describes better what we're doing here: extracting identifiers from an item.

Would get${ucfirst($propertyName)} be acceptable to retrieve identifiers instead of using the property accessor?

No, we cannot make such assumptions. Plus, it'd be a BC break.

dunglas · 2017-03-28T11:41:53Z

What we can do however is providing an implementation of the PropertyAccessorInterface doing that. I usually do this for perf in my project. Such class should be included directly in Symfony.

soyuka · 2017-03-28T12:08:48Z

@dunglas You mean having a kind of IdentifierPropertyAccessor witch would end up doing something simpler than the original class (still no assumption could be made regarding get{$attribute}) ? We also don't need setValue in this context.

dunglas · 2017-03-28T12:11:33Z

@soyuka yes, that you can register for your project if you stick to a particular convention (like public properties, or getters and setters). It prevents to trigger the introspection system, so it improves performances.

soyuka · 2017-03-28T12:15:36Z

Oh I'm doing this already on my project for this particular class :). So, I'll just add a ItemIdentifiersExtractorInterface so that identifiers can be cached here but I won't touch the propertyAccessor thing.

soyuka · 2017-03-29T14:12:57Z

ping @teohhanhui @dunglas

I tried to avoid breaking changes lmk what you think.
Note that Behat tests are green, just waiting for implementation approval to add tests (and #1014 comes first obviously).

dunglas · 2017-04-01T07:18:52Z

src/Bridge/Symfony/Bundle/Resources/config/api.xml

+            <argument type="service" id="api_platform.item_identifiers_extractor.cached" />
+        </service>
+
+        <service id="api_platform.item_identifiers_extractor" class="ApiPlatform\Core\Bridge\Symfony\Routing\ItemIdentifiersExtractor">


Can't those services be private?

dunglas · 2017-04-01T07:24:47Z

src/Bridge/Symfony/Routing/ItemIdentifiersExtractor.php

+use Symfony\Component\PropertyAccess\PropertyAccess;
+use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
+
+class ItemIdentifiersExtractor implements ItemIdentifiersExtractorInterface


Can you make it final and add some PHPDoc? Btw, why do this belong in the Symfony Bridge? It can be used in a more general context.

Can be moved under ApiPlatform\Core\Metadata\Property namespace then?

Simperfit · 2017-04-07T16:43:34Z

src/Bridge/Symfony/Routing/CachedItemIdentifiersExtractor.php

+    private $propertyAccessor;
+    private $decorated;
+
+    public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyAccessorInterface $propertyAccessor = null, ItemIdentifiersExtractorInterface $decorated = null)


should $decorated be null ?

Simperfit · 2017-04-07T16:44:45Z

src/Bridge/Symfony/Routing/ItemIdentifiersExtractor.php

+use Symfony\Component\PropertyAccess\PropertyAccess;
+use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
+
+class ItemIdentifiersExtractor implements ItemIdentifiersExtractorInterface


Simperfit · 2017-04-07T16:45:30Z

👍 Great work @soyuka

soyuka · 2017-04-12T14:25:32Z

src/Api/IdentifiersExtractor.php

+
+                if ($propertyMetadata->isIdentifier()) {
+                    if (isset($identifiers[$propertyName])) {
+                        throw new RuntimeException(sprintf('Composite identifiers not supported in "%s" through relation "%s" of "%s" used as identifier', $relatedResourceClass, $propertyName, $resourceClass));


@teohhanhui I'm having a hard time trying to add test coverage on this one. IMO this is not a valid case, but maybe you can give me some more context/informations about the Exception? Origin commit you authored.

soyuka · 2017-04-12T14:29:24Z

I did a great job covering this part as it wasn't tested anywhere (except through IriConverter, but not fully covered).

There's still a point to discuss: the location of the IdentifiersExtractor.

I'm not sure that it should actually be under ApiPlatform\Core\Metadata because it doesn't interfere with metadata, rather uses the metadata to extract identifiers. Therefore, IMHO this class should be in ApiPlatform\Core\Api (so is ResourceClassResolver for example). Let me know if you disagree we can always move it elsewhere :).

soyuka · 2017-04-20T15:22:28Z

All green ping @api-platform/core-team

Simperfit · 2017-04-20T16:04:53Z

@soyuka could you please update with master ?

dunglas · 2017-04-20T17:58:06Z

src/Api/IdentifiersExtractor.php

@@ -0,0 +1,88 @@
+<?php
+
+declare(strict_types=1);


missing blank line

dunglas · 2017-04-20T17:58:48Z

src/Api/CachedIdentifiersExtractor.php

@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);


Missing blank line

dunglas · 2017-04-20T17:59:20Z

src/Api/CachedIdentifiersExtractor.php

+    private $propertyAccessor;
+    private $decorated;
+
+    public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $decorated)


Maybe should you put $propertyAccessor as last parameter (because it is optional)

dunglas · 2017-04-20T17:59:58Z

src/Api/CachedIdentifiersExtractor.php

+            $cacheItem = $this->cacheItemPool->getItem($cacheKey);
+            $cacheIsHit = $cacheItem->isHit();
+
+            if ($cacheIsHit) {


Can be merged with the previous line

dunglas · 2017-04-20T18:02:09Z

src/Api/CachedIdentifiersExtractor.php

+    public function __construct(CacheItemPoolInterface $cacheItemPool, PropertyAccessorInterface $propertyAccessor = null, IdentifiersExtractorInterface $decorated)
+    {
+        $this->cacheItemPool = $cacheItemPool;
+        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();


$propertyAccessor ?? PropertyAccess::createPropertyAccessor();?

dunglas · 2017-04-20T18:03:56Z

src/Api/IdentifiersExtractorInterface.php

+interface IdentifiersExtractorInterface
+{
+    /**
+     * Find identifiers from an Item (Object).


dunglas · 2017-04-20T18:04:58Z

src/Exception/CacheException.php

@@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);


dunglas · 2017-04-20T18:05:13Z

tests/Api/CachedIdentifiersExtractorTest.php

@@ -0,0 +1,135 @@
+<?php
+
+declare(strict_types=1);


dunglas · 2017-04-20T18:05:53Z

src/Api/CachedIdentifiersExtractor.php

+                    $relatedCacheItem = $this->cacheItemPool->getItem($relatedCacheKey);
+
+                    if (!$relatedCacheItem->isHit()) {
+                        throw new CacheException('No relation cache item founded, we need more cache to continue.');


Can't we avoid to use exceptions? Exception handling is slow.

dunglas · 2017-04-20T18:06:46Z

tests/Api/IdentifiersExtractorTest.php

@@ -0,0 +1,111 @@
+<?php
+
+declare(strict_types=1);


soyuka · 2017-04-21T13:00:39Z

@dunglas this should be good now!

dunglas · 2017-04-21T13:16:11Z

src/Api/IdentifiersExtractor.php

+    {
+        $this->propertyNameCollectionFactory = $propertyNameCollectionFactory;
+        $this->propertyMetadataFactory = $propertyMetadataFactory;
+        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();


dunglas · 2017-04-21T13:16:49Z

src/Bridge/Symfony/Routing/IriConverter.php

        $this->itemDataProvider = $itemDataProvider;
        $this->routeNameResolver = $routeNameResolver;
        $this->router = $router;
        $this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();


Can you change this one too? ??

dunglas

👍 Feel free to merge.

…r resource

Cache identifiers for better performance on getIriFromItem

soyuka force-pushed the performance branch from 03f2937 to ce41266 Compare March 17, 2017 16:23

soyuka mentioned this pull request Mar 17, 2017

Let's talk about performances #212

Closed

dunglas reviewed Mar 20, 2017

View reviewed changes

soyuka added this to the 2.1.0 milestone Mar 22, 2017

soyuka force-pushed the performance branch 2 times, most recently from 1fcc90f to 6babac9 Compare March 22, 2017 10:10

soyuka changed the title ~~[wip] Performance improvements~~ [wip] Performance improvements on getIriFromItem Mar 22, 2017

soyuka force-pushed the performance branch from 6babac9 to f198a88 Compare March 29, 2017 14:07

soyuka changed the title ~~[wip] Performance improvements on getIriFromItem~~ Cache identifiers for better performance on getIriFromItem Mar 29, 2017

soyuka force-pushed the performance branch 2 times, most recently from 09aff4b to 63db69e Compare March 29, 2017 14:10

dunglas requested changes Apr 1, 2017

View reviewed changes

Simperfit reviewed Apr 7, 2017

View reviewed changes

soyuka force-pushed the performance branch from 63db69e to 1e3a433 Compare April 12, 2017 14:20

soyuka commented Apr 12, 2017

View reviewed changes

soyuka force-pushed the performance branch from 1e3a433 to 83079de Compare April 12, 2017 14:30

soyuka added the [RFR] Ready for Review label Apr 12, 2017

soyuka added the enhancement label Apr 12, 2017

Simperfit approved these changes Apr 13, 2017

View reviewed changes

soyuka requested review from dunglas and teohhanhui April 18, 2017 06:13

soyuka force-pushed the performance branch 2 times, most recently from 6a76d54 to 2d8c093 Compare April 20, 2017 15:01

soyuka force-pushed the performance branch from 2d8c093 to 36a3a7f Compare April 20, 2017 16:39

dunglas requested changes Apr 20, 2017

View reviewed changes

soyuka force-pushed the performance branch 2 times, most recently from b636643 to 56774b4 Compare April 20, 2017 18:58

dunglas reviewed Apr 21, 2017

View reviewed changes

dunglas approved these changes Apr 21, 2017

View reviewed changes

soyuka force-pushed the performance branch 2 times, most recently from b9cac9f to 5548232 Compare April 21, 2017 13:29

Implement ItemIdentifiersExtractor to cache identifiers properties pe…

fc1196b

…r resource

soyuka force-pushed the performance branch from 5548232 to fc1196b Compare April 21, 2017 14:54

soyuka merged commit f73309c into api-platform:master Apr 24, 2017

soyuka deleted the performance branch April 24, 2017 07:24

soyuka mentioned this pull request Jul 20, 2017

Improve the performance of CachedIdentifiersExtractor #1257

Merged

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#997 from soyuka/performance

a79ac97

Cache identifiers for better performance on getIriFromItem

		@@ -0,0 +1,88 @@
		<?php

		declare(strict_types=1);

		@@ -0,0 +1,101 @@
		<?php

		declare(strict_types=1);

		@@ -0,0 +1,19 @@
		<?php

		declare(strict_types=1);

		@@ -0,0 +1,135 @@
		<?php

		declare(strict_types=1);

Uh oh!

Conversation

soyuka commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add getIriFromItem cache

Profiler:

before

after

Uh oh!

teohhanhui commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Mar 28, 2017

Uh oh!

teohhanhui commented Mar 28, 2017

Uh oh!

dunglas commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soyuka commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Mar 28, 2017

Uh oh!

soyuka commented Mar 28, 2017

Uh oh!

soyuka commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Apr 12, 2017

Uh oh!

soyuka commented Apr 20, 2017

Uh oh!

Simperfit commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Mar 17, 2017 •

edited

Loading

Add `getIriFromItem` cache

teohhanhui commented Mar 20, 2017 •

edited

Loading

soyuka commented Mar 22, 2017 •

edited

Loading

teohhanhui commented Mar 28, 2017 •

edited

Loading

dunglas commented Mar 28, 2017 •

edited

Loading

soyuka commented Mar 28, 2017 •

edited

Loading