Add JSON:API performance tests

+1 😄

Although I wonder how representative this would be of the real world, where JSON:API performance problems likely only appear on sites with either a huge number of (content) entities or very complex (content) entity types (i.e. with lots of fields on them).

🐛 RoutePreloader loads a lot of routes Active is a good example of something that impacts JSON:API. It might not have the same impact as a complex and slow filters on a very large database but in its current state, it would add 10 or so queries to entity types, even single ones with a bunch of reference fields.

There's a german saying that's basically "Trust is good, control is better". As always with the performance tests, they're full of interesting things.

I added a test that visits a json api resource (node detail page) on a cool cache (frontpage visited, but no jsonapi route yet), then one with a warm dynamic page cache cache, then invalidates that node and visits the page again.

The most obvious thing that I saw is that there are around 9 uncached loadbyUuid lookup queries for the same UUID, didn't look into that yet but that seems like a very obvious target for optimizations, especially considering that uuid entity queries are really inefficient as they join the data table for absolutely no reason at all (existing open issues).

The route lookups are as expected, 6 separate route lookups that are then separately cached, and with the current implementation in [##3503843] would no longer be cached. Note that it's 8 data cache lookups (1 for the route match, then 7 route preloads, including the full frontend route preload even though that actually excludes jsonapi routes. I think my suggestion in the other issue to keep track of routes by format could be interesting as we could keep all of them in discovery cache.

Another thing is the resource type caches, those could be candidates for the discovery or bootstrap bin instead of default?

✨ Add static cache for loadEntityByUuid function to store uuid-id pairs in memory Active is already open for those uuid lookups, but no MR there yet.

When looking for that I also found #2878673: Cache loadEntityByUuid() calls for block_content → .

The most obvious thing that I saw is that there are around 9 uncached loadbyUuid lookup queries for the same UUID, didn't look into that yet but that seems like a very obvious target for optimizations

Yeah I wonder why we would ever even need to do that unless the request itself has the uuid and we need to resolve from that. Wonder if something is going id -> uuid -> id.

The request/route is based on the UUID, so one UUID lookup is expected, but no idea why it's then doing it again so many times, looks like something during normalization does that. Should be pretty easy to find, a static cache would hide that, but maybe we can also prevent it from happening.

And yes, block_content was done as a cache collector under the assumption that there's a somewhat limited amount of block_content blocks placed as regular blocks. that doesn't scale and I think a 1:1 cache isn't that useful either.

\Drupal\jsonapi\ParamConverter\EntityUuidConverter() is the problem, and the fact that it does the upcasting when generating routes, so we do that once for every route that contains the node UUID.

It also doesn't even go through \Drupal\Core\Entity\EntityRepository::loadEntityByUuid() at the moment, so a static cache won't help without changing that. We should be able to change that though.

And on a collection with 50 nodes, it's going to do that with 50 different UUID's.

One general thing - when I originally added PerformanceTestTrait and the performance test base class I had good intentions of adding functional and kernel variants of it, but obviously haven't done that yet and not sure there are even issues. This issue is one where we really don't need a real browser to do the testing.

I don't think that should block this issue, but it might be time to open issues for kernel and functional variants, and then we could convert this over when those base classes are available to use.

Yes, would make sense to not having to go through a real browser here. I was considering for a second if I should try to use the trait on a functional test and expected some kind of problem when using that to access non-HTML pages, but it worked so I just went with that.

FWIW, I think it makes sense to get this committed without any improvements first, and then we can use the test in issues like ✨ Add static cache for loadEntityByUuid function to store uuid-id pairs in memory Active to show the improvements.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

fixed new phpcs rule.

Although a small question regarding the sleep, although this seems sensible. Don't see any issues here. Does remind me of using metrics.txt for this in gitlab.

There are two main reasons for the sleep() in performance tests:

1. Cache collectors end up writing different things depending on whether the page or asset aggregates complete first. This should not be an issue for JSON:API but it is for every other performance test. This is why there are multiple requests to warm caches and sleeping.

2. Performance tests don't use WaitTerminateTrait, partly due to the requirement for an additional database query (but that might go away after 📌 Move test middleware out of CoreServiceProvider Active ), but partly for 'realism'. We could maybe revisit this one after that issue lands.

JSON:API shouldn't need to worry about #1, but it probably does still need to worry about #2.

Haven't actually reviewed the MR but will try to soon.

Overall this looks great but one thing to mark it needs work for.

I removed one extra request.

Locally, the test passed without any frontpage request and no sleep but on CI it didn't. Unsure how relevant it is to warm up the regular front caches and asset stuff at all. I tested once by going to /jsonapi overview instead, but that for example removes the path prefix lookup for /jsonapi and I kind of like having that once in there.

I also tested a repeat test run, but those are missing the chrome container: https://git.drupalcode.org/issue/drupal-3438070/-/jobs/4781741

Oh I completely missed that the cache warming was requesting the front page still (and not a JSON:API page) which still means asset aggregates get requested which could still result in cache collector fun - BUT because we log in the user before that, those caches will be warmed before we hit the front page anyway, or worst case by the front page request, so we should indeed not need two.

Committed cd5bc86 and pushed to 11.x. Thanks!

catch → closed merge request !11168

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