8000 Upgrading to symfony/messenger 6.4+ causes major CPU spike (min 2x load) because of unnecessary gc_collect_cycles() on every message · Issue #59788 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Upgrading to symfony/messenger 6.4+ causes major CPU spike (min 2x load) because of unnecessary gc_collect_cycles() on every message #59788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
YoYoShik opened this issue Feb 16, 2025 · 8 comments
Closed

Upgrading to symfony/messenger 6.4+ causes major CPU spike (min 2x load) because of unnecessary gc_collect_cycles() on every message #59788

YoYoShik opened this issue Feb 16, 2025 · 8 comments
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. Messenger Status: Needs Review

Comments

@YoYoShik
Copy link
YoYoShik commented Feb 16, 2025 •
edited
Loading

Symfony version(s) affected

6.4.0 - 7.*

Description

Use gc_collect_cycles() after every message. Works exactly when connected symfony/messenger and symfony/redis-messenger

How to reproduce

Tested with
php 8.1.8 + symfony/messenger 6.4.* + (symfony/redis-messenger 6.3.* or 6.4.* )
php 8.4.3 + symfony/messenger 7.0.* + symfony/redis-messenger 7.0.*

You won't see the difference with one handler running. Therefore, I recommend running this all on an external environment in many instances. I have so many messages that I need to run 17 handlers at the same time.

For check that everything is fine on 6.3.* version:
Install symfony/messenger 6.3.* and symfony/redis-messenger 6.3.*

  1. Create class for dispatch
<?php

namespace App\Messenger\Message\Worker;

class TestMessage {}
  1. Create empty handler only with return in __invoke()
<?php

namespace App\Messenger\MessageHandler\WorkerHandler;

use App\Messenger\Message\Worker\TestMessage;
use Symfony\Component\Messenger\Attribute\AsMessageHandler;

#[AsMessageHandler]
class TestMessagesWorkerHandler
{
    public function __invoke(TestMessage $testMessage)
    {
        return;
    }
}
  1. Config in messenger.yaml
framework:
    messenger:
        transports:
            test_message_worker:
                dsn: "%env(REDIS_DSN)%"
                options:
                    delete_after_ack: true
                    stream: "queues:test_messages_stream"
                    group: "test_messages_group"
                    consumer: "test_messages_group-%env(HOSTNAME)%"
        routing:
            'App\Messenger\Message\Worker\TestMessage': test_message_worker
  1. Implement message bus (MessageBusInterface $messageBus) in many places in you app and start dispatching
    $this->messageBus->dispatch(new TestMessage());
    And run bin/console messenger:consume test_message_worker (many instances)

Result CPU you can see on picture with mark P1

  1. I see symfony/messenger 6.4.0 Worker (Symfony\Component\Messenger\Worker). gc_collect_cycles() were added at 120 line. That's why i want test it on 6.3.0. Add it to our handler
<?php

namespace App\Messenger\MessageHandler\WorkerHandler;

use App\Messenger\Message\Worker\TestMessage;
use Symfony\Component\Messenger\Attribute\AsMessageHandler;

#[AsMessageHandler]
class TestMessagesWorkerHandler
{
    public function __invoke(TestMessage $testMessage)
    {
        gc_collect_cycles();
        return;
    }
}

Deploy and run bin/console messenger:consume test_message_worker (many instances)

Result CPU you can see on picture with mark P2
6. Now upgrade packages symfony/messenger and symfony/redis-messenger to 6.4.0 (or 6.4.*). In fact, updating symfony/redis-messenger does not cause problems as long as the symfony/messenger package is <6.4.
And remove gc_collect_cycles() from our handler. We already have it in Worker class line 120.

<?php

namespace App\Messenger\MessageHandler\WorkerHandler;

use App\Messenger\Message\Worker\TestMessage;
use Symfony\Component\Messenger\Attribute\AsMessageHandler;

#[AsMessageHandler]
class TestMessagesWorkerHandler
{
    public function __invoke(TestMessage $testMessage)
    {
        return;
    }
}

Deploy and run bin/console messenger:consume test_message_worker (many instances)

Result CPU you can see on picture with mark P3

As you see P2 and P3 has same CPU results. And this result is higher than before on 6.3.*

Image

Paragraph 5 were added to show what exactly gc_collect_cycles() affects performance

Possible Solution

Perhaps add a new option to the messenger:consume command to ignore this behavior?

Additional Context

No response
@YoYoShik YoYoShik added the Bug label Feb 16, 2025
@evmnaumov
Copy link

Have the same issue on all versions 6.4.0 (php 8.2, 8.4) and higher (7.* included). On latest 6.3.* (php 8.1) all is fine.

@YoYoShik

This comment has been minimized.

Sign in to view
@94noni
Copy link
Contributor
94noni commented Feb 19, 2025 •
edited
Loading

For your temporary hack, prefer using this https://github.com/cweagans/composer-patches its better imho :)

Also, try opening a PR to make this gc call optional perhaps if you are sure its your culprit here?

I use messenger but with amqp (prod) and doctrine (dev) and l didnt spotted this (or at least my devops teamate)

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Feb 19, 2025 •
edited
Loading

Sharing hacks isn't very useful. They will stay forever and will mislead readers, even after a solution is found.
Better spend time figuring out a solution.

So, this call has been added in #52253.
Looks like we were missing input about the CPU impact.

I don't know a GC strategy that'd work for everybody, so we might need to make this optional.
One idea could be to add a handler that does call the GC when some GarbageCollectMessage is dispatched.
A Scheduler task could the dispatch those messages.

Another idea: have an optional (userland ?) event listener that implements the required logic to call the GC.

For now I'd be fine reverting #52253.
WDYT @jwage?

@jwage
Copy link
Contributor
jwage commented Feb 21, 2025

@nicolas-grekas I am fine reverting it. I can handle this in my codebase in another way that doesn't require it to be in core.

@stof
Copy link
Member
stof commented Feb 21, 2025

Instead of making the Worker class itself trigger the GC, I suggest implementing (in core) a middleware triggering it. Then, projects wanting to trigger the GC between each message can enable the middleware with 1 line in their config.
This would be similar to several other middlewares implemented in the core.

@chalasr chalasr added the Help wanted Issues and PRs which are looking for volunteers to complete them. label Mar 6, 2025
@nicolas-grekas nicolas-grekas mentioned this issue Mar 21, 2025
Merged
@nicolas-grekas
Copy link
Member

Let's remove the call on 6.4/7.2 and improve on 7.3 thanks to #60018
Anyone up for the PR on 6.4?

@jwage
Copy link
Contributor
jwage commented Apr 21, 2025

@nicolas-grekas I will submit a PR to revert the change.

@jwage jwage mentioned this issue Apr 21, 2025
Merged
fabpot added a commit that referenced this issue Apr 21, 2025
@fabpot
bug #60248 [Messenger] Revert " Add call to gc_collect_cycles() aft…
368bfb7 
…er each message is handled" (jwage)

This PR was merged into the 6.4 branch.

Discussion
----------

[Messenger] Revert " Add call to `gc_collect_cycles()` after each message is handled"

This reverts commit b0df65a.

Reverting changes introduced in #52253 because of the issue reported here #59788

Commits
-------

fc9c551 Revert "[Messenger] Add call to `gc_collect_cycles()` after each message is handled"
@fabpot fabpot closed this as completed Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Help wanted Issues and PRs which are looking for volunteers to complete them. Messenger Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@fabpot @jwage @nicolas-grekas @stof @94noni @xabbuh @chalasr @carsonbot @evmnaumov @YoYoShik
0