-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
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 |
Thanks for the link to that issue.
I think having an explicit 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.
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:
|
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. |
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). |
I've looked into this and realize that for the Looking at the current worker code, the |
To solve #59788, I suggest that resetting the peak memory usage and collecting cycles get implemented as 2 separate middlewares. |
Yes, but running |
src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/EventListener/ResetMemoryUsageListener.php
Outdated
Show resolved
Hide resolved
Please rebase to get rid of the merge commit. 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],
];
}
} |
a5c3f71
to
9b908ca
Compare
Shall we backport the GC part of the patch to 6.4 to fix #59788? |
9b908ca
to
35c88ff
Compare
/** | ||
* @author Tim Düsterhus <tim@tideways-gmbh.com> | ||
*/ | ||
final class ResetMemoryUsageListener implements EventSubscriberInterface |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
e38db5d
to
896ab90
Compare
Thank you @TimWolla. |
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.