Resolve included resources prior to normalization

This depends on #2996000: Deduplicate `meta.omitted.links` based on `href` (`via` link).

I'll provide a self-review of this patch to help explain it in my next comment.

1. +++ b/src/Controller/EntityResource.php
   @@ -123,12 +137,12 @@ class EntityResource {
   -  public function getIndividual(ResourceType $resource_type, EntityInterface $entity) {
   +  public function getIndividual(ResourceType $resource_type, EntityInterface $entity, Request $request) {
   

   Since includes are now being resolved prior to normalization, we need access to the include parameter. Hence, we add the Request object.

2. +++ b/src/Controller/EntityResource.php
   @@ -123,12 +137,12 @@ class EntityResource {
   -    $response = $this->buildWrappedResponse($entity);
   +    $response = $this->buildWrappedResponse($entity, $this->getIncludes($request, $entity));
   

   Now, the primary data and includes are passed as an explicit argument to buildWrappedResponse.

3. +++ b/src/Controller/EntityResource.php
   @@ -385,7 +399,7 @@ class EntityResource {
   -    $response = $this->respondWithCollection($entity_collection, $entity_type_id);
   +    $response = $this->respondWithCollection($resource_type, $entity_collection, $this->getIncludes($request, $entity_collection));
   

   The same is true for respondWithCollection

4. +++ b/src/Controller/EntityResource.php
   @@ -872,6 +898,30 @@ class EntityResource {
   +  public function getIncludes(Request $request, $data, $related_field = NULL) {
   +    return $request->query->has('include') && !empty($include_parameter = $request->query->get('include', ''))
   +      ? $this->includeResolver->resolve($request->get(Routes::RESOURCE_TYPE_KEY), $data, $include_parameter, $related_field)
   +      : NULL;
   +  }
   

   This is what resolves includes in the controller. We only enter the resolution process if the request actually has an include parameter and it's not empty.

5. +++ b/src/Exception/EntityAccessDeniedHttpException.php
   @@ -8,15 +8,19 @@ use Drupal\Core\Cache\CacheableMetadata;
   +use Drupal\jsonapi\JsonApiResource\ResourceIdentifier;
   +use Drupal\jsonapi\JsonApiResource\ResourceIdentifierInterface;
   +use Drupal\jsonapi\JsonApiResource\ResourceIdentifierTrait;
   

   1. The spec says that included must not contain duplicate resources.
   2. This patch resolves includes into an EntityCollection.
   3. It makes sense for EntityCollection to then have a method to deduplicate itself.
   4. EntityCollection can receive objects that are not entities, but have a 1:1 correspondence with an entity. Like a LabelOnlyEntity or an EntityAccessDeniedHttpException.
   5. Therefore, EntityCollection needs a uniform interface that it can use to compare and deduplicate its items.
   6. This patch introduces a ResourceIdentifierInterface for that purpose and a trait that makes it easy to implement.

6. +++ b/src/IncludeResolver.php
   @@ -0,0 +1,220 @@
   +/**
   + * Resolves included resources for an entity or collection of entities.
   + *
   + * @internal
   + */
   +class IncludeResolver {
   ...
   +  public function resolve(ResourceType $base_resource_type, $data, $include_parameter, $related_field = NULL) {
   ...
   +    $include_tree = static::toIncludeTree($base_resource_type, $include_parameter, $related_field);
   +    return EntityCollection::deduplicate($this->resolveIncludeTree($include_tree, $entity_collection));
   +  }
   

   The include resolver works with a 3 phased approach.

   1. First, it parses the include query parameter into a tree of field names.
   2. Then, it recursively descends that tree of field names against an initial EntityCollection. As it descends the tree, it loads targeted entities and adds them to a resolved EntityCollection.
   3. Finally, before it returns the resolved EntityCollection it deduplicates it.

7. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -144,4 +144,42 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +  public static function merge(EntityCollection $a, EntityCollection $b) {
   

   This is how the include resolver appends newly resolved includes to an EntityCollection

8. +++ b/src/JsonApiResource/EntityCollection.php
   @@ -144,4 +144,42 @@ class EntityCollection implements \IteratorAggregate, \Countable {
   +  public static function deduplicate(EntityCollection $collection) {
   ...
   +        $deduplicated[$resource_identifier->getTypeName() . ':' . $resource_identifier->getId()] = $resource;
   ...
   +        $deduplicated[$resource->getTypeName() . ':' . $resource->getId()] = $resource;
   

   This is how the ResourceIdentifierInterface is used to deduplicate includes.

9. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
   @@ -10,7 +10,7 @@ namespace Drupal\jsonapi\JsonApiResource;
   - * @todo Add support for the missing optional members: 'jsonapi', 'links' and 'included' or document why not.
   + * @todo Add support for the missing optional members: 'jsonapi' and 'links' or document why not.
   

   It seems that we're slowly getting rid of all this :D

10. +++ b/src/JsonApiResource/JsonApiDocumentTopLevel.php
    @@ -21,15 +21,27 @@ class JsonApiDocumentTopLevel {
    +    assert(!$data instanceof ErrorCollection || is_null($includes));
    

    This assert is unnecessary. Will remove it.

11. +++ b/src/Normalizer/EntityNormalizer.php
    @@ -9,8 +9,6 @@ use Drupal\Core\Field\FieldTypePluginManagerInterface;
    -use Drupal\jsonapi\Normalizer\Value\IncludeOnlyRelationshipNormalizerValue;
    -use Drupal\jsonapi\Normalizer\Value\NullFieldNormalizerValue;
    
    +++ b/src/Normalizer/RelationshipItemNormalizer.php
    @@ -5,9 +5,7 @@ namespace Drupal\jsonapi\Normalizer;
    -use Drupal\jsonapi\JsonApiResource\JsonApiDocumentTopLevel;
    ...
    -use Drupal\jsonapi\Controller\EntityResource;
    
    @@ -50,27 +48,15 @@ class RelationshipItemNormalizer extends FieldItemNormalizer {
    -    $target_entity = $relationship_item->getTargetEntity();
    ...
    -    if (!empty($context['include']) && in_array($host_field_name, $context['include']) && $target_entity !== NULL) {
    
    +++ b/src/Normalizer/Value/EntityNormalizerValue.php
    @@ -67,24 +60,12 @@ class EntityNormalizerValue implements ValueExtractorInterface, CacheableDepende
    -    // Gather includes from all normalizer values, before filtering away null
    -    // and include-only normalizer values.
    -    $this->includes = array_map(function ($value) {
    -      return $value->getIncludes();
    -    }, $values);
    ...
    -    $this->includes = array_reduce($this->includes, function ($carry, $includes) {
    -      return array_merge($carry, $includes);
    -    }, []);
    ...
    -    $this->includes = array_filter($this->includes);
    @@ -113,16 +94,6 @@ class EntityNormalizerValue implements ValueExtractorInterface, CacheableDepende
    -  public function rasterizeIncludes() {
    
    @@ -133,19 +104,4 @@ class EntityNormalizerValue implements ValueExtractorInterface, CacheableDepende
    -  public function getIncludes() {
    -    $nested_includes = array_map(function ($include) {
    -      return $include->getIncludes();
    -    }, $this->includes);
    -    return array_reduce(array_filter($nested_includes), function ($carry, $item) {
    -      return array_merge($carry, $item);
    -    }, $this->includes);
    
    +++ b/src/Normalizer/Value/FieldNormalizerValue.php
    @@ -60,13 +53,6 @@ class FieldNormalizerValue implements FieldNormalizerValueInterface {
    -    $this->includes = array_map(function ($value) {
    -      if (!$value instanceof RelationshipItemNormalizerValue) {
    -        return NULL;
    -      }
    -      return $value->getInclude();
    -    }, $values);
    -    $this->includes = array_filter($this->includes);
    
    @@ -90,22 +76,6 @@ class FieldNormalizerValue implements FieldNormalizerValueInterface {
    -  public function rasterizeIncludes() {
    -    return array_map(function ($include) {
    -      return $include->rasterizeValue();
    -    }, $this->includes);
    -  }
    ...
    -  public function getIncludes() {
    -    return $this->includes;
    -  }
    @@ -113,22 +83,4 @@ class FieldNormalizerValue implements FieldNormalizerValueInterface {
    -  public function getAllIncludes() {
    -    $nested_includes = array_map(function ($include) {
    -      return $include->getIncludes();
    -    }, $this->getIncludes());
    -    $includes = array_reduce(array_filter($nested_includes), function ($carry, $item) {
    -      return array_merge($carry, $item);
    -    }, $this->getIncludes());
    -    // Make sure we don't output duplicate includes.
    -    return array_values(array_reduce($includes, function ($unique_includes, $include) {
    -      $rasterized_include = $include->rasterizeValue();
    -      $unique_includes[$rasterized_include['data']['type'] . ':' . $rasterized_include['data']['id']] = $include;
    -      return $unique_includes;
    -    }, []));
    -  }
    
    +++ b/src/Normalizer/Value/FieldNormalizerValueInterface.php
    @@ -11,14 +11,6 @@ use Drupal\Core\Cache\CacheableDependencyInterface;
    -  public function getIncludes();
    
    @@ -27,12 +19,4 @@ interface FieldNormalizerValueInterface extends ValueExtractorInterface, Cacheab
    -  public function getAllIncludes();
    
    +++ /dev/null
    @@ -1,32 +0,0 @@
    -class IncludeOnlyRelationshipNormalizerValue extends FieldNormalizerValue {
    
    +++ b/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
    @@ -109,16 +112,11 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
    -      $this->includes = array_map(function ($value) {
    -        return $value->getIncludes();
    -      }, $values);
    ...
    -      $this->includes = array_reduce($this->includes, function ($carry, $includes) {
    -        array_walk($includes, [$this, 'addCacheableDependency']);
    -        return array_merge($carry, $includes);
    -      }, []);
    @@ -246,42 +224,13 @@ class JsonApiDocumentTopLevelNormalizerValue implements ValueExtractorInterface,
    -  public function getIncludes() {
    -    $nested_includes = array_map(function ($include) {
    -      return $include->getIncludes();
    -    }, $this->includes);
    -    $includes = array_reduce(array_filter($nested_includes), function ($carry, $item) {
    -      return array_merge($carry, $item);
    -    }, $this->includes);
    -    // Make sure we don't output duplicate includes.
    -    return array_values(array_reduce($includes, function ($unique_includes, $include) {
    -      $rasterized_include = $include->rasterizeValue();
    -
    -      if (empty($rasterized_include['data'])) {
    -        $unique_includes[] = $include;
    -      }
    -      else {
    -        $unique_key = $rasterized_include['data']['type'] . ':' . $rasterized_include['data']['id'];
    -        $unique_includes[$unique_key] = $include;
    -      }
    -      return $unique_includes;
    -    }, []));
    -  }
    
    +++ b/src/Normalizer/Value/NullFieldNormalizerValue.php
    @@ -36,13 +36,6 @@ class NullFieldNormalizerValue implements FieldNormalizerValueInterface {
    -  public function getIncludes() {
    
    @@ -57,18 +50,4 @@ class NullFieldNormalizerValue implements FieldNormalizerValueInterface {
    -  public function rasterizeIncludes() {
    ...
    -  public function getAllIncludes() {
    
    +++ b/src/Normalizer/Value/RelationshipItemNormalizerValue.php
    @@ -40,17 +33,10 @@ class RelationshipItemNormalizerValue extends FieldItemNormalizerValue implement-    assert($include === NULL || $include instanceof EntityNormalizerValue || $include instanceof JsonApiDocumentTopLevelNormalizerValue || $include instanceof HttpExceptionNormalizerValue);
    ...
    -    if ($include !== NULL) {
    -      $this->setCacheability(static::mergeCacheableDependencies([$include, $values_cacheability]));
    -    }
    
    @@ -72,21 +58,4 @@ class RelationshipItemNormalizerValue extends FieldItemNormalizerValue implement
    -  public function rasterizeIncludes() {
    ...
    -  public function getInclude() {
    +++ b/src/Normalizer/Value/ValueExtractorInterface.php
    @@ -17,12 +17,4 @@ interface ValueExtractorInterface {
    -  public function rasterizeIncludes();
    

    This patch let's us eliminate SOO much complexity from the normalization process 🤩. I didn't even highlight all of it.

    A lot of that complexity to the include resolver, but hopefully having it all in one place will make the whole system easier to reason about, profile, and maintain.

WOOT, #2 only has Kernel and Unit test failures!

This cleans up all the unit tests.

#2984911: Remove access to the Request object in the normalization process landed, so this needed a reroll.

I reached out to a few individuals who I thought may have the infrastructure in place to run some performance benchmarks against this patch (@btully, @caseylau, @mglaman, @sime & @steven.wichers)

I'm excited to see all of their results, but I have some initial results to share.

@mglaman was kind enough to set up two environments based on the 2.x-dev branch, one without the patch applied and one with it applied. He also provided test data and a testing URL that emulates a typical commerce site. That data came from Commerce Demo. On those applications, both page_cache and dynamic_page_cache were uninstalled so that each request would thoroughly exercise the changes in the patch.

That URL was: https://{environment_hostname}/jsonapi/commerce_product/simple?include=field_brand,field_product_categories,field_special_categories,stores,variations&fields[taxonomy--term]=name

I used Postman to run 100 requests against each environment from my local machine.

I've compiled the results here: https://docs.google.com/spreadsheets/d/1OkB-MdDXPt3o_v6oxIKx7ZCDnDpFa9y5...

On average, the request took 5.77 seconds to complete without the patch in #13. With the patch applied, the request took an average of 4.08 seconds to complete. These times include network round trips from my local to the testing environment and back.

This means that the patch reduced average request time by 29.19%, or, put differently, the requests were completed 41.22% faster.

Given that these results include network latency, I believe the performance improvement is underestimated.

@caseylau, does that bug exist without the patch on 2.x-dev?

Per #20, the current patch doesn't resolve nested references.

That is very unlikely because we have explicit test coverage for that.

I think this is a more subtle regression than that. Namely that more than one bundle will end up in the EntityCollection and that some of those bundles may not have all of the same fields.

@Wim Leers, remember this?

// @todo: see if we can live without this.
if (!$entity->hasField($field_name)) {
  continue;
}

My suspicion is that the answer is no, we can't :P

I'm going to try to replicate the issue in #20 in a moment.

I was able to reproduce the bug described in #20.

It was indeed what I suspected in #22.

Here's a simple fix :)

#2996000: Deduplicate `meta.omitted.links` based on `href` (`via` link) landed! So we don't need a combined patch anymore.

@caseylau's example URL has something strange in it.

/jsonapi/taxonomy_term/fruit?include=field_node,field_node.field_tags

It needs both field_node and field_node.field_tags. But with this patch applied, it shouldn't need field_node to work. So, why is it needed? Because using field_node.field_tags alone does not work in HEAD. In fact, we already have an old issue for that: #2946537: Test coverage: Inclusion of intermediate resources when include is a multi-part relationship path

Manual testing should show that this patch does not require the intermediate field.

I tried to apply the test coverage from #2946537 and found out that it's incomplete. The actual results are correct, but the expectations are wrong.

@btully, just shared some results with me. While I can't share the details of project, I can generally summarize the initial results.

He and @steven.wichers's data is primarily focused on testing concurrent users. But out of that, we can extract average response times before and after the patch was applied.

The initial data that I've received is for requests with no includes. That means we should probably not expect much improvement there.

That said, we actually did see some improvement. Average response time dropped from 3.29 seconds to 3.14 (π!). That's 4.8% faster. It's not very significant, but it's useful because it tells us that it didn't make non-include requests slower 👍.

Other observations:

@btully shared a link to New Relic with me. That let me drill down and explore what PHP calls took the most time. Two things jumped out at me...

First, nearly half the time was spent in getAccessCheckedEntity. While we can't really do anything to help with that, we can perhaps make some recommendations in the module docs about it (e.g. make use of static caching wherever possible).

Second, the other half of the time was spent in the normalization process. Not surprising. JSON API is primarily a module that normalizes data into JSON. What was surprising is that a large part of that time was spent normalizing (and computing) the processed text property. Again, we can't do a ton about that, but we may want to consider a Drupal core issue to somehow make this a stored value instead a computed property (I realize that would be very difficult since one can have any number of different text formats). If not, suggest that users disable or exclude it if it's not necessary.

@btully is running a second set of benchmarks that do have includes. I'll report those results soon.

@caseylau, thanks! And.. wow. I did not expect that!

It does raise the question, why is 2.x is so much slower than JSON API 1.x?

Regardless, that's a tremendous improvement. Thanks for sharing your results!

@btully, shared his results with me last evening.

Prior to applying the patch, response times were 8.64s. After applying the patch, they dropped to 6.3s. That's 27% less response time or just over 37% faster. That roughly matches my results in #18. It's certainly not as dramatic as @caseylau's results, but that's somewhat logical, @caseylau's request has a lot more includes than @btully's and my request.

Another thing to point out... remember that without includes, the request took 3.14s in #27. Almost exactly half the time. Looking at @btully's query, I suspect that he has about an equal number of included resources as he does primary resources. Which means that the cost of includes has been dramatically reduced. What we're seeing here is a relatively linear scaling between the number of resources serialized and the response time. @caseylau's results show that without the patch, it's much worse than that.

IOW, with this patch, includes no longer bear a significant penalty.

Given all these results, I think this is a valuable change. Let's try to land it 🙂

Afterward, we can attack the normalization process and find improvements there.

This failed testing because of #2998601: HEAD is failing tests for various reasons.

Partial review. Sorry I had to drop the review mid patch.

I'll bet you had a good excuse ;) Don't worry about it!

1. heh, that was a little too clever wasn't it? Honestly, that was a "leftover" from some bug hunting that I did. Fixed.
2. That's just what I thought made most sense when I was resolving conflict during a rebase. I'll reorder it to better respect HEAD. Having the NullEntityCollection you suggest will make it nicer since the optional arg will go away (meaning it doesn't need to be the final arg).
3. AFAICT, we always treat the langcode field as an attribute, not a relationship. That means that this function will never be called because there's not a related route for it.
4. Done. I really like that :)
5. Yep. Done.
6. Done.
7+8: :D
9. That's a PHP7 only thing. Can we make a followup issue to convert this using a polyfill? We could do that under the ForwardCompatibility namespace.

Eww, I generated that last patch with -M to make a smaller patch, but it's unreadable. Here's a more readable version.

Fixed.

Thank you SO much @caseylau. That was incredibly useful!

I set up the site you provided then used git bisect to find the commit which introduced the performance regression. That gave me this: db8111aff492a7d7ff9ab1b14b10bd74ca6de49e is the first bad commit

So, that means that the regression was introduced in #2948666: Remove JSON API's use of $context['cacheable_metadata'].

Checking out that version and the one immediately prior to it, I isolated the issue. Before that commit, includes were rasterized in JsonApiDocumentTopLevelNormalizer. After that commit, rasterization of includes was moved into JsonApiDocumentTopLevelNormalizerValue::rasterizeValue().

Why did that cause an issue?

Because JsonApiDocumentTopLevelNormalizer::normalize() used to have these lines:

$value_extractor = $this->buildNormalizerValue($object->getData(), $format, $context);
if (isset($context['is_include_normalization']) && $context['is_include_normalization'] === TRUE) {
  return $value_extractor;
}

What those lines did was cause an early return just before includes are rasterized.

After #2948666, include rasterization was moved to JsonApiDocumentTopLevelNormalizerValue::rasterizeValue(). That method is called recursively from the top level object. This recursive call meant that nested includes were rasterized multiple times. At least as many times as there were levels of includes. IOW, an include 3 levels deep would be rasterized at least 3 separate times.

That is where I stopped.

This patch eliminates recursive rasterization and therefore eliminates that performance regression, explaining the 12x improvement @caseylau saw in #28.1.

The remaining 3x improvement is likely due to the fact that we're not normalizing duplicate includes and because we're aggregating entity loads rather than loading includes 1 by 1.

Thanks for the thorough review!

I agree with all your changes. (And I also lament that fact that private services are such a PITA!)

#45.15 was a good catch. Restored the implementation.

IMHO, this is ready for commit. We've had thorough review and approval by all three maintainers, more community review and validation than any other issue in the module's history, and we've simplified and made the codebase faster. YEAH!