Improve performance of the membership loader

Thinking of scrapping the membership loader altogether, see 📌 Deprecate the membership loader in favor of shared bundle classes Fixed

We can try to incorporate this into the new static methods from that issue, though. But I'm thinking of using a memory cache so that we can use cache tags rather than manual clears. It's just safer that way as we have dedicated cache tags for when someone gets a new membership for instance.

I'd like this in the 3.3.0 release. We can try and bake this into the new GroupMembership shared bundle class.

Rerolled the patch for 2.x.

Gonna see if I can fix this in 3.3.x

Found a small issue with my latest patch, this should fix it!

1. +++ b/src/GroupMembershipLoader.php
   @@ -47,25 +61,63 @@ class GroupMembershipLoader implements GroupMembershipLoaderInterface {
   +    // Prime the cache with all group memberships via ::loadByUser().
   

   People could have a large amount of memberships, every membership incurring an entity load. I think it would be better to load the single membership here and rely on the fact that people use single loads sparingly.

2. +++ b/src/GroupMembershipLoader.php
   @@ -47,25 +61,63 @@ class GroupMembershipLoader implements GroupMembershipLoaderInterface {
   +    return $this->userMemberships[$account->id()][$cache_id][$group->id()] ?? FALSE;
   

   Don't think this will work as we don't set the third index of this array anywhere. $group->id() is nowhere used in loadByUser()

Adjusting the above in the MR I'm working on.

Cross-posted :/ Think you just addressed my 2nd point. Will take it from here.

This is what I had in mind. Reviews welcome.

Simplified the cache ID logic, we don't need md5 here IMO. Also swapped the dash out for a dot because role IDs can contain dashes and we could potentially get a cache ID collision if we use dashes.

I'm not a fan of all the static stuff, but that's what you get with these bundle classes. Also not a fan of calling a cache clear three times, I think we need to switch to cache tags on a memory cache here. That way, we don't need any manual clears at all and it works even if people bypass Group::addMember() ad Group::removeMember().

We used to have a filter argument on the storage loader methods. I removed those because it made caching the outcome really hard. Now I'm thinking if we should move all of this logic back to the storage, find a clever way to cache it there and then simply rely on the storage for doing the heavy lifting.

Hmm, gonna revert the move to the storage handler for a bit and think about this more.

Hmm, issues I see with this are when your conditions become invalid.

E.g.: We cache all members of a group with certain roles. During the request we change one of these members' roles. Now we're screwed because we will keep returning said member from the cache as if they still had the roles we're querying for.

If we were to use a proper cache, this might not be a problem because we could invalidate the entry by virtue of cache tags. The only downside is that we only promoted MemoryCache to a fully supported one in D10.3 ( 📌 Fix MemoryCache discovery and DX Needs review ), so any test relying on RefreshVariablesTrait won't work with whatever code relies on the MemoryCache.

Currently thinking of using MemoryCache nonetheless and adding a @todo to properly declare it once D10.3 is out.

We could even use a DB cache here and merely cache the GroupRelationship IDs. EntityStorageBase already makes sure we do not load an entity more than we need to, so we could then query our cache and call $storage->loadMultiple() on it. If an entity is saved anywhere, we will get a fresh copy without having to worry about invalidating our own caches.

This will also keep the memory footprint down because we're only caching IDs rather than full entities. The only thing we have to make sure is that we really get our cache tags right so that we do serve stale results from the DB/static cache.

Pushed a version which uses a proper cache as described above. This cache stores the IDs and then we still call $storage->loadMultiple() on that, keeping in mind that the storage caches the actual entity objects.

If this works, it could be a really elegant solution as we wouldn't have to manually clear anything.

Tomorrow is my day off, but I'll run the latest version through our tests to see if we can remove the custom cache resets. Thanks for working on this. The content overview was kind of slow taking over 10 sec for some users. With the patch it went down to like 4/5 sec, so pretty significant improvement.

Tests are still failing, will try to fix that tomorrow

Okay so this goes green now (will fix the unrelated phpstan issues in a separate issue).

The beauty if this solution is that it caches the query results across requests, meaning that if your membership information doesn't change, we immediately know what your membership IDs are from a single cache get rather than having to go to the potentially humungous group_relationship_field_data table.

One thing to keep an eye on is how many cache entries this will generate, we are currently using the default backend as the DB backend, but we might want to assign a custom one like we did for the memory cache.

@catch hinted that for the single loads I could use a primed cache to reduce the amount of entity queries we're firing when warming the cache. Something that was already in the earlier patches, actually.

Will see if I can adjust the MR with that change in mind.

Okay, all tests go green with the recent suggestion added to the MR. I'm gonna add one more comment to document why we choose to prime the cache with loadByUser rather than loadByGroup and call it a day. I'm feeling really optimistic about the impact this MR might have on performance.

Made a few changes:

• Used a custom DB backend because on large websites this may take up a lot of data, so people can specifically turn off the persistent caching but not the memory caching.
• Removed the individual cacheability, list cache tags should suffice here

Right, for me this patch is "ready". So if any of the reporters in this issue could verify and post results, that would be great. :)

Thinking of targeting the next release against D10.3 instead of D10.2. There will only be a few weeks at most after this release before 10.3 is out so may as well immediately benefit from the new code.

Okay this looks good to me. I really was hoping for at least one of the people credited here to provide some results, but at one point I just have to accept that it may not happen and move on, hoping for the best.

We have test coverage, so it really shouldn't break anything. Whether it drastically increased performance is left to be seen. Group has no tests using PerformanceTestTrait yet, so I'm shooting in the dark here. Might want to add one of those tests now that the minimum version is D10.3.

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

Follow-up: 🐛 Invalid service definition for cache services Needs review