Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Setting to major for the possible performance benefits from this.
I've been testing the latest group dev with the query access patch and as discussed with @kristiaanvde, what stands out the most is the performance of loadByEntity(), which I suspect is called more often than before.
The method already has a static cache, bin in my case, it was called with 130 different entities.
The method does 3 things:
1. Load group content types for the entity type of the requested entity. This is currently not cached at all and does an expensive config entity query, among other things
2. Query for group_content entities for the given types and entity ID. Uses an entity query, which is notoriously slow. It's a query builder on top of a query builder that creates bad queries with unnecessary joins, plus the alter hooks and so on.
3. load the entities.
For 1, we can add a static cache as a first step. I added it on \Drupal\group\Entity\Storage\GroupContentTypeStorage::loadByEntityTypeId(), but the overhead of that might be pretty minor and we could maybe only statically cache \Drupal\group\Entity\Storage\GroupContentTypeStorage::loadByContentPluginId(). Additionally, it would make sense to add a lookup key to the group content type entity type, to improve the first-time lookup as well, but that might be a relatively minor improvement compared to the static cache.
For 2. converting the entity query to a plain query against the database table helps.
Not much we can do about 3.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | interdiff-3158906-4-6.txt | 4.61 KB | kristiaanvandeneynde |
| #6 | improve_performance-3158906-6.patch | 6.27 KB | kristiaanvandeneynde |
| |||
Comments
Comment #2
BerdirExpecting some test fails from this as we need to reset the static cache if group_content entities are created or deleted.
Comment #4
BerdirInteresting, so somehow these tests only failed because I I need double {{ }} because the inner ones are for the variable. Not sure why only those failed and not everything exploded, but easy fix.
After that, nothing seemed to fail, so I did add an explicit test for the static cache reset.
Comment #5
BerdirAlso, this is the before/after of my profiling. Ignore the specific numbers, xdebug was enabled and blackfire tends to make things 2-3x as slow, especially a lot of fast calls.
Comment #6
kristiaanvandeneyndeThanks a lot, that Blackfire result looks really promising. I've looked into adding a "lookup_keys" before, but was unsure if that implied I needed to write an update hook for it to take effect (by clearing the internal cache somehow) and ran out of time investigating that.
The answer to that is sadly: When I first started writing Group for D8, my test-writing skills were basically zero. They have since improved a lot and new features are properly tested, as are new submodules (really proud of the tests in Subgroup). So until I open a 2.0.x branch where I refactor or clean up my older code that I'm no longer happy with, there are a few gaps in Group's test coverage :(
Attached is a patch which basically does nothing but change the code style a little. Credit will remain yours for all the help you've been lately :)
Comment #8
kristiaanvandeneyndeComment #9
Berdir> there are a few gaps in Group's test coverage :(
Yeah, but I'm not talking about gaps here :) This query should have failed every single time it was called in a test, as it should always be a prefixed table in tests, and loadByEntity() is a central piece of the API. Not sure what's different about kernel tests.
Anyway, doesn't matter too much. Thanks for the commit.