Add static caching for PermissionsHashGenerator::generate()

IMO the better solution would be to statically cache the results of each cache context. Then all cache contexts benefit in the same way.

(We could key the static cache on the hash of the request it is given, to not cause a lot of pain for tests that are running multiple requests.)

Would something like this help? #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset()

@Wim so the problem with that is we need to be able to invalidate the static caches if they change. When I added a basic static cache above, our tests failed because adding a new permission to a role didn't invalidate. #4 shows a way to make the tests pass but that won't help real sites.

Well, but that's only really a problem in tests, right? Is it not okay for those few tests to call a \Drupal\Core\Cache\Context\CacheContextsManager::resetCache() method?

That'd improve performance for every single cache context, and would prevent a lot of repetitive work. I can imagine this saving hundreds of function calls easily?

I was gonna say why not just use an in-memory cache back-end… but then we don't know the necessary cache tags :P

In any case, this is a step forward, but I just feel like it'd be much better, architecturally, perf-wise and reasoning/DX-wise, to enforce that for a given request, the values for a cache context remain constant, which implies they don't need to be calculated every single time.

But, what you're saying makes sense too. It basically means that static caching should be done by every individual cache context, because we cannot do it at a higher level, because we won't know how to invalidate it.

So, let's do this.

1. +++ b/core/core.services.yml
   @@ -140,6 +140,16 @@ services:
   +  cache.backend.memory:
   

   If we're adding this back to core, then we must remove it again from sites/default/development.services.yml.

2. +++ b/core/core.services.yml
   @@ -140,6 +140,16 @@ services:
   +  # A special cache bin that by default does not persist beyond the length of the request.
   

   80 cols.

3. +++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php
   @@ -40,9 +47,10 @@ class PermissionsHashGenerator implements PermissionsHashGeneratorInterface {
   +  public function __construct(PrivateKey $private_key, CacheBackendInterface $cache, CacheBackendInterface $static) {
   

   New parameter, but docblock not updated.

4. +++ b/core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php
   @@ -150,6 +150,27 @@ public function testGenerateCache() {
   +   * Tests the generate method with static cache returned.
   

   Just @covers ::generate will do.

5. +++ b/core/tests/Drupal/Tests/Core/Session/PermissionsHashTest.php
   @@ -150,6 +150,27 @@ public function testGenerateCache() {
   +    $this->permissionsHasn = new PermissionsHashGenerator($this->privateKey, new \Drupal\Core\Cache\MemoryBackend('test'), $this->cache);
   

   s/Hasn/Hash/

   :)

berdir filed exactly the same issue, for the exact same reasons: #2509478: PermissionsHashGenerator should have a static cache. Marked it as a duplicate.

Copying his IS verbatim:

Problem/Motivation

While profiling #2498919: Node::isPublished() and Node::getOwnerId() are expensive, I noticed that PermissionsHashGenerator is called multiple times (5, in my case) and every time calls out to the cache backend. That's almost 1ms in my case.

Caching itself might be questionable. For my authenticated user with only the default permissions from the authenticated role, fetching from cache was actually slower than generating the hash. But I guess that is different if you have multiple roles and many permissions.

Proposed resolution

Add a static cache, by uid or list of roles.

Love it! RTBC + 1

#2417895: AccountPermissionsCacheContext/PermissionsHashGenerator must special case user 1, since permissions don't apply to it conflicted, rebased.

1. +++ b/core/core.services.yml
   @@ -140,6 +140,16 @@ services:
   +  cache.backend.memory:
   +    class: Drupal\Core\Cache\MemoryBackendFactory
   +    arguments: ['@cache_tags.invalidator.checksum']
   
   +++ b/sites/development.services.yml
   @@ -3,7 +3,5 @@
   -  cache.backend.memory:
   -    class: Drupal\Core\Cache\MemoryBackendFactory
   

   Removed arguments:

2. +++ b/core/core.services.yml
   @@ -140,6 +140,16 @@ services:
   +  cache.static:
   +    class: Drupal\Core\Cache\CacheBackendInterface
   +    tags:
   +      - { name: cache.bin, default_backend: cache.backend.memory }
   +    factory: cache_factory:get
   +    arguments: [memory]
   

   I think other bins are named as cache.[bin name] so replaced [memory] with [static].

3. +++ b/core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php
   @@ -182,12 +200,45 @@ public function testGenerateCache() {
   +    $this->permissionsHash->generate($this->account1);
   

   To make the test pass I changed account1 to account2 because account1 (super user) doesn't use the static cache.

Maybe a naive question, I see that Drupal\Core\Cache\Context\CacheContextsManager::convertTokensToKeys is kinda the root of the evilness here.

So as you will see there, we ask MANY things multiple times, of which the PermissionHashGenerator is just one of them by accident.
Would it not be much more effective to static cache on that heigher level?

Yes, but as often, the trick is invalidation. The permission hash generator knows that it's based on the current user. But the context manager doesn't know that the the generated user.permission varies by the current user. We could cache it by current user, but I'm not sure that will always work. We have the cacheablity metadata now, but I'm not sure that will help.

And I think this will still be called directly as well.

+++ b/core/tests/Drupal/Tests/Core/Session/PermissionsHashGeneratorTest.php
@@ -138,12 +145,15 @@ protected function setUp() {
+    $this->staticCache = $this->getMockBuilder('Drupal\Core\Cache\CacheBackendInterface')
+      ->disableOriginalConstructor()
+      ->getMock();

Quick tip: This can be just $this->getMock('Drupal\Core\Cache\CacheBackendInterface');

Did some quick profiling, this now safes 1% in total for the default frontpage as anonymous user without page cache:

### FINAL REPORT

=== 8.0.x..8.0.x compared (55a37f7567581..55a37f90df5d2):

ct  : 30,717|30,717|0|0.0%
wt  : 71,937|72,163|226|0.3%
mu  : 16,723,496|16,723,456|-40|-0.0%
pmu : 16,755,552|16,755,552|0|0.0%

=== 8.0.x..2501117 compared (55a37f7567581..55a37fa26abf8):

ct  : 30,717|30,610|-107|-0.3%
wt  : 71,937|71,277|-660|-0.9%
mu  : 16,723,496|16,761,328|37,832|0.2%
pmu : 16,755,552|16,793,400|37,848|0.2%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2501117           |     30,610 |     32,910 |     30,633 |     30,610 |     30,610 |
| 8_0_x             |     30,717 |     33,017 |     30,740 |     30,717 |     30,717 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2501117           |     71,277 |    100,554 |     75,279 |     73,877 |     80,367 |
| 8_0_x             |     72,163 |    101,668 |     75,761 |     74,842 |     81,553 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2501117           | 16,761,312 | 17,310,096 | 16,804,254 | 16,761,328 | 17,155,360 |
| 8_0_x             | 16,723,440 | 17,271,544 | 16,772,721 | 16,723,456 | 17,075,725 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| 2501117           | 16,793,336 | 17,480,560 | 16,837,677 | 16,793,400 | 17,186,680 |
| 8_0_x             | 16,755,552 | 17,442,376 | 16,806,196 | 16,755,552 | 17,106,853 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

Thanks for the tip and profiling @dawehner!

+++ b/core/modules/user/src/Tests/UserPermissionsTest.php
@@ -56,6 +57,7 @@ function testUserPermissionChanges() {
+    Cache::invalidateTags(["config:user.role.$this->rid"]);

@@ -70,6 +72,7 @@ function testUserPermissionChanges() {
+    Cache::invalidateTags(["config:user.role.$this->rid"]);

@@ -83,6 +86,7 @@ function testUserPermissionChanges() {
+    Cache::invalidateTags(["config:user.role.$this->rid"]);

It would be nice to get rid of these. How about adding MemoryBackend::reset() method that is called by WebTestBase::refreshVariables()?

#29:

But I think what we really want here is a cache tag enabled memory backend (which we already have the code for), then the cache tag invalidation would just work.

So calling MemoryBackend::reset() shouldn't be necessary. OTOH, indeed, invalidating the cache tags manually also shouldn't be necessary. Why is that?

#30: It's necessary to see any changes made in the other thread.

Here's a patch with MemoryBackend::reset().

Back to RTBC, that looks great!

I don't understand:

1. how/where MemoryBackend::reset() is being called?
2. why that is necessary at all; why aren't the cache tag invalidations caused by changing permissions (which cause Role entities to be saved, and hence the cache tags for those roles to be invalidated) sufficient?

#33: Cache tags are cached statically, hence they are only loaded / invalidated once per request.

Yes, this is correct and better.

As always, we have two runtime environments in simpletest, the test and actual requests. It's impossible to for the code in the requests to reset the static caches in the test environment, that's why we have refreshVariables() and reset config, cache tags on the checksum service and so on already.

RTBC + 1 - x-post

Ahhhhhh! I was very confused because CacheBackendInterface::reset() does not exist. So I was wondering how this could possibly ever be called automatically. But, of course, using is_callable() directly like https://api.drupal.org/api/drupal/core%21modules%21simpletest%21src%21We... does… well, that works.

Let's create a ResettableCacheBackendInterface in a follow-up?

Oh, and RTBC+1 then :)

Sorry for the distraction & confusion!

Thanks to @Berdir for pointing me to #2311945: Add ResettableCacheBackendInterface, which is the issue to do #37.

Oops, olli already linked it in #30. I had no idea based on the issue title that that was what it did. Now retitled that other issue.

+++ b/core/core.services.yml
@@ -1259,7 +1268,7 @@ services:
-    arguments: ['@private_key', '@cache.default']
+    arguments: ['@private_key', '@cache.default', '@cache.static']

Sorry, one more question... How about injecting a BackendChain (here or in a follow-up)? Isn't this an use case for it?

While reading the memory backend code I got confused that we serialize/unserialize the data there. I cannot imagine that there is a proper usecase for it.

References. If we don't, then we might end up with changing cached data by reference (objects), that's not something that you'd expect as a user of the API.

#2538956: Document that MemoryBackend::prepareItem()/::set() uses unserialize()/serialize() to break references

I added a beta evaluation.

Conceptually I think I'd prefer this to be using something like #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() because to me it looks like we're using the memory cache as a substitute for a static property that we want to be able to clear at test time.

#46: I don't understand what you mean; this issue uses the memory cache backend specifically to allow cache tag invalidations to still work. #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() does not fix that AFAICT.

But I suspect I'm overlooking/misreading something :)

Committed 9aaddd2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Added #3096986: Use CacheFastBackend for PermissionsHashGenerator.

This is somewhat related.