πŸ‡§πŸ‡ͺBelgium @kristiaanvandeneynde

Antwerp, Belgium
Account created on 31 May 2011, over 12 years ago
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

That's a great tip, thanks. I'll remove it once we're fully done with the performance stuff.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I've been doing that for the past few days :D

It keeps complaining because we're touching the performance tests here and both @catch and I have been hard at work optimizing those tests. So it keeps throwing merge conflicts here whenever one of those issues is committed.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Feel free to set back to RTBC if this is sufficient.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Looks good to me too. The way it's implemented makes a lot of sense to me.

It will only work for browser tests, but I can't immediately think of a scenario where we would worry about session gc during kernel tests.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Made the two methods static

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Just a small nitpick, we're returning early twice with basically the same conditions.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

More specifically, I was thinking about:

  • Only the top of CacheTagsChecksumDecorator::invalidateTags() because CacheTagsChecksumTrait already short-circuits if the list of tags is empty.
  • The top of CacheTagsChecksumDecorator::getCurrentChecksum()
  • The top of CacheTagsChecksumTrait::getCurrentChecksum() because it runs a bunch of code that will end up doing nothing if the list of tags is empty
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Sooooo... a follow-up for getCurrentChecksum and invalidateTags then? :D

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I noticed we're only changing isValid(), what about the other two methods we're tracking in our perfrmance tests?

Re #11, not really:

    $query_tags = array_diff($tags, array_keys($this->tagCache));
    if ($query_tags) {
      $tag_invalidations = $this->getTagInvalidationCounts($query_tags);
      $this->tagCache += $tag_invalidations;
      // Fill static cache with empty objects for tags not found in the storage.
      $this->tagCache += array_fill_keys(array_diff($query_tags, array_keys($tag_invalidations)), 0);
    }

If $tags is empty, then so will $query_tags so we weren't running a query for that. It was showing up in OpenTelemtry, though, because we were tracking all calls in the decorator. This patch fixes that so you'll see a significantly reduced amount of them being tracked.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Starting to wonder how much value there is in chasing the performance tests here until {#3421164] lands.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Well yes mr. bot I was in the middle of things here...

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Updated the MR, πŸ› Call refreshVariables() where needed in various tests RTBC is still pending commit to make the MR smaller. Also the performance impact as to what queries changed will be far more visible once we commit πŸ“Œ Log every individual query in performance tests Needs work .

Still in favor of deleting RenderCacheTest as a whole because the user.permissions test case is no longer valid and the only other test case (user.roles) will soon become invalid too after a recent discussion we had where said cache context should not be returning a magic string for UID 1. Issue is pending for that one.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Other issue got committed, so dropping the PP tag and removing draft status from the MR.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so after skimming this:

  1. I like the idea
  2. I like the use of modern php stuff, but am hesitant to see functions yield arrays with multiple pieces of information.
  3. I'm not sure why you would use tagged services and then still allow people to specify callbacks in their permissions.yml files. To me it feels like we should use one or the other, not both. I'm personally in favor of tagged services.
  4. For the time being I would allow and process both ways for BC reasons but throw a deprecation warning when callbacks are still detected in permissions.yml files.
  5. Disclaimer: I haven't checked ✨ Directory based automatic service creation Needs review nor the 4 issues linked in the IS yet and maybe that might change some of the above points.

All in all I really appreciate you working on this as I fully agree what we currently have isn't modern at all. Listing callbacks inside yaml files feels a bit outdated.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Regarding declare(strict_types = 1); vs declare(strict_types = 1); I found >100 and ~90 use cases of them respectively. So unless we want to standardize on those I'd just leave it as is. PhpStorm seemed to manually add the spaces on my end.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Come to think of it, these empty checks would explain the high number of cache tag operations we're seeing. Implementing code knows it's safe to call these operations with empty arrays and therefore does not bother checking how many tags there are before sending the (empty) list to the respective methods.

If we were to run an empty check before we call any operation, the number would go down drastically.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Switched to enum/match and also made sure that we use the name property to get a strign we can send to OpenTelemetry as I've read enums do not support __toString().

Furthermore made sure we actually send the cache tag stuff to OT (in our case Grafana) too and found something interesting. We are getting a bunch of calls where no tags were provided. See attached screenshot below. Only the one at the bottom actually had any tags, many of the resulting spans had none.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I also excluded user.group_permissions

That was for Group v1, for v2/v3 I wouldn't recommend turning it off but rather switching to more optimized use of insider/outsider roles, like you did.

I feel like this might become very specific to your project and I am curious as to what might be causing your performance issues. We are currently running a large website ourselves with Group and the query performance isn't that bad. However, my bandwidth for these specific type of problems is rather limited when it comes to Factorial-sponsored time.

If you can convince your managers, Factorial could do a more in-depth review of your installation. If it turns out to be Group related we can then contribute a fix back to the community, if it's specific to your installation we can try to fix it. Either way, we could free up some dedicated time then to help solve your performance issues.

Feel free to send a mail to hello@factorial.io and/or kristiaan@factorial.io if you want us to do an in-depth review of your installation. If your managers don't feel like that, then you can always open a separate support request with as much detail you can provide and we can try to fix this at an admittedly far slower pace.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Yeah, let's go with match(). I do agree it's far more readable.

I withdrew my objection in the edit above. I just feel like this is a regression to oldschool php where things have magic meanings. How would you teach the difference now between switch() and match() if the latter can be duckpunched into behaving like the former :)

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

While a match() definitely looks nicer and is more concise, I'm worried about the fact that we're not using it the way it was intended. We're executing an increment in a place where you're supposed to return a value and I'm not sure if php will keep allowing that for all eternity.

Would you agree with a switch() instead?

        switch($operation) {
          case CacheTagMetrics::getCurrentChecksum:
            $cache_tag_checksum_count++;
            break;

          case CacheTagMetrics::isValid:
            $cache_tag_is_valid_count++;
            break;

          case CacheTagMetrics::invalidateTags:
            $cache_tag_invalidation_count++;
            break;

          default:
            throw new \LogicException('Uncaught cache tag operation found.');
        };

The exception also takes care of your concern that we'd forget to count a newly monitored operation. I mean, it's definitely more verbose than the match() statement, but semantically it seems more correct?

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ made their first commit to this issue’s fork.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

During one of many tests locally, StandardPerformanceTest::testAnonymous() failed on the checksum count:
150 is greater or equal to 143 and is smaller or equal to 146

This happened on only one out of 10ish test runs

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

All performance tests go green with exact query counts on my local now. I also removed the query aliasing in favor of parameter aliasing.

However, on one occasion, StandardPerformanceTest::testLoginBlock() had one extra query as the second-to-last query:

This was expected
      'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
      'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.cron_last" ) AND "collection" = "state"',

This actually happened
      'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
      'DELETE FROM "sessions"  WHERE "timestamp" < "TIMESTAMP"',
      'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.cron_last" ) AND "collection" = "state"',
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I applied the nits, the only two things still left for discussion are:

  • Do we keep using microtime()? -> Broader than this issue, should perhaps discuss in a follow-up
  • Should we switch to enum? -> Fine by me, but how do we use match() then?
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Tests go green with exact query counts, will try and do the same for Umami tests tomorrow.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Still RTBC for all I care as you can fix on commit, but minor copy-paste mistake in a comment.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Merged in your work, will see what testbot thinks.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Will merge in the changes from πŸ“Œ Investigate variable database query counts in performance tests Active and tag this issue as PP-1 if the other hasn't been committed by then.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Looks good to me. Tests also went green on my local and the changes will help me in reducing the expected results of πŸ“Œ Log every individual query in performance tests Needs work

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

In the process of setting up a new laptop but once I'm settled in I'll review the other issue and merge it into this one

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Please open a new issue with steps to reproduce on a clean install. This issue has been closed 8 months ago.

if we have an outside role assigned to more than 1 Drupal role

Did you do this through code because that's unintended and the UI doesn't allow that.

The workaround I can think of is to write a script to make people members of all the groups so they can see the groups even when they are not administrator.

That would completely kill your site's performance

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Oops, cross-posted.

Okay so the final fail I'm seeing is related to a block ID being random (e.g.: "config:block.block.jf547b5k"8). But that only happens in a cache tag invalidation and as I wrote above we can stop tracking those as queries and simply do a count on them without being too specific.

So StandardPerformanceTest looking really good now. Need to apply the same approach to the Umami performance tests. But I'm on vacation as of right now, so will have to wait until next week :)

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Seeing a lot of SELECT "tag", "invalidations" FROM "cachetags" WHERE "tag" IN . We could perhaps track those separately like we do with cache gets, sets, etc. by checking for the calling class being DatabaseCacheTagsChecksum.

Just like we already check for DatabaseBackend to only log queries that didn't come from the DB cache, we could add in DatabaseCacheTagsChecksum to not track those queries either, then decorate cache_tags.invalidator.checksum and add some tracking in there.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

So the MR is a proof of concept.

  1. The goal is to allow us to track queries that are highly variable and we don't really care about with aliases: watchdog_insert, sessions_insert, sessions_select, etc. We still see what happened, but without the detail.
  2. Then, we replace highly variable arguments such as timestamp, IP address, username, etc. with capitalized keywords, such as TIMESTAMP, CLIENT_IP, etc.

This allows us to build a list of expected queries and compare it to what was actually tracked. We may need to sort the queries before comparing, but so far on my local they have always been in the same order.

NW because this isn't finished yet.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Needs CR and adjust code to point to said CR when we are ready to tag a release.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Seems good to me now, will set to postponed as discussed earlier.

WARNING do not use this on D10.2 or lower as the MemoryCache is not properly recognized there yet and will fall back to the default backend, often DB.

Phpcs can be ignored because by the time we get D10.3, Coder should have a new release and it includes πŸ“Œ Allow _construct() method to omit docblock Needs review

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Going to try and fix this in 10.3 using MemoryCache and then postpone the issue until that version of core is released.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Currently, the following calls isActive():

  • The route access check for turning admin mode on or off
  • The toolbar admin mode switcher
  • The access policy (only runs if your permissions weren't retrieved from cache)
  • The 'user.in_group_sites_admin_mode' cache context

So at best, you only call isActive() once: for the cache context. Then, based on the result it retrieves the right toolbar item and your group permissions from cache. At worst, you call it three times if your permissions weren't cached, because the switching callbacks don't render the toolbar so it's either one or the other.

I'm on the fence. Calling the private temp store between 1 and 3 times doesn't seem that crucial to warrant caching the result. But if custom code is also interested in it, it might bump that to 3 - 6 times. Any higher and I would still tell you look into the custom code first.

I'm just wondering if I am fully onboard with using a mere property here. Perhaps a memory cache would be better so we can reset it in the browser tests.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I'm inclined to reject this patch. Sure it adds caching using a property, but the question is whether we need caching to begin with?

If you call ::isActive() more than only a few times during a request, you really ought to consider why you're calling it that often. You could cache the result in your implementing code or, better yet, cache the outcome of why you needed to check the admin mode. If you're calling ::isActive() from multiple places, then you should probably refactor your code so that the admin mode dependency is more centralized.

Either way, I'm not opposed per se, it's just that I don't see the overall value compared to the downsides:

  • Potential stale "cache" property
  • Difficult to test across multiple requests (aka browser tests)

The current test fails are because the static property is not affected by drupalGet() (which is another request) and therefore reports incorrect values according to the test cases.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

While the changes initially looked acceptable to me, they break the tests. Could be that the tests simply need adjusting then, but setting to NW.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

GitLab CI might run in subdirectories, leading the the failures we saw here.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

No worries, hopefully you'll see a significant performance boost once you have your roles set up properly!

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so that works, asking around what changed in core that would explain the test failures on GitLab CI.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

WTF I just checked that RedirectResponse contains "/admin/group/sites/settings", which works when running tests locally and somehow fails on Drupal CI. Gonna try and trigger this another way.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

kristiaanvandeneynde β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

unless there are any more objections: could we get this committed please? It's the last soft blocker for πŸ“Œ Implement the new access policy API Needs work , which I'm really hoping to land in 10.3

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Sorry it became really busy right after I posted this question. Thanks for taking the time to address my concerns.

While I don't fully agree with the architectural decision to make one config entity behave as a "sort of bundle entity" of another config entity, I can appreciate that you do not wish to carry out "huge architectural changes" at this point in the module's lifetime. I managed to work my way around the issue in our project using your API quite easily, so it's not like it's an insurmountable obstacle.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I have a very simple view that looks at the published pages of a certain article content type (sierraarticle). It is not part of group, the article content type is not installed on any group type.

Which is correct because group does not add that group_node plugin to the query.

However, it seems like you do have about 7 other node types installed on your group types and those do need to be added to the query, even if it doesn't make sense to you. All Group gets from core is "this is a node query", not "this is an article query", so Group has to run its access checks on the node types that are grouped.

Thos numbers are the group IDs, and are repeated again for each group plugin node type.

Are you using any extra module that hands out permissions on individual group levels? Because it seems like your calculated permissions have a lot of entries for the individual scope. if someone only has maybe one or two memberships with individual roles assigned, they should only see one or two group IDs in that query.

I am not seeing any group type checks in your query at all, indicating no synchronized permissions (inside or outsider) have been handed out whatsoever.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Many of our content types are not used by group

Those do not make it into the query unless you installed them on a group type.

and many views do not including any group content.

Doesn't matter, if the view contains nodes and nodes could be shown/hidden by Group then Group will attach itself to the query. If it's a View for an admin UI where you know the people viewing it have full access, turn off query rewriting for the view.

And in those views, I am seeing giant "gcfd"."gid" IN () for every group node plugin

This means you have a large amount of "individual" group roles assigned, see if you can get rid of many of these memberships in favor of synchronized roles (outsider/insider). They will drastically speed up your queries.

Finally, it would really help to see these slower sites in action, because I've got a feeling that a large part of the performance hit is due to a misconfiguration of roles and plugins.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

2 out of 3 side quests from #47 got committed, so this MR just got smaller :)

Merged in 11.x and I think I adjusted performance values correctly, will keep an eye on results. If we could now get πŸ› Call refreshVariables() where needed in various tests RTBC committed, then I think this MR would become roughly as small as it can be.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

That would be acceptable to me. We shouldn't hold up this issue over a policy discussion about what to do when nothing is wrong.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Looks good to me, will accept once we can run pipelines.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Sweet, awesome that you were able to make the switch

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Re #49 Sounds about right. I also summarized it as such in #33

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I don't see it either, to me it shows as the last argument and is marked as optional. If #222 was a mistake than feel free to set back to RTBC

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

From a code perspective this looks good. Conceptually, I haven't been involved in this discussion at all so I'm still rolling with the approval given in #201

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

To be fair, now that I've taken a closer look and that you're checking the tagged services for the invalidator interface, I'm less fussed about it. The code makes it clear that you're not interested in the cache backends as caches, but rather as services that might implement the cache tags invalidator interface.

Just poked @catch on Slack and he also seems to be fine with it under the circumstances presented here. Sorry about the previous noise but seeing the two types of cache backends mixed together in one property automatically raised a red flag with me. After further review, it's actually quite OK the way you implemented it.

RTBC

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Sorry, in my latest review comment I misread a few lines. The property can be typehinted as such as you check for the interface when the service is collecting the tagged services.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

As I mentioned in the review, will ask for a second opinion on whether we need to be this strict with keeping the two separated. Still NW because of the property typehint suggesting something that may not be true.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

This looks fine conceptually, still NW per my latest comment, though.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

I feel like #27.1 was never addressed. My assumption is also that if CacheableMetadata::merge() has $result = clone $this;, then it seems kind of moot to assert($result instanceof CacheableMetadata);.

Comment #28 also touched on the ampersand but not sure if we really need that.

From a cache submaintainer point of view, I see nothing wrong here. Assertions are harmless, casting casche context return values to strings is too because worst case scenario that leads to a few cache entries no longer being reachable, at which point they get recalculated and stored again. Only been subsystem maintainer for a few months now, so will ask @catch before removing tag.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Not going to weigh in on whether this can be approved as @catch has already done so, but setting back to NW for a small nitpick. Feel free to RTBC again once that is fixed.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

We cannot mix cache bins and memory cache bins as the latter do not 100% behave as a cache bin. For more info, see πŸ“Œ Fix MemoryCache discovery and DX Needs review

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so kind of chasing performance tests now as they are sadly not as exact as we would like them to be. We'll fix this for sure, but in the meantime please focus on the actual code and ignore the performance tests counts.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Makes me wonder if we should split cache tags out from database queries too in the assertions so it's easier to see when that's the thing that changes.

I think that would be a great quality of life improvement.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Updated MR to what I think is right after merge.

Also, the small issues from #47 are all RTBC now, so would be great if we could commit those to make the MR here smaller.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Summary of the above: Seems like it's significantly faster on anonymous sessions and sometimes slower on authenticated ones.

Also, not going to comb over the other result sets as it takes a long time to do so. But I can post the exports here for someone else to take a look at.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so tests results are in.

On my shitty laptop, performance was as follows:

  • StandardPerformanceTest::testAnonymous(), part 1: 2.32s -> 1.59s (31% faster)
  • StandardPerformanceTest::testAnonymous(), part 2: 446ms -> 419ms (6% faster)
  • StandardPerformanceTest::testAnonymous(), part 3: 319ms -> 245ms (23% faster)
  • StandardPerformanceTest::testLogin(): 227ms -> 226ms (same)
  • StandardPerformanceTest::testLoginBlock(): 255ms -> 332ms (30% slower)

So performance for anonymous users went up across the board and was worse for one login test case out of two.

Here is the difference on StandardPerformanceTest::testLoginBlock, because that one was slower.

So if we reduce that to just the entries that are unique, we get (cache gets):

  1. get access_policy, CID "access_policies:drupal:[user.is_super_user]=0:[user.roles]=anonymous"
  2. get access_policy, CID "access_policies:drupal:[user.is_super_user]=0:[user.roles]=authenticated"

Compared to:

  1. getMultiple config, CID "user.role.anonymous"
  2. get bootstrap, CID "user_permissions_hash:anonymous"
  3. getMultiple config, CID "user.role.authenticated"

And query-wise:

  1. SELECT "tag", "invalidations" FROM "test51561594cachetags" WHERE "tag" IN ( "access_policies", "config:user.role.anonymous" )
  2. SELECT "name", "value" FROM "test51561594key_value" WHERE "name" IN ( "system.private_key" ) AND "collection" = "state"
  3. SELECT "tag", "invalidations" FROM "test51561594cachetags" WHERE "tag" IN ( "access_policies", :"config:user.role.authenticated" )

Compared to:

  1. SELECT "tag", "invalidations" FROM "test99820655cachetags" WHERE "tag" IN ( "entity_types" )
  2. SELECT "tag", "invalidations" FROM "test99820655cachetags" WHERE "tag" IN ( "config:user.role.anonymous" )
πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Looks good to me. Hopefully it stays with these 2.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay this bot is starting to annoy me

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Only fail is because of πŸ“Œ Adjust StandardPerformanceTest to avoid random failures Needs review , currently working on comparing the telemtry data from pre-patch and post-patch to get an accurate description of the performance changes.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Not for StandardPerformanceTest, but rather OpenTelemetryAuthenticatedPerformanceTest: https://git.drupalcode.org/issue/drupal-3417362/-/jobs/709685

This one failed even though no real code was changed in that MR< only test code in unrelated tests. Should we also fix that here?

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Silly bot.

Created second MR for the 11.x version

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Created a second branch as setting the current one to 11.x led to a massive amount of changes (due to it being created from 10.2.x)

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Currently failing because we allow neither 38 nor 39, adding suggestions to MR

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Thanks for the review!

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Yup, same like other issue: thought it could also go in 10.2 as it's a bug report.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Yeah sure, thought this could be in 10.2 and 11.x as it's a bugfix.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so most recent performance values are in:

  • OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache()
    • Query count goes up by 1
    • Cache get count goes down by 1
  • StandardPerformanceTest::testAnonymous()
    • Query count goes up by 1
    • Cache get count goes down by 1
  • StandardPerformanceTest::testLogin()
    • Query count goes up by 2
    • Cache get count goes down by 1
  • StandardPerformanceTest::testLoginBlock()
    • Query count goes up by 2
    • Cache get count goes down by 2

Not sure why this is different from #42 but I get the same values locally so I guess they're correct.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

All 3 spin-off issues from #47 have a green MR and are easy to review and commit.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Bumping this one to major given the time sensitivity. If we do not land this before 10.3 is released, it might become really hard to land at all.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Hmm cache get count went down by one all of a sudden. Adjusted MR but this might spell trouble. Ignore the commit message mentioning query count, that was me not being properly awake yet.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Splitting off some low-hanging fruit to make this MR smaller, will update comment as I go:

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Oh, right forgot the new performance testing now disregards queries coming from cache sets/gets. Could definitely be cache tags, yeah. If we want a definitive answer we'd have to compare the current output vs output when running this on ddev with your tools.

Either way, the main difference between old code and new code is what I wrote in #44. Your permissions now come from potentially many sources and we need to gather them from said sources at least once until we cache it. At that point we have cache redirects to follow, depending on the complexity.

So I'd start looking there to see what changed, but then again your tool might simply tell us which queries are gone and which are new.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

The calculated permissions are stored in the DB using VariationCache so that we can actually have access policies (impossible without cache contexts). Before, we'd load your user roles and be done with it, which was always one query. Now we load your calculated permissions and, depending on how variable those are, follow a cache redirect or two to get to the final ones.

So I'm almost 100% sure the extra queries are cache redirects being followed.

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Okay so new performance values are in:

  • OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache() query count goes up by 1
  • StandardPerformanceTest::testAnonymous() query count goes up by 1
  • StandardPerformanceTest::testLogin() query count goes up by 2
  • StandardPerformanceTest::testLoginBlock() query count goes up by 2

Cache get count stays the same across the board

πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

Tried to apply the workaround from πŸ› Spell-checking job fails with "Argument list too long" when too many files are changed Active so we can run tests here.

Production build https://api.contrib.social 0.61.6-2-g546bc20