Problem/Motivation

Working on #3464736: Replace HostEntity::isEnabledForRegistration and src/Plugin/Block/RegistrationStatus.php we identified that:

We call $host_entity->isAvailableForRegistration() 3 times in various places of the code of this block. When rendering the block we therefore call it 3 times. Which means the registration validation event is going to be fired 3 times on that request by this block alone.

it's registered as a callback. For an enabled block it is called once, and then a second time that I added. In theory I think what you are suggesting is correct, but in reality it seems the constructor is getting called more often than the build function, at least in a debugging session. Let's leave as is for now and address in a performance follow up. We may want to cache inside the host entity, if we can do that safely.

In theory we should be able to use a memory cache to safely cache validation results at the level of the validator.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork registration-3497730

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Active » Postponed
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Postponed » Active
john.oltman’s picture

john.oltman commented
Assigned: Unassigned » john.oltman

john.oltman opened merge request !107

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Nice!

  /**
   * The default cache contexts to vary every cache item by.
   *
   * Tests can change the current user or the language within a single
   * test run, so ensure results are cached per user and per language.
   *
   * @var string[]
   */
  protected array $cacheContexts = [
    'languages',
    'user',
  ];

I wonder if this runs a risk of hiding cacheability bugs.

As an alternative we could override ::setCurrentUser() in RegistrationKernelTestBase and reset the validator cache when the current user changes.

  /**
   * The default cache tags to invalidate every cache item by.
   *
   * Ensures results are recalculated any time registrations are added or
   * deleted.
   *
   * @var string[]
   */
  protected array $cacheTags = [
    'registration_list',
  ];

Shouldn't this be the responsibility of any constraint validators that depend on existing registrations?

  • john.oltman committed 3a9fcc25 on 3.3.x 
    #3497730: Add variation cache to registration validator
john.oltman’s picture

john.oltman commented
Title: Consider registration validation performance impact » Add variation cache to registration validator
Assigned: john.oltman » Unassigned
Status: Active » Fixed

Thanks! I had the same thought on the cache tags and moved that into the HasRoom validator where it belonged in the first place. The worst that the cache contexts can do is result in less caching of validation results, which won't cause any bugs, but could reduce performance. In reality, the user context will be there already, and I don't think multiple languages will come into play for the typical page request, so it shouldn't matter. If you are worried about something else please explain further. Keep in mind the cache contexts you are seeing in this commit are only added to the variation cache, not to the validation results.

john.oltman opened merge request !109

Status: Fixed » Closed (fixed)

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