Log: delete source from builder cache when finalize #14475
Merged
Conversation
This file contains 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
The cache of logs in Log::Builder is using a WeakRef for the Log object to not prevent the GC from collecting them, that would leak memory, but it keeps the hash entry, which also leaks, keeping strings (potentially dynamically allocated) along with entries in the hash (infinitely growing hash). This patch adds a finalizer to Log, as suggested by @beta, to remove the log entry from the builder cache when a Log object is collected.
|
Oh, weird, the specs for |
The log instance may have been replaced, in which case we musn't delete it from the cache.
|
Fixed: don't indiscriminately delete the log instances (oops) since it may have been replaced (of course). I updated the chart but it doesn't show any visual difference. |
straight-shoota
approved these changes
Apr 12, 2024
1 task
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.
The cache of logs in Log::Builder is using a WeakRef for the Log object to not prevent the GC from collecting them, that would leak memory, but it keeps the hash entry, which also leaks, keeping strings (potentially dynamically allocated) along with entries in the hash (infinitely growing hash).
This patch adds a finalizer to Log, as suggested by @beta, to remove the log entry from the builder cache when a Log object is collected.
The runtime improvement for the sample from #14473 is dramatic. It looks almost flat with a mere 5s versus 67s for the current master at 100 000 iterations.
Note: this is a workaround to fix the leak, that can be monkey-patched in applications that need it, not a proper solution to the problem.