Skip to content

[Messenger] Reset peak memory usage for each message #60018

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.

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

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

TimWolla
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

PHP’s peak memory usage is a bit of global state that is not useful to keep in a long-running process handling individual self-contained messages, since a single high-memory message (handler) running early in a worker’s lifecycle will skew the numbers for all remaining messages processed by that worker.

By resetting the peak memory usage for each message it becomes possible to measure a given message type’s maximum memory usage more accurately, allowing to optimize hardware resources, for example by placing individual messages with handlers requiring a high memory-usage into their own transport that is executed on a larger worker instance.

As part of this change the cycle collection is also moved out of the Worker into an event-listener, since the cycle collection is not a core task of the Worker, since cycle collection would happen implicitly as well.

@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 21, 2025

I think the test failures are unrelated, because other PRs created around the same time also fail the same tests. But please let me know if this is related to the change in src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php, since I'm new-ish to Symfony (contributions) 😃

@nicolas-grekas
Copy link
Member

What about resolving #59788 in the process?
Removing gc_collect_cycles might be enough.
Dunno if a middleware would be more appropriate (since @stof mentioned this way in the linked issue)

@TimWolla
Copy link
Contributor Author

Thanks for the link to that issue.

Removing gc_collect_cycles might be enough.

I think having an explicit gc_collect_cycles() outside of the actual message processing is a good thing, since this avoids running the cycle collection while a message is currently being processed, introducing unexpected and unpredictable delays.

Though of course this amount if increased CPU usage is not a good thing either. Perhaps this is worth filing an issue with PHP itself? At least the documentation could more clearly indicate how expensive the cycle collector is.

Dunno if a middleware would be more appropriate

I lack the Symfony experience to say whether a middleware or an event listener would be more appropriate. Please tell me what do to here, I can:

  • Revert the changes for gc_collect_cycles(), making this PR focused on the peak memory usage reset and deferring the cycle collection solution.
  • Split this into two separate event listeners, one for the reset and the other for the cycle collection.
  • Split this into an event listener for the reset and a middleware for the cycle collection.
  • Something else?

@stof
Copy link
Member

stof commented Mar 24, 2025

Both the reset of the peak memory usage and the cycle collection could be implemented either as middlewares or as event listeners I think. but FrameworkBundle has an existing configuration to choose which middlewares to register, while making it a listener would require introducing a new configuration setting to make it configurable. That's why I suggested a middleware initially.
Another reason is that the middleware would be usable for projects using the Messenger component without its event-dispatcher integration (the event dispatcher is optional in the Worker class). This does not matter for the framework usage as FrameworkBundle always injects the event dispatcher, but it matters for standalone usage.

@TimWolla
Copy link
Contributor Author

I see, thank you. I'll have a look at updating this PR to make use of middlewares and I'll report back if I have any further questions (or an updated implementation).

@TimWolla
Copy link
Contributor Author

I've looked into this and realize that for the gc_collect_cycles() neither a middleware nor the currently chosen events would be appropriate, because the cycle collection happens before the message is ack()ed (this introducing the unintended delay into message processing).

Looking at the current worker code, the WorkerRunningEvent with a check for !$event->isWorkerIdle() would probably be best, but would come with the event drawbacks you mentioned.

@stof
Copy link
Member

stof commented Mar 24, 2025

To solve #59788, I suggest that resetting the peak memory usage and collecting cycles get implemented as 2 separate middlewares.

@TimWolla
Copy link
Contributor Author

Yes, but running gc_collect_cycles() in a middleware is not particularly useful, as I've outlined in my previous comment: This kind of “clean-up” work should ideally happen after ack'ing a message and before retrieving the next one so that the message processing is not unnecessarily blocked by the cycle collection.

@TimWolla TimWolla requested a review from nicolas-grekas April 8, 2025 13:51
@nicolas-grekas
Copy link
Member

Please rebase to get rid of the merge commit.
Here's my review as a diff actually:

diff --git a/src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php b/src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php
index c4168018bd..f5c199c530 100644
--- a/src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php
+++ b/src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php
@@ -12,35 +12,36 @@
 namespace Symfony\Component\Messenger\EventListener;
 
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
-use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent;
 use Symfony\Component\Messenger\Event\WorkerMessageReceivedEvent;
+use Symfony\Component\Messenger\Event\WorkerRunningEvent;
 
 /**
  * @author Tim Düsterhus <tim@tideways-gmbh.com>
  */
-class ResetMemoryUsageListener implements EventSubscriberInterface
+final class ResetMemoryUsageListener implements EventSubscriberInterface
 {
+    private bool $collect = false;
+
     public function resetBefore(WorkerMessageReceivedEvent $event): void
     {
         // Reset the peak memory usage for accurate measurement of the
         // memory usage on a per-message basis.
         memory_reset_peak_usage();
+        $this->collect = true;
     }
 
-    public function collectAfter(WorkerMessageHandledEvent|WorkerMessageFailedEvent $event): void
+    public function collectAfter(WorkerRunningEvent $event): void
     {
-        // Run the cycle collector after handling a message to avoid it
-        // running while the message is reserved and in processing.
-        gc_collect_cycles();
+        if ($event->isWorkerIdle() && $this->collect) {
+            gc_collect_cycles();
+        }
     }
 
     public static function getSubscribedEvents(): array
     {
         return [
-            WorkerMessageReceivedEvent::class => ['resetBefore', -1024],
-            WorkerMessageFailedEvent::class => ['collectAfter', -1024],
-            WorkerMessageHandledEvent::class => ['collectAfter', -1024],
+            WorkerMessageReceivedEvent::class => ['resetBefore', 1024],
+            WorkerRunningEvent::class => ['collectAfter', -1024],
         ];
     }
 }

@TimWolla TimWolla force-pushed the messenger-memory-reset branch from a5c3f71 to 9b908ca Compare April 8, 2025 13:52
@chalasr
Copy link
Member

chalasr commented Apr 8, 2025

Shall we backport the GC part of the patch to 6.4 to fix #59788?

/**
* @author Tim Düsterhus <tim@tideways-gmbh.com>
*/
final class ResetMemoryUsageListener implements EventSubscriberInterface
Copy link
Contributor

@94noni 94noni Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt to add some debug/info log here ? can it be usefull for ppl?

so they know if they want to "disable/opt out" this subscriber in their app

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if anyone needs this to be configurable. I don't see why it should at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, i was mostly commenting to this thx

@nicolas-grekas nicolas-grekas force-pushed the messenger-memory-reset branch from e38db5d to 896ab90 Compare April 8, 2025 14:15
@nicolas-grekas
Copy link
Member

Thank you @TimWolla.

@nicolas-grekas nicolas-grekas merged commit 99881e6 into symfony:7.3 Apr 8, 2025
3 of 10 checks passed
@TimWolla TimWolla deleted the messenger-memory-reset branch April 8, 2025 14:20
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants