Problem/Motivation

ResourceTypeRepository::get is looping excessively over the list of resources exposed by the API. In my case I have ~120 entity types and not all of them are likely to be used but they are still iterated over. I am getting 50k+ calls to $resourceType->getEntityTypeId() and around 1k calls to $this->all() method.

Proposed resolution

Introduce a static cache that will have entity:bundle mapping over the exact resource in the list or keep a tomb-store record in case it's missing, so we spare any full list loops.

Remaining tasks

Discussion, patch, benchmarks.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

Title: ResourceTypeRepository » ResourceTypeRepository possible speed-up
Issue summary: View changes
Wim Leers’s picture

Title: ResourceTypeRepository possible speed-up » Optimize ResourceTypeRepository::get(): don't loop over all possible resource types in every call
Issue tags: +API-First Initiative

I've never liked that bit either. I'm +1 to your general proposal :)

Mind rolling a patch?

gabesullice’s picture

+1, BUT #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType may make this less consequential. Nonetheless, it's a small change that makes sense.

ndobromirov’s picture

Status: Active » Needs review
FileSize
1.22 KB
139.12 KB

Here is the patch and the after benchmarks (9-10 times faster).

gabesullice’s picture

@ndobromirov, can you reroll the patch to use a protected property, so it won't break when we do #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin.

gabesullice’s picture

Status: Needs review » Needs work
ndobromirov’s picture

Yeah but how will the cache invalidation happen based on cache tags on a private property?

Otherwise I am all for it. It's going to be even faster :)

e0ipso’s picture

@ndobromirov we should not need cache invalidation in this case, should we? Nothing that invalidates the cached entry should change mid-request.

ndobromirov’s picture

Here is the newly requested version :). No benchmarks - should be faster...

gabesullice’s picture

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -137,12 +144,25 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
+      // We might have FALSE in cache, so we need to convert it back to NULL.
+      return $this->cache[$cid] ?: NULL;
...
+    // Using FALSE, as Drupal cache API discards NULL values from cache.
+    $result = FALSE;
...
+    return $result ?: NULL;

I think this is now unnecessary? Otherwise, I think this looks good.

ndobromirov’s picture

It still is. We are checking with an isset() for cache presence.

Having a key that is with a NULL value is treated by PHP as not set, thus a cache miss. We want to have that cached as well, so we are not looping over all of the list to find that it's not in it again and again.

gabesullice’s picture

8.5 failed because it's broken on HEAD. I find the attached a little bit easier to reason about. It's just me being nitpicky. If tests pass and @ndobromirov is +1, I'll commit.

gabesullice’s picture

@e0ipso brought up the performance of array_key_exists vs. isset and we had this exchange:

Chat showing we agreed array_key_exists is satisfactory since in this instance it's O(1)

  • e0ipso committed 85b180e on 8.x-2.x authored by ndobromirov
    Issue #3017239 by ndobromirov, gabesullice, Wim Leers, e0ipso: Optimize...
e0ipso’s picture

Status: Needs review » Fixed

Committed and credited @ndobromirov, who is great and is helping a lot!

Wim Leers’s picture

🎉

gabesullice’s picture

👏 @ndobromirov

ndobromirov’s picture

It could be Isset() || array_key_exists() to handle both cases. I was trying to not have the function call at all.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.