Bug #6030
closed
Thread-local "leak" in rb_exec_recursive*
Added by headius (Charles Nutter) about 12 years ago.
Updated almost 5 years ago.
Description
I believe there may be a "leak" in the rb_exec_recursive* functions in thread.c.
We have ported these methods for recursion detection in JRuby, and as in MRI they use a map inside a thread-local to track method name + object references. However, none of these method ever clean up the thread local when the recursive walk is complete.
The thread-local data is initialized in thread.c, recursive_list_access, around line 3819 (in 1.9.3 branch):
if (NIL_P(hash) || TYPE(hash) != T_HASH) {
hash = rb_hash_new();
OBJ_UNTRUST(hash);
rb_thread_local_aset(rb_thread_current(), recursive_key, hash);
As far as I can tell, this thread-local is never cleared, holding a hash reference for as long as the thread is alive.
I forgot to point out a couple things...
-
By "leak" I mean this will hold references to data longer than it should. It's not a true leak in that it is bounded.
-
It is bounded, but only by method names; recursive_list_access keys those thread-locals based on the current method name from the current frame, which means it will "leak" as many hash objects as there are unique methods utilizing rb_exec_recursive*.
From recursive_list_access:
...
VALUE sym = ID2SYM(rb_frame_this_func());
...
list = rb_hash_new();
OBJ_UNTRUST(list);
rb_hash_aset(hash, sym, list);
In JRuby, I fixed this by having rb_exec_recursive_outer clean up the thread-local upon completion, and I added recursive_list_operation (Ruby.java, recursiveListOperation) as a new "outermost" function to wrap our version of rb_exec_recursive. our rb_exec_recursive also gained an safeguard + error if it is called without being inside recursive_list_operation.
The commits are here:
https://github.com/jruby/jruby/commit/8f2ba4214eb070a83951aef1f143ebf6ebcecce4
https://github.com/jruby/jruby/commit/f7c1ffd93f34cef816168f36f5fe4f960a9ac2e2
It was a more severe problem for JRuby because the hash (a RubyHash) held references to the current JRuby instance, keeping it alive for as long as the thread was alive. Still a "leak", but a larger amount of bounded data held for too long.
Hi.
I'm not sure I see where there is a problem.
If Array#==
, say, is called in a given thread, it's quite likely that it will be called again later on, so why not keep the reference to the empty hash ready for next call?
As long as these hashes are empty at the end of the rb_exec_recursive_*, it's not clear that there is much memory to recoup. It could also affect the performance.
It would be possible to bloat up memory use by calling many method names that all hit this logic; saving a hash per name and never releasing it seems unnecessary.
If it's not a big enough deal for MRI to fix, so be it...it was an issue in JRuby because of the additional rooting of JRuby instances, but JRuby support MVM.
For that matter, this also is highly unlikely to work with the MVM stuff, though I know that's still off in experimental-land anyway.
It just seems like a good idea to clean up after the recursive logic is done, rather than leaving a dirty hash lying around.
- Assignee set to nobu (Nobuyoshi Nakada)
- Priority changed from Normal to 3
- Status changed from Open to Assigned
- Status changed from Assigned to Feedback
- Assignee changed from nobu (Nobuyoshi Nakada) to headius (Charles Nutter)
This ticket was discussed at today's developer meeting.
Nobu said that per-method entries are removed when it becomes unneeded. So, the "leak" is one hash object per thread. Correct? If so, it is not a big deal. Let us know if we missed something.
- Status changed from Feedback to Closed
Also available in: Atom
PDF
Like0
Like0Like0Like0Like0Like0Like0Like0