Problem/Motivation

The TaggedHandlersPass loops around all the service definitions many times. We call \Symfony\Component\DependencyInjection\ContainerBuilder::findTaggedServiceIds 370 odd times when installing minimal.

Proposed resolution

Build a map of all the services and tags first and use it.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3185917-5.patch2.99 KBalexpott
#5 2-5-interdiff.txt1.22 KBalexpott
#2 3185917-2.patch2.91 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3185917-2.patch2.91 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

On standard install this saves +1000 calls to ContainerBuilder::findTaggedServiceIds()

During a regular module install on standard it saves: 90 calls. And this will increase if an installed module uses the tagged service pattern.

It's not huge gains in terms of performance but it is certainly doing less.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

This looks like a neat optimisation - as you say it's small but it does remove a lot of method calls. We already had a comment hinting at the optimisation, even:

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
@@ -89,19 +96,22 @@ class TaggedHandlersPass implements CompilerPassInterface {
     // Avoid using ContainerBuilder::findTaggedServiceIds() as that we result in
     // additional iterations around all the service definitions.

Let's move this comment above the first loop and fix the grammar while we are here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-5-interdiff.txt1.22 KB
3185917-5.patch2.99 KB

@longwave good point re comment - moved and fixed.

Yep this does save many function calls - it's just that they are not particularly expensive - but every little helps.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Looks good.

  • catch committed d937a57 on 9.2.x 
    Issue #3185917 by alexpott, longwave: Optimise TaggedHandlerPass

  • catch committed 752615a on 9.1.x 
    Issue #3185917 by alexpott, longwave: Optimise TaggedHandlerPass

(...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Looks great to me too.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2472337: Provide a lazy alternative to service collectors which just detects service IDs

Nice, this further optimizes what #2472337: Provide a lazy alternative to service collectors which just detects service IDs introduced! 👍

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Performance

Status: Fixed » Closed (fixed)

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