From 2e6e2fd9da18b74aa9555d09a871b24895e42773 Mon Sep 17 00:00:00 2001 From: Koichi Sasada Date: Mon, 13 Dec 2021 02:15:05 +0900 Subject: [PATCH] fix local TP memory leak It free `rb_hook_list_t` itself if needed. To recognize the need, this patch introduced `rb_hook_list_t::is_local` flag. This patch is succession of https://github.com/ruby/ruby/pull/4652 --- iseq.c | 5 ++--- vm_core.h | 3 ++- vm_trace.c | 37 +++++++++++++++++++++++-------------- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/iseq.c b/iseq.c index 21622fa288..9538847577 100644 --- a/iseq.c +++ b/iseq.c @@ -3359,6 +3359,7 @@ iseq_add_local_tracepoint(const rb_iseq_t *iseq, rb_event_flag_t turnon_events, if (n > 0) { if (iseq->aux.exec.local_hooks == NULL) { ((rb_iseq_t *)iseq)->aux.exec.local_hooks = RB_ZALLOC(rb_hook_list_t); + iseq->aux.exec.local_hooks->is_local = true; } rb_hook_list_connect_tracepoint((VALUE)iseq, iseq->aux.exec.local_hooks, tpval, target_line); } @@ -3413,9 +3414,7 @@ iseq_remove_local_tracepoint(const rb_iseq_t *iseq, VALUE tpval) local_events = iseq->aux.exec.local_hooks->events; if (local_events == 0) { - if (iseq->aux.exec.local_hooks->running == 0) { - rb_hook_list_free(iseq->aux.exec.local_hooks); - } + rb_hook_list_free(iseq->aux.exec.local_hooks); ((rb_iseq_t *)iseq)->aux.exec.local_hooks = NULL; } diff --git a/vm_core.h b/vm_core.h index 78d06012a1..6bbce23078 100644 --- a/vm_core.h +++ b/vm_core.h @@ -609,8 +609,9 @@ void rb_objspace_call_finalizer(struct rb_objspace *); typedef struct rb_hook_list_struct { struct rb_event_hook_struct *hooks; rb_event_flag_t events; - unsigned int need_clean; unsigned int running; + bool need_clean; + bool is_local; } rb_hook_list_t; diff --git a/vm_trace.c b/vm_trace.c index 3f589d3778..466856341d 100644 --- a/vm_trace.c +++ b/vm_trace.c @@ -69,8 +69,11 @@ static void clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list); void rb_hook_list_free(rb_hook_list_t *hooks) { - hooks->need_clean = TRUE; - clean_hooks(GET_EC(), hooks); + hooks->need_clean = true; + + if (hooks->running == 0) { + clean_hooks(GET_EC(), hooks); + } } /* ruby_vm_event_flags management */ @@ -200,10 +203,13 @@ static void clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list) { rb_event_hook_t *hook, **nextp = &list->hooks; - VM_ASSERT(list->need_clean == TRUE); rb_event_flag_t prev_events = list->events; + + VM_ASSERT(list->running == 0); + VM_ASSERT(list->need_clean == true); + list->events = 0; - list->need_clean = FALSE; + list->need_clean = false; while ((hook = *nextp) != 0) { if (hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED) { @@ -216,19 +222,21 @@ clean_hooks(const rb_execution_context_t *ec, rb_hook_list_t *list) } } - if (list == rb_ec_ractor_hooks(ec)) { - /* global events */ - update_global_event_hook(prev_events, list->events); + if (list->is_local) { + if (list->events == 0) { + /* local events */ + ruby_xfree(list); + } } else { - /* local events */ + update_global_event_hook(prev_events, list->events); } } static void clean_hooks_check(const rb_execution_context_t *ec, rb_hook_list_t *list) { - if (UNLIKELY(list->need_clean != FALSE)) { + if (UNLIKELY(list->need_clean)) { if (list->running == 0) { clean_hooks(ec, list); } @@ -251,7 +259,7 @@ remove_event_hook(const rb_execution_context_t *ec, const rb_thread_t *filter_th if (data == Qundef || hook->data == data) { hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED; ret+=1; - list->need_clean = TRUE; + list->need_clean = true; } } } @@ -1249,10 +1257,11 @@ disable_local_event_iseq_i(VALUE target, VALUE iseq_p, VALUE tpval) rb_hook_list_t *hooks = def->body.bmethod.hooks; VM_ASSERT(hooks != NULL); rb_hook_list_remove_tracepoint(hooks, tpval); - if (hooks->running == 0) { + + if (hooks->events == 0) { rb_hook_list_free(def->body.bmethod.hooks); + def->body.bmethod.hooks = NULL; } - def->body.bmethod.hooks = NULL; } return ST_CONTINUE; } @@ -1301,9 +1310,9 @@ rb_hook_list_remove_tracepoint(rb_hook_list_t *list, VALUE tpval) while (hook) { if (hook->data == tpval) { hook->hook_flags |= RUBY_EVENT_HOOK_FLAG_DELETED; - list->need_clean = TRUE; + list->need_clean = true; } - else { + else if ((hook->hook_flags & RUBY_EVENT_HOOK_FLAG_DELETED) == 0) { events |= hook->events; } hook = hook->next;