Store query caches in IsolatedExecutionState to avoid memory bloat #52622
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
Fix: rails#52617 Closes: rails#52618 It isn't per say a leak because the connection reaper eventually prune these, and also it's not expected that new Threads are constantly making entries into that cache. But it's true that with Fiber it's probably a bit more common, and the default reaper frequency isn't adapted to clear this fast enough. So instead of waiting for the reaper to trigger, which may take a long time, we keep the caches in IsolatedExcutionState so that when the owning thread or fiber is garbage collected its cache is too. However since we need to be able to clear all threads caches, we keep a cache version on the pool, and bump it to invalidate all caches of a pool at once.
|
Also this makes me think we could actually eliminate the reaper thread. |
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Store query caches in IsolatedExecutionState to avoid memory bloat
|
Hum, this seems to have introduced an issue with the PG test suite. It tend to run out of connection, It's not evident why just yet, but I suspect it's because we're now holding onto the pool from the query cache store. |
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Followup: #52622 This reference causes connections to linger around longer on Rails CI. Instead we can simply share an atomic integer to bump the version. x
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Followup: #52622 This reference causes connections to linger around longer on Rails CI. Instead we can simply share an atomic integer to bump the version. x
b64eadd improves that. Now one worry left however is that memory isn't reclaimed when a ConnectionPool is GCed, so if some users create lots of epehemeral pools, it could cause a leak. I'll see if I can fix that too. |
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Followup: #52622 Avoid leaking query caches when a ConnectionPool is gargabe collected.
|
24ded03 will do. |
byroot
added a commit
that referenced
this pull request
Aug 16, 2024
Followup: #52622 Avoid leaking query caches when a ConnectionPool is gargabe collected.
byroot
added a commit
to byroot/rails
that referenced
this pull request
Aug 16, 2024
Fix: rails#52617 Followup: rails#52622 Previous fixes solved the memory issues, but our fallback implementation of WeakKeyMap actually have terrible performance, and I can't find a way to do it in a performent way. So instead we replace it by a specialized weak map that only accept Thread or Fiber as keys, and simple purge dead threads on insertion. This gives us reasonable performance on Ruby 3.1 and 3.2.
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
Followup: rails#52622 This reference causes connections to linger around longer on Rails CI. Instead we can simply share an atomic integer to bump the version. x
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
Followup: rails#52622 Avoid leaking query caches when a ConnectionPool is gargabe collected.
DanielaVelasquez
pushed a commit
to DanielaVelasquez/rails
that referenced
this pull request
Oct 3, 2024
Fix: rails#52617 Followup: rails#52622 Previous fixes solved the memory issues, but our fallback implementation of WeakKeyMap actually have terrible performance, and I can't find a way to do it in a performent way. So instead we replace it by a specialized weak map that only accept Thread or Fiber as keys, and simple purge dead threads on insertion. This gives us reasonable performance on Ruby 3.1 and 3.2.
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.
Fix: #52617
Closes: #52618
It isn't per say a leak because the connection reaper eventually prune these, and also it's not expected that new Threads are constantly making entries into that cache. But it's true that with Fiber it's probably a bit more common, and the default reaper frequency isn't adapted to clear this fast enough.
So instead of waiting for the reaper to trigger, which may take a long time, we keep the caches in
IsolatedExecutionStateso that when the owning thread or fiber is garbage collected its cache is too.However since we need to be able to clear all threads caches, we keep a cache version on the pool, and bump it to invalidate all caches of a pool at once.