Split from #3155568: Filter by bundle in EntityConverter route param converter.

Problem/Motivation

In #3155568: Filter by bundle in EntityConverter route param converter we've discovered that you cannot properly deprecate a class of a route access checker because the service is instantiated even no route is using it. Instantiating such a services is useless and has a performance cost. Fixing this issue might not bring signifiant performance improvement but it's still a win.

Steps to reproduce

n/a

Proposed resolution

  • Static access checkers: Delay the instantiation of the access checker service until the point where we test if the checker is applicable to a certain route. In this way we are only instantiating services that are in use by defined routes.
  • Dynamic access checkers: Right now the instantiation of all access checker services helps differentiate between static and dynamic services as we check if instance implements a certain interface (see CheckProvider::loadDynamicRequirementMap()). But we can do this when compiling the container rather than on runtime. Collect all service IDs of dynamic access checkers into a container parameter (dynamic_access_check_services) and use it in CheckProvider. When rebuilding the container is always better than at runtime.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

New array container parameter dynamic_access_check_services

Release notes snippet

n/a

Issue fork drupal-3183036

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Related issues: +#3155568: Filter by bundle in EntityConverter route param converter

Added #3155568: Filter by bundle in EntityConverter route param converter as related issue.

claudiu.cristea opened merge request !44

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Status: Active » Needs review

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

What a beauty of a patch. Do we need to document the container parameter somewhere? Should we collect before and after stats?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented

@jibran

Do we need to document the container parameter somewhere?

Are there other "hidden" container parameters documented? I can do it, but not sure where. But I've already added this comment:

// Collect dynamic access checker services.

Should I expand it with an explanation about its scope?

Should we collect before and after stats?

If you mean "static" by "stats", I guess it's not important. Right now is after. Moving it before does it change something?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Hehe, I do mean stats. As it is a performance issue. It would be good to look at the before and after numbers for this patch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Are there other "hidden" container parameters documented?

Please see top of core.services.yml.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue tags: +needs profiling

@jibran

It would be good to look at the before and after numbers for this patch.

Well, as I said in the IS, I don't think the performance will increase too much. It might even show no improvement as in core we don't have access checked not used by routes. However, I'll try to create a benchmark scenario.

Please see top of core.services.yml.

I think app.root and site.path are there because they are meant to be used by 3rd party code. But there are also those hidden parameters, which have an internal purpose. Normally we don't want to expose them because they are used only by core and they don't have a "public" value: authentication_providers, twig_extension_hash, language.default_values, cache_contexts, cache_bins, cache_default_bin_backends, main_content_renderers, container.modules, install_profile, container.namespaces, persist_ids and so on... All these are are container parameters but they are not meant to be used by developers in 3rd-party code. They are only for internal usage. The same is for dynamic_access_check_services.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

cache_default_bin_backends is an interesting example: #2363351: Cached services can't be used in service providers/modifiers but I assume this doesn't apply here. Agreed that internal properties that aren't mean for configuration but just a sort of cache don't need to be added to the core.services.yaml

Also think this doesn't need profiling, it is only called during a router rebuild and there's no way that even a handful of services that we don't load is a measurable gain.

Maybe just remove both the needs profiling and performance tag which I think is slightly misleading. You can't claim performance gains without proving it, but that's not the goal here anyway, the main thing is being able to properly deprecate access checks.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented
Issue tags: -Performance, -needs profiling

Remove tags

  • catch committed f992a8b on 9.3.x 
    Issue #3183036 by claudiu.cristea, jibran, Berdir: Don't instantiate...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed f992a8b and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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