Improve performance of the membership loader

I see there is a lot of speed after applying the patch. From 10s to 1s. And also after apply of this @https://www.drupal.org/node/2888588

When not providing an account in the loadByUser method you will get the following error: Call to a member function id() on null. I have moved the current user fallback code on top in the method.

Patch #6 went a bit wrong, the #7 patch is the correct one.

In theory this sounds like a great idea. I think providing some kind of actual performance numbers would be super awesome, but I don't think you could go wrong with committing this.

I'm not sure how strict the group maintainers are regarding tests, but it looks like this doesn't provide any - but it does increase the API surface. I think this should be covered with unit or kernel tests.

Comment #7 breaks getMembers.

Original:

  protected function wrapGroupContentEntities($entities) {
    $group_memberships = [];
    foreach ($entities as $group_content) {
      $group_memberships[] = new GroupMembership($group_content);
    }
    return $group_memberships;
  }

New:

  protected function wrapGroupContentEntities($entities) {
    $group_memberships = [];
    foreach ($entities as $group_content) {
      $group_memberships[$group_content->gid->target_id] = new GroupMembership($group_content);
    }
    return $group_memberships;
  }

Only one membership per group is allowed.

With correct patch this time ;-)

Update status

You might also be interested in #3029849: Statically cache gnode_node_grants()

This badly broke in our case, the group page was no longer accessible. Didn't investigate yet, but somehow "return GroupAccessResult::allowedIfHasGroupPermission($entity, $account, 'view group');" returned FALSE for the user, despite having access to do that.

Uploading a proper patch that doesn't contain "modules/contrib/group" in the path might reveal some test fails.

Keeping an eye on this. Maybe we should offload some of the logic to the GroupContentStorage? That would be the ideal place to do any caching related to loading group content entities. The membership loader is also a valid class to do some static caching in, but the bulk should probably happen in the storage.

#3029878: Statically cache loadByEntity() takes a similar approach (making the storage responsible for caching)

Here's a new version of #11, without the modules/contrib/group paths included.

Work in progress, but this reduces the number of test failures.

Its green. I'm not familiar enough with group yet to properly review the patch unfortunately.

I'm close to introducing a custom static cache backend and would like to use that instead of class properties because it would support cache tags for clearing the cache entries. See the feature/query_access branch for the 'cache.group.static_no_serialize' service.

So this should be marked OutDated or Wont Fix?

We have MemoryCache in core now, https://www.drupal.org/node/2973262

This is still an issue, so should stay open I think even if the fix ends up being different.

1. +++ b/src/GroupMembershipLoader.php
   @@ -78,24 +92,44 @@ class GroupMembershipLoader implements GroupMembershipLoaderInterface {
   +
   +    if (!isset($this->userMemberships[$account->id()][$cache_id][$group->id()])) {
   +      $group_contents = $this->groupContentStorage()->loadByGroup($group, 'group_membership', $filters);
   +
   +      if ($group_memberships = $this->wrapGroupContentEntities($group_contents)) {
   +        $this->userMemberships[$account->id()][$cache_id][$group->id()] = reset($group_memberships);
   +      }
   +    }
   

   This sets $this->userMemberships[$account->id()[$cache_id][$group->id()] with a single group.

2. +++ b/src/GroupMembershipLoader.php
   @@ -106,6 +140,15 @@ class GroupMembershipLoader implements GroupMembershipLoaderInterface {
        }
    
   +    $cache_id = md5($account->id());
   +    if ($roles) {
   +      $cache_id = is_array($roles) ? ':' . md5(implode('-', $roles)) : ':' . md5($roles);
   +    }
   +
   +    if (isset($this->userMemberships[$account->id()][$cache_id])) {
   +      return $this->userMemberships[$account->id()][$cache_id];
   +    }
   +
   

   This looks in the same cache item and takes what's in there - but it can already have been primed for a single group earlier in the request.

   So if the first call to the membership loader is for a single group, you end up with a corrupt static cache of group memberships, and this in turn corrupts the group permissions cache.

   If we wanted to share the cache between load() and loadByUser() then it either needs separate cache entries with load() falling back to loadByUser() or always populate the cache ID for every group.

Additionally:

    if (!isset($this->userMemberships[$account->id()][$cache_id][$group->id()])) {
      $group_contents = $this->groupContentStorage()->loadByGroup($group, 'group_membership', $filters);

      if ($group_memberships = $this->wrapGroupContentEntities($group_contents)) {
        $this->userMemberships[$account->id()][$cache_id][$group->id()] = reset($group_memberships);
      }
    }

This only sets the static cache if a group membership is returned, meaning if ::load() is called repeatedly for the same group membership that doesn't exist, it will try to load it each time. It would be better to explictly cache when no memberships are returned too.

This should help - just check to see if we already called loadByUser() and if not, call it and use the results of that.

Fixing that bug in the patch uncovered another one.

Thinking of scrapping the membership loader altogether, see #3382559: Deprecate the membership loader in favor of shared bundle classes

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.

Gonna see if I can fix this in 3.3.x

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 (#3402850: Fix MemoryCache discovery and DX), 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 don't 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.

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 of 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. :)