remove runtime.SetFinalizer #905
Conversation
There was a problem hiding this comment.
I did some research on runtime.SetFinalizer and it should not be used for "large" objects,
I have never heard this before. References please.
has no guarantees on being called,
Agreed. However, it is a best effort attempt to reclaim C memory in some cases
and if not done properly will not free memory properly,
Probably true but I need more information on what is or isn't proper in the context of your issue.
and is considered a "hack" for code that doesnt cleanup properly (closing connections, etc).
Repeating myself, this is generally true but we have it as a just-in-case mechanism for freeing memory allocated by the C/C++ based code that go-ceph calls into.
In the bug that I reported I had only done about 30 concurrent copies of a 2Gi image, this resulted in 1.1G of RSS; memory was never freed. I have done 90+ concurrent copies of a 2Gi image and memory usage stays around 210M RSS; so I believe this PR fixes.
I'm extremely puzzled as to why a memory cleanup mechanism would cause your problem when it should only be triggered in the cases where other objects otherwise don't get freed.
I appreciate your effort to try and help improve go-ceph, but I think I need a little convincing this is the correct change. First off, you've eliminated a lot of runtime.SetFinalizer calls in a single patch. It would be nice to try and narrow things down to exactly which calls might be causing your problem. Furthermore, it will be much more convincing to show some sort of heap profiling before and after the changes showing exactly what kind of leaks occur when the finalizer is in place. With data, we may find we can improve our use of finalizer rather than totally eliminating it.
| if err := c.ensureConnected(); err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
What's this change have to do with the rest of the patch?
There was a problem hiding this comment.
// freeConn releases resources that are allocated while configuring the
// connection to the cluster. rados_shutdown() should only be needed after a
// successful call to rados_connect(), however if the connection has been
// configured with non-default parameters, some of the parameters may be
// allocated before connecting. rados_shutdown() will free the allocated
// resources, even if there has not been a connection yet.
Doing the check will result in a memory leak if there is no connection based on what it says in the src documentation.
https://lemire.me/blog/2023/05/19/the-absurd-cost-of-finalizers-in-go/, https://utcc.utoronto.ca/~cks/space/blog/programming/GoFinalizerCostsNotes
why not just communicate that end user should close/destroy/shutdown where proper?
yea but it doesnt work, and its unreliable:
|
I believe this fixes #904
I did exact same tests in that issue (more actually) and memory usage is ~200MB, down from 1100MB.
I did some research on
runtime.SetFinalizerand it should not be used for "large" objects, has no guarantees on being called, and if not done properly will not free memory properly, and is considered a "hack" for code that doesnt cleanup properly (closing connections, etc). It also has a significant performance hit when the GC runs, and tells the GC to keep track of the code, but that doesnt really make sense when there are destroy/shutdown/close functions right?In the bug that I reported I had only done about 30 concurrent copies of a 2Gi image, this resulted in 1.1G of RSS; memory was never freed. I have done 90+ concurrent copies of a 2Gi image and memory usage stays around 210M RSS; so I believe this PR fixes.