ResourceTypeRepository computes ResourceType value objects on *every request*

160ms to calculate the relatable resource types only once? That seems excessive.

Can you share the insight of calculateRelatableResourceTypes? Maybe even a blackfire trace we can drill down on?

Ugh. Even though I don't like the extra complexity this brings in the numbers make a good point. NOTE: I do not think this will be a problem in non-paragraph based data models.

I'm on the fence on this. This goes against the simplification we hoped to introduce in #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin..

1) Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead
Undefined index: meta

That's not failing in HEAD, so it seems like a legitimate failure. OTOH, it's pretty strange that a small abstract change like this causes such a specific failure. I'll try to reproduce.

Reproduced. The failure is due to the persistent cache not being invalidated after the jsonapi_test_collection_count is installed.

Which brings us to another point: JSON:API Extras configuration changes need to invalidate this cache. \Drupal\jsonapi_extras\EventSubscriber\ConfigSubscriber::onResponse() is adding the 'config:jsonapi_extras.settings' and 'config:jsonapi_resource_config_list' cache tags, but they'll also need to be associated when the ResourceType objects are cached that are generated by \Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository.

IOW: this needs more test coverage, and needs manual testing with JSON:API Extras. It potentially also needs changes in JSON:API Extras. We don't want to break JSON:API Extras during the 2.0 RCs.

I'm very glad to see that the work we did on making things more isolated, including making ResourceType value objects actual value objects (and no longer call any services), are paying off. This sort of optimization is exactly what I had in mind while we were working on that 😀

Yes it would. At least for jsonapi. But jsonapi_extras 3.x doesn't use those cache tags, so you'd still need manual testing. Or … better yet, expand the automated tests of JSON:API Extras :)

This LGTM. Just needs to fix the CS violation:

FILE: ...napi_test_collection_count/jsonapi_test_collection_count.install
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Agreed that this is looking close!

1. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -70,6 +70,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +   * List of default cache tags added by the repository.
   

   Cache tags used for caching the repository.

2. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -70,6 +70,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +   * @var array
   

   Nit: string[]

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -70,6 +70,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +  protected $defaultCacheTags = ['jsonapi_resource_types'];
   

   s/$defaultCacheTags/$cacheTags/

4. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -107,7 +114,7 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   -      $this->staticCache->set('jsonapi.resource_types', $resource_types, Cache::PERMANENT, ['jsonapi_resource_types']);
   +      $this->staticCache->set('jsonapi.resource_types', $resource_types, Cache::PERMANENT, $this->defaultCacheTags);
   
   +++ b/tests/modules/jsonapi_test_collection_count/jsonapi_test_collection_count.install
   @@ -0,0 +1,8 @@
   +  \Drupal::service('cache_tags.invalidator')->invalidateTags(['jsonapi_resource_types']);
   

   Alternatively, we could remove this hook and instead do Cache::mergeTags($this->cacheTags, ['config:core.extension']), then it'd be invalidated whenever a module is installed, which seems the more prudent approach anyway :)

Or … better yet, expand the automated tests of JSON:API Extras :)

Please do!

This is looking much better now! 😀

+++ b/jsonapi.services.yml
@@ -142,9 +142,17 @@ services:
+  cache.jsonapi_resource_types:
+    class: Drupal\Core\Cache\BackendChain
+    arguments: ['default']
+    calls:
+      - ['appendBackend', ['@cache.jsonapi_memory']]
+      - ['appendBackend', ['@cache.default']]

This is looking quite complex. In fact, this is the first time I've ever seen this. I understand how this works, but how do we know we absolutely need this? Are we concerned about repeated unserialization? Needs profiling to prove we really need both layers.

Are we concerned about repeated unserialization? Needs profiling to prove we really need both layers.

We are. In fact this proved to be a problem already with the previous cache.static backend.

Thanks for confirming.

1. +++ b/jsonapi.services.yml
   @@ -142,9 +142,17 @@ services:
   +  cache.jsonapi_resource_types:
   

   I think we can mark this private too.

2. +++ b/jsonapi.services.yml
   @@ -142,9 +142,17 @@ services:
   +    arguments: ['default']
   ...
   +    # We need this, so Drupal's cache_tags.invalidator service knows about it.
        # This way it can invalidate the data in here based on tags.
        tags: [{ name: cache.bin }]
   

   And rather than repeating this, I wanted to inherit it using parent: cache.default, but that unfortunately fails; it somehow insists on inheriting the class too then.

3. +++ b/jsonapi.services.yml
   @@ -142,9 +142,17 @@ services:
   +    arguments: ['default']
   

   This doesn't quite make sense, this is not the default bin, but the jsonapi_resource_types bin. Due to chaining, it just happens to reuse some of it.

4. +++ b/jsonapi.services.yml
   @@ -142,9 +142,17 @@ services:
   +    # We need this, so Drupal's cache_tags.invalidator service knows about it.
        # This way it can invalidate the data in here based on tags.
   

   This comment can now go away, because this is now clearly a cache bin, whereas before, when this was just using the MemoryCache class, that was not obvious.

5. Most importantly: this could use a comment documenting the rationale.

Addressed all that.

+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -107,7 +114,11 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
+      $cache_tags = Cache::mergeTags($this->cacheTags, ['config:core.extension']);

We should also be invalidating whenever the set of fields is modified through for example adding a new configurable field (the entity_field_info cache tag) and/or when the set of entity types changes (the entity_types cache tag).

At least the former can easily be tested by adding a new required field with a default value; this should immediately show up.

Right now, this patch's more aggressive caching is breaking that. That needs test coverage!

#43 looks good!

This test should do it.

It will fail because there's still a class property used as a cache that's not cleared with the ->all() cache is cleared.

I'd like to see a patch for #3019729: Invalidate JSON:API Extras' cache tags when needed before merging this. Otherwise JSON:API Extras will break, people will get impatient and I'll have the pressure to fix it quick-quick-quick.

Thanks @ndobromirov!

Since we're changing ResourceTypeRepository, it's a good opportunity to clean up a little. So, don't mind me. Your patch looked good and I haven't fundamentally changed any of it.

I won't commit this till #3019729: Invalidate JSON:API Extras' cache tags when needed lands, per @e0ipso.

1. +++ b/README.md
   @@ -1 +1,2 @@
   -Please read the [jsonapi.api.php](jsonapi.api.php) file and the online [JSON API](https://www.drupal.org/docs/8/modules/json-api) documentation.
   +Please read the [jsonapi.api.php](jsonapi.api.php) file and the online
   +[JSON API](https://www.drupal.org/docs/8/modules/json-api) documentation.
   

   Let's not change this.

2. +++ b/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
   @@ -97,4 +98,13 @@ class ResourceTypeRepositoryTest extends KernelTestBase {
   +  public function testCaching() {
   +    $this->assertEmpty($this->resourceTypeRepository->get('node', 'article')->getRelatableResourceTypesByField('field_relationship'));
   +    $this->createEntityReferenceField('node', 'article', 'field_relationship', 'Related entity', 'node');
   +    $this->assertCount(2, $this->resourceTypeRepository->get('node', 'article')->getRelatableResourceTypesByField('field_relationship'));
   +  }
   

   I asked for test coverage in #44.

   This tests that. So removing the tag.

3. +++ b/jsonapi.services.yml
   @@ -142,11 +142,23 @@ services:
   +    private: true
   ...
   +    private: true
   

   These don't have any effect; it needs to be public: false. Yes, this is incredibly bad DX on Symfony's behalf…

#3019729: Invalidate JSON:API Extras' cache tags when needed was blocked on #3020237: [Regression] Broken with latest jsonapi 2.0-rc3. #3020237 landed yesterday. I just re-RTBC'd #3019729. Soon this will be RTBC too.

#3019729: Invalidate JSON:API Extras' cache tags when needed landed.

It conflicted with #2992833: Add a version negotiation to revisionable resource types, which added a hunk that this patch was also adding. So this patch became slightly smaller & simpler :)

I'm very sorry for the silence here, @ndobromirov! 😞

Do we still need this with #3014232-62: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources committed? The issue summary is no longer accurate.

#38 + #39 + #40 say we need the chained cache (with first static caching and then persistent caching) to avoid the cost of repeated unserialization. Is that still true in HEAD? (Since December, we refactored away a lot of places where the resource type repository is accessed repeatedly.)

#61 shows this makes things slower?

1. +++ b/jsonapi.services.yml
   @@ -144,10 +144,21 @@ services:
   +    class: Drupal\Core\Cache\BackendChain
   +    arguments: ['jsonapi_resource_types']
   +    calls:
   +      - ['appendBackend', ['@cache.jsonapi_memory']]
   +      - ['appendBackend', ['@cache.default']]
   

   Why not a regular cache.backend.chainedfast instead?

2. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -70,6 +70,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +  protected $cacheTags = ['jsonapi_resource_types'];
   

   Why only these here? You add some additional below. Any reason to have some, but not all here?

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -97,16 +104,32 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +        // Any module has the capability to expose new entities or alter
   +        // existing ones in multiple ways. So we merge 'config:core.extension'
   +        // tag to clear this data on any module install.
   +        'config:core.extension',
   

   If a module does that it should automatically clear the necessary tags by calling the save action on field / entity definitions. Right?

4. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -97,16 +104,32 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +      $this->staticCache->set('jsonapi.resource_types', $resource_types, Cache::PERMANENT, $cache_tags);
   

   I'm not 100% sure we are getting the cache tags rights here. It's my own limited knowledge of the Entity API system's fault. Maybe we can get someone to confirm this looks right?

5. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -143,20 +166,9 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    $key = "$entity_type_id:$bundle";
   

   Let's keep the $cid as it was before. This way we only change the relevant bits.

6. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -410,11 +419,17 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    $cid = "jsonapi:is_reference:{$field_definition->getType()}";
   +    if (isset($this->cache[$cid])) {
   +      return $this->cache[$cid];
   +    }
   

   This seems like an unrelated change. However I'm +1. One thing, did you find this to be a performance bottleneck?

Thanks for looping back in #79! I'm looking forward to get this in. I'd love to hear about what you find. Thanks for collaborating SO much on these issues. Your skills and expertise are deeply appreciated. Please try to not get discouraged by lack of progress.

Your skills and expertise are deeply appreciated. Please try to not get discouraged by lack of progress.

Amen! Thank you @ndobromirov! I suspect this will get some new life in the next few weeks. The 8.7 release and commit freeze + DrupalCon + a maintainer (me) being on paternity leave meant that the pace of the JSON:API issue queue was slowed down. I hope it will pick up again soon.

#2819335: Resource (entity) normalization should use partial caching landed, let's move this forward again! This is the next big performance win.

• I raised questions in my last comment (#70), and I see @ndobromirov answered them all 👍
• The profiling done in #73 is super valuable and very convincing 👏
• #75 indicates this is in production on 10 sites 🥳
• @e0ipso did an excellent review in #76, which @ndobromirov addressed.
• #76.1: to add to what @ndobromirov already wrote: APCu has extremely limited storage space. We should not add more to it, that'll slow Drupal overall down, since bootstrap-critical caches then won't have enough room in APCu anymore. This explains the counterintuitive results in #83.
• #76.4: see below.

1. Removed some unnecessary whitespace changes to keep the patch scope smaller.
2. I agree with @e0ipso in #76.2 that it's weird to have some cache tags in a class property and others elsewhere, if all of them are the same always anyway. So moved all of them there.

   (I also think it makes sense to have this live at the class level rather than in code dealing with implementation details, because it ensures cache tags are not dynamically computed, which is something this further communicates.)

3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -101,16 +108,32 @@ public function all() {
   +        // Any module has the capability to expose new entities or alter
   +        // existing ones in multiple ways. So we merge 'config:core.extension'
   +        // tag to clear this data on any module install.
   +        'config:core.extension',
   

   I know I suggested this in #34, but this was at a time where entity_field_info and entity_types were still absent. It's safe to remove these now.

   It's easy to convince yourself of this: look at the DB row for these cache tags in the cachetags table, then install for example the Actions module, and observe both cache tags incrementing.

   Removed. ✅

4. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -101,16 +108,32 @@ public function all() {
   +        // Clear the data on any field change in the system.
   +        'entity_field_info',
   +        // Clear the data on any entity type change in the system.
   +        'entity_types',
   

   These are used by \Drupal\Core\Entity\EntityDisplayRepository::getAllDisplayModesByEntityType() and \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions(), so 👍

   To be safe, we should add entity_bundles too though, which is used by \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo(). Done. ✅ Also expanded the test coverage for this.

5. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -74,6 +74,13 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
      protected $cache = [];
   

   This was introduced by #3017239: Optimize ResourceTypeRepository::get(): don't loop over all possible resource types in every call but is now no longer necessary, thanks to ::all():
   A) returning a result faster (thanks to using caching itself)
   B) that result now has keys that allow a faster checking for a match

   Continuing on this observation:

   +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -414,11 +423,17 @@ protected function getRelatableResourceTypesFromFieldDefinition(FieldDefinitionI
   +    $cid = "jsonapi:is_reference:{$field_definition->getType()}";
   +    if (isset($this->cache[$cid])) {
   +      return $this->cache[$cid];
   +    }
   

   This is now the only place where $this->cache is used. And this is one of the exceedingly rare cases where an unclearable static cache is actually okay. Because the result depends on a lot of loading of PHP objects and then checking if the final object in the chain is an instance of a particular class.

   So: it's fine to remove $this->cache. Done. ✅

6. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -147,20 +170,9 @@ public function get($entity_type_id, $bundle) {
   -    return $this->cache[$cid];
   ...
   +    $resources = $this->all();
   +    return isset($resources[$cid]) ? $resources[$cid] : NULL;
   

   This change means it's switching from its own static cache to not having any caching and instead relying on ::all() itself being much faster (thanks to caching). 👍

   Continuing on this observation, let's look at the lines next to it:

   +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -147,20 +170,9 @@ public function get($entity_type_id, $bundle) {
   +    $cid = "$entity_type_id:$bundle";
   
   @@ -395,11 +407,8 @@ protected function getRelatableResourceTypesFromFieldDefinition(FieldDefinitionI
   +      $cid = "$entity_type_id:$target_bundle";
   

   In both of these cases, this is not a CID but a key as returned by ResourceTypeRepository::all(). Because in both cases, making the call to ::all() return faster (thanks to caching) is just one aspect of the performance boost; the other aspect is removing the need to iterate over all returned resource types to find a match. Finding a match efficiently wasn't possible before, but it is now, thanks to this key.

   So renamed $cid to $key. ✅

7. +++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
   @@ -13,12 +13,13 @@
   -class ResourceTypeRepositoryTest extends KernelTestBase {
   +class ResourceTypeRepositoryTest extends JsonapiKernelTestBase {
   

   Nit: If we're changing this, then we can also simplify $modules.

   Done. ✅

8. +++ b/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
   @@ -97,4 +98,13 @@ public function getProvider() {
   +    $this->assertCount(2, $this->resourceTypeRepository->get('node', 'article')->getRelatableResourceTypesByField('field_relationship'));
   

   👍 I didn't understand this at first, but it makes sense: this field_relationship field can have relationships to both any Node and since two bundles exist, that results in two relatable resource types: node--article and node--page.

9. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -101,16 +108,32 @@ public function all() {
   +      $this->staticCache->set('jsonapi.resource_types', $resource_types, Cache::PERMANENT, $cache_tags);
   

   This is now no longer a simple static cache. When #2984886: Trigger route rebuild when new bundles/fields are added/removed introduced it, it was. But then #3014232: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources changed it to use MemoryCache (which is a pretty new thing in D8, see https://www.drupal.org/node/2973262). And this is then taking that further and changing it into something else entirely. Will do that in the next patch iteration to keep interdiffs easy to understand.

This addresses #85.9.

Remaining deprecation notices (5)

  3x: a:4:{s:11:"deprecation";s:148:"The $bin parameter is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Omit the first parameter. See https://www.drupal.org/node/3061125";s:5:"class";s:67:"Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest";s:6:"method";s:7:"testGet";s:15:"triggering_file";s:68:"/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Cache/BackendChain.php";}
    3x in IDE_PHPUnit_Loader::init

  1x: a:4:{s:11:"deprecation";s:148:"The $bin parameter is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Omit the first parameter. See https://www.drupal.org/node/3061125";s:5:"class";s:67:"Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest";s:6:"method";s:7:"testAll";s:15:"triggering_file";s:68:"/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Cache/BackendChain.php";}
    1x in IDE_PHPUnit_Loader::init

  1x: a:4:{s:11:"deprecation";s:148:"The $bin parameter is deprecated in drupal:8.0.0 and is removed from drupal:9.0.0. Omit the first parameter. See https://www.drupal.org/node/3061125";s:5:"class";s:67:"Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest";s:6:"method";s:11:"testCaching";s:15:"triggering_file";s:68:"/Users/wim.leers/Work/d8/core/lib/Drupal/Core/Cache/BackendChain.php";}
    1x in IDE_PHPUnit_Loader::init

This is caused by the recent #3061117: Make optional and deprecate the BackendChain constructor parameter, easily fixed.

Oops, rebased.

1. While debugging this patch, I noticed that \Drupal\jsonapi\ParamConverter\ResourceTypeConverter::convert() is used on pretty much every JSON:API route, and that calls \Drupal\jsonapi\ResourceType\ResourceTypeRepository::getByTypeName(). Which happens to be the only method on the repository to not yet have been converted from iterating over all returned values until a match is found. Like #85.6 described, this is one aspect of the performance improvement.

   So let's make that also use a direct lookup rather than iterating.

2. I think #85.6's s/cid/key/ was an improvement … but it made me realize something more:

   +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -83,33 +91,36 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +            $entity_type->id() . ':' . $bundle => $this->createResourceType($entity_type, $bundle),
   
   @@ -148,19 +159,9 @@ public function get($entity_type_id, $bundle) {
   +    $key = "$entity_type_id:$bundle";
   
   @@ -395,11 +396,8 @@ protected function getRelatableResourceTypesFromFieldDefinition(FieldDefinitionI
   +      $key = "$entity_type_id:$target_bundle";
   

   This key is awfully reminiscent of something else … the resource type name. It uses -- as a separator instead of :.

   Furthermore, we already know for a fact that each ResourceType must have a unique name, per the JSON:API spec.

   So … let's use resource type names as the keys for ::all()?

   I think this makes overwhelmingly much sense 😂

My initial thought:

With #2819335 having landed, the before vs after performance picture will likely have changed. Would you be able to do the profiling in #73 one last time? 🙏

… but then I realized that actually, this is completely independent of #2819335!

That issue dealt with normalizations of resources (entities). This issue deals with bootstrapping the ResourceTypeRepository.

The profiling in #73 is therefore still accurate. Which means I think this is ready! Thanks for your patience and excellent work here, @ndobromirov! 🙏

Since all I did was renaming things, adding a few comments, removing some whitespace, adding a single test assertion — in short, nothing but trivial bits that did not materially change the patch but merely made it easier to understand, I think it's okay for me to RTBC.

Now we need to change things in JSON:API Extras.

We shouldn't block core commits for mere convenience (because requiring trivial changes) of contrib modules that are not using official APIs. We're careful (and I was too), but it should never result in an un-RTBC unless it's making things completely impossible.

#3019729: Invalidate JSON:API Extras' cache tags when needed will continue to work fine as-is, it'll just need have more cache tags added to it. Besides, #3019729 was a premature commit anyway, because it assumed that a particular implementation would land in JSON:API.

I'm wondering if the running assumption is that the in-memory cache entries can be invalidated mid-request. If there is no use case for that, I think that a property cache may be a more acceptable solution for the in-memory caching. Can you provide more clarification to that end?

Such static caches make unit tests nearly impossible. They also bake in the assumption all over the code base that it's okay to have global state because it's going to disappear in the next request anyway. Baking in this assumption all over the place is what Drupal core used to have, and it hugely complicated tests.

So it's IMHO less about a concrete use case than it is about the risks and consequences of baking in certain assumptions.

Such static caches make unit tests nearly impossible.

Wouldn't that be solved with a setter?

it's okay to have global state because it's going to disappear in the next request anyway.

I think that is indeed OK. I would go further and guess that the in-memory cache back end leverages that fact.

it hugely complicated tests.

Would it be an acceptable approach if a setter or a ::clearMemoryCache() method made those tests simple again?

@e0ipso I was answering in the abstract.

1. This cache backend-based approach is not being introduced here, #2984886: Trigger route rebuild when new bundles/fields are added/removed introduced it due to observed bugs.
2. I think you're forgetting that the in-memory cache is also persisted?

@ndobromirov that's super interesting! Are you using PHP:PM with a modern version of Drupal?

Very nice.

Committed 22a2503 and pushed to 8.8.x. Thanks!