Improve memory consumption by cleaning up unneeded references #32
10000
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While debugging some very odd memory issues in a live application, I noticed that this component shows some unexpected memory consumption and memory would not immediately be freed as expected. Let's not call this a "memory leak", because memory was eventually freed, but this clearly caused some unexpected and significant memory growth.
One of the core issues has been located and addressed via reactphp/event-loop#164, but even with that patch applied the
resolve()method behaved a bit unexpected.I've used the following script to demonstrate unreasonable memory growth:
Initially this peaked at around 320 MB on my system. After applying the referenced patch, this went down significantly and fluctuated somewhere between 1 MB and 12 MB. After applying this patch, this script reports a constant memory consumption of around 0.7 MB.
This implementation includes some of the ideas discussed in reactphp/promise#46 and reactphp/socket#113. Eventually, we should look into providing a way to address this within our promise implementation.
My vote would to be get this in here now as it addresses a relevant memory issue and eventually address this in the upstream component (at which point this changeset also does no harm).