Skip to content
Snippets Groups Projects
Commit c89b8f33 authored by Sean Blommaert's avatar Sean Blommaert Committed by Kristiaan Van den Eynde
Browse files

Issue #2895988 by kristiaanvandeneynde, seanB, opdavies, catch, bojan_dev,...

Issue #2895988 by kristiaanvandeneynde, seanB, opdavies, catch, bojan_dev, erlendoos: Improve performance of the membership loader
parent 4bab4adc
No related branches found
No related tags found
2 merge requests!193Issue #3304728 by kristiaanvandeneynde: Add member page missing due to...,!111Issue #3397021 by adamfranco: Add entity_mappings for phpstan-drupal
Pipeline #181303 passed with warnings
......@@ -41,6 +41,26 @@ services:
tags:
- { name: 'access_check', applies_to: '_group_latest_revision' }
cache.group_memberships_memory:
class: 'Drupal\Core\Cache\MemoryCache\MemoryCacheInterface'
tags:
- { name: 'cache.bin.memory', default_backend: 'cache.backend.memory.memory' }
factory: ['@cache_factory', 'get']
arguments: ['group_memberships_memory']
cache.group_memberships:
class: 'Drupal\Core\Cache\CacheBackendInterface'
tags:
- { name: 'cache.bin' }
factory: ['@cache_factory', 'get']
arguments: ['group_memberships']
cache.group_memberships_chained:
class: 'Drupal\Core\Cache\BackendChain'
calls:
- ['appendBackend', ['@cache.group_memberships_memory']]
- ['appendBackend', ['@cache.group_memberships']]
tags:
- { name: 'cache.bin.memory' }
cache_context.route.group:
class: 'Drupal\group\Cache\Context\RouteGroupCacheContext'
arguments: ['@current_route_match', '@entity_type.manager']
......
......@@ -2,6 +2,8 @@
namespace Drupal\group\Entity;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\group\Entity\Storage\GroupRoleStorageInterface;
......@@ -68,15 +70,43 @@ trait GroupMembershipTrait {
*/
public static function loadSingle(GroupInterface $group, AccountInterface $account) {
$storage = \Drupal::entityTypeManager()->getStorage('group_content');
$cache_backend = \Drupal::service('cache.group_memberships_chained');
assert($cache_backend instanceof CacheBackendInterface);
// Get the same CID as ::loadByUser without roles.
$cid = static::createCacheId([
'entity_id' => $account->id(),
'roles' => 'any-roles',
]);
if ($cache = $cache_backend->get($cid)) {
if (empty($cache->data) || empty($cache->data[$group->id()])) {
return FALSE;
}
return $storage->load($cache->data[$group->id()]);
}
$ids = $storage->getQuery()
->accessCheck(FALSE)
->condition('gid', $group->id())
->condition('entity_id', $account->id())
->condition('plugin_id', 'group_membership')
->execute();
// Prime the cache by loading all of the user's memberships. For now, it
// seems like there's a higher likelihood of us needing all of them rather
// than a few individual ones. If we load them one by one, we have to fire
// multiple entity queries, which incurs a rather big performance hit.
//
// We choose to prime the cache by calling ::loadByUser over ::loadByGroup
// because a group could have a large amount of members. If you have a user
// with a large amount of memberships, you should check whether you can
// optimize this by making better use of insider and outsider roles.
//
// If loading all of the memberships turns out to happen quite often when
// we do, in fact, only need one or two, then we should revisit this.
$memberships = static::loadByUser($account);
foreach ($memberships as $membership) {
assert($membership instanceof GroupRelationshipInterface);
if ($membership->getGroupId() === $group->id()) {
return $membership;
}
}
return $ids ? $storage->load(reset($ids)) : FALSE;
return FALSE;
}
/**
......@@ -84,6 +114,20 @@ trait GroupMembershipTrait {
*/
public static function loadByGroup(GroupInterface $group, $roles = NULL) {
$storage = \Drupal::entityTypeManager()->getStorage('group_content');
$cache_backend = \Drupal::service('cache.group_memberships_chained');
assert($cache_backend instanceof CacheBackendInterface);
$cid = static::createCacheId([
'gid' => $group->id(),
'roles' => $roles ?? 'any-roles',
]);
if ($cache = $cache_backend->get($cid)) {
if (empty($cache->data)) {
return [];
}
return $storage->loadMultiple($cache->data);
}
$query = $storage->getQuery()
->accessCheck(FALSE)
......@@ -94,8 +138,11 @@ trait GroupMembershipTrait {
$query->condition('group_roles', (array) $roles, 'IN');
}
$ids = $query->execute();
return $ids ? $storage->loadMultiple($ids) : [];
$cacheability = (new CacheableMetadata())
->addCacheTags(['group_content_list:plugin:group_membership:group:' . $group->id()]);
$cache_backend->set($cid, $ids = $query->execute(), $cacheability->getCacheMaxAge(), $cacheability->getCacheTags());
return $storage->loadMultiple($ids);
}
/**
......@@ -103,11 +150,25 @@ trait GroupMembershipTrait {
*/
public static function loadByUser(AccountInterface $account = NULL, $roles = NULL) {
$storage = \Drupal::entityTypeManager()->getStorage('group_content');
$cache_backend = \Drupal::service('cache.group_memberships_chained');
assert($cache_backend instanceof CacheBackendInterface);
if (!isset($account)) {
$account = \Drupal::currentUser();
}
$cid = static::createCacheId([
'entity_id' => $account->id(),
'roles' => $roles ?? 'any-roles',
]);
if ($cache = $cache_backend->get($cid)) {
if (empty($cache->data)) {
return [];
}
return $storage->loadMultiple($cache->data);
}
$query = $storage->getQuery()
->accessCheck(FALSE)
->condition('entity_id', $account->id())
......@@ -117,8 +178,41 @@ trait GroupMembershipTrait {
$query->condition('group_roles', (array) $roles, 'IN');
}
$ids = $query->execute();
return $ids ? $storage->loadMultiple($ids) : [];
$cacheability = (new CacheableMetadata())
->addCacheTags(['group_content_list:plugin:group_membership:entity:' . $account->id()]);
// Cache the IDs by group ID, so we can use this cache in ::loadSingle().
$cached_ids = [];
foreach ($memberships = $storage->loadMultiple($query->execute()) as $membership) {
assert($membership instanceof GroupRelationshipInterface);
$cached_ids[$membership->getGroupId()] = $membership->id();
}
$cache_backend->set($cid, $cached_ids, $cacheability->getCacheMaxAge(), $cacheability->getCacheTags());
return $memberships;
}
/**
* Creates a cache ID based on provided values.
*
* @param array<string, mixed> $values
* A group of values that were used to filter, keyed by an identifier.
*
* @return string
* The cache ID.
*/
protected static function createCacheId(array $values) {
ksort($values);
$cid_parts = ['group_memberships'];
foreach ($values as $key => $value) {
if (is_array($value)) {
sort($value);
$value = implode('.', $value);
}
$cid_parts[] = $key . '[' . $value . ']';
}
return implode(':', $cid_parts);
}
}
......@@ -319,7 +319,7 @@ class GroupRelationship extends ContentEntityBase implements GroupRelationshipIn
public function getListCacheTagsToInvalidate() {
$tags = parent::getListCacheTagsToInvalidate();
$group_id = $this->get('gid')->target_id;
$group_id = $this->getGroupId();
$plugin_id = $this->getRelationshipType()->getPluginId();
$entity_id = $this->getEntityId();
......
......@@ -5,6 +5,7 @@ namespace Drupal\Tests\group\Kernel;
use Drupal\Core\Session\AccountInterface;
use Drupal\group\Access\GroupPermissionCheckerInterface;
use Drupal\group\Entity\GroupInterface;
use Drupal\group\Entity\GroupMembership;
use Drupal\group\Entity\GroupMembershipInterface;
use Drupal\group\Entity\GroupRoleInterface;
use Drupal\group\Entity\GroupTypeInterface;
......@@ -68,7 +69,7 @@ class GroupMembershipTest extends GroupKernelTestBase {
parent::setUp();
$this->account = $this->createUser();
$this->groupType = $this->createGroupType();
$this->groupType = $this->createGroupType(['creator_membership' => FALSE]);
$this->groupRoleInsider = $this->createGroupRole([
'group_type' => $this->groupType->id(),
'scope' => PermissionScopeInterface::INSIDER_ID,
......@@ -163,4 +164,67 @@ class GroupMembershipTest extends GroupKernelTestBase {
$this->assertSame($expected, $this->groupMembership->hasPermission('edit group'));
}
/**
* Tests the loading of a single membership.
*
* @covers ::loadSingle
*/
public function testLoadSingle() {
$membership = GroupMembership::loadSingle($this->group, $this->account);
$this->assertSame($this->groupMembership->id(), $membership->id());
// Check non-matching retrievals.
$this->assertFalse(GroupMembership::loadSingle($this->group, $this->createUser()));
$this->assertFalse(GroupMembership::loadSingle($this->createGroup(['type' => $this->groupType->id()]), $this->account));
$this->assertFalse(GroupMembership::loadSingle($this->createGroup(['type' => $this->groupType->id()]), $this->createUser()));
}
/**
* Tests the loading of all memberships of a group.
*
* @covers ::loadByGroup
*/
public function testLoadByGroup() {
$expected = [$this->account->id()];
// Add a new member to the group twice to check that caches don't break.
for ($i = 0; $i < 2; $i++) {
$account = $this->createUser();
$expected[] = $account->id();
$this->group->addMember($account);
$account_ids = [];
foreach (GroupMembership::loadByGroup($this->group) as $membership) {
assert($membership instanceof GroupMembershipInterface);
$account_ids[] = $membership->getEntityId();
}
$this->assertSame($expected, $account_ids);
}
}
/**
* Tests the loading of all memberships of a user.
*
* @covers ::loadByUser
*/
public function testLoadByUser() {
$expected = [$this->group->id()];
// Add a membership for the user twice to check that caches don't break.
for ($i = 0; $i < 2; $i++) {
$group = $this->createGroup(['type' => $this->groupType->id()]);
$expected[] = $group->id();
$group->addMember($this->account);
$group_ids = [];
foreach (GroupMembership::loadByUser($this->account) as $membership) {
assert($membership instanceof GroupMembershipInterface);
$group_ids[] = $membership->getGroupId();
}
$this->assertSame($expected, $group_ids);
}
}
}
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment