Problem/Motivation

Kernel listeners are registered twice, which slows down things quite a bit as all listeners are called twice (and he might even have bad and hard to find side-effects).

Proposed resolution

This patch removes the duplication. Enjoy the performance improvement!

CommentFileSizeAuthor
free-perf-improvment.patch935 bytesfabpot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Thank you!
This issue was mentioned before, #1873436: Compiler compiles everything twice and indeed your patch fixes this problem.

I'm wondering whether this should be put into a test.

kscheirer’s picture

Status: Needs review » Needs work
Issue tags: +Performance, +Needs tests

Wow, good catch!

I think this does deserve a test, since this fixes behavior that should not have been broken in the first place. The test would be to register a listener, trigger the event, and then check that the listener is only executed once.

It's a clear performance gain I'm sure, but some kind of benchmark will go a long way to convincing core maintainers to get this patch in fast.

fabpot’s picture

I will let you work on the tests and benchmarks. I don't see the point of those as this is clearly a typo.

marcingy’s picture

This is just removing duplicate code. So the fact that tests still pass means it has tests.

Current code is

    // Add a compiler pass for registering event subscribers.
    $container->addCompilerPass(new RegisterKernelListenersPass(), PassConfig::TYPE_AFTER_REMOVING);
    // Add a compiler pass for adding Normalizers and Encoders to Serializer.
    $container->addCompilerPass(new RegisterSerializationClassesPass());
    // Add a compiler pass for registering event subscribers.
    $container->addCompilerPass(new RegisterKernelListenersPass(), PassConfig::TYPE_AFTER_REMOVING);
Crell’s picture

That's weird, as I'm not sure I ever saw listeners firing twice. Some of them would have caused fatal errors if they ran twice.

Crell’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Crosspost.

Crell’s picture

Now that I look at the actual code, not just the patch, this is a major "Duh". :-) +1 on RTBC. I don't think this needs a test or benchmark, as even without benchmarks the current code is obviously wrong.

msonnabaum’s picture

I agree that a benchmark is unnecessary. This seems more like correcting a mistake that has the side effect of a performance improvement. Best to just correct the mistake.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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