Fix some leaks in systhreads by gasche · Pull Request #11406 · ocaml/ocaml

gasche · 2022-07-06T15:16:14Z

This PR, joint work with @Engil and @fabbing, fixes some leaks in threads that we hope are the leak issues reported in #11289.

@toots, could you try on trunk (or 5.0) and tell us if you still observe leaks with this PR?

We wonder who would be interested in reviewing this. Maybe @gadmm, @xavierleroy or @sadiqj?

gadmm · 2022-07-06T15:26:29Z

I had a quick look, overall this looks good and makes sense to me. Not a formal review yet but let me know if you need one from me. I am also reassured that there is no overlap with the other PRs on systhreads.

The issue was originally for 4.13, is the fix substantially different there? Do we know what introduced the issue?

gasche · 2022-07-06T18:30:15Z

Not a formal review yet but let me know if you need one from me.

If you are available and interested to do a review, then yes a review would be welcome. (I think it could definitely help other people decide to approve the PR.)

We haven't investigated the 4.13 systhreads codebase, so I don't know where leaks are there and how 4.14 fixed them. My guess is that the two implementations are very different, and the sources of leak are probably different as well.

Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

(there are probably other resource leaks in domain_terminate, but those are reused from the work on thread cleanup.) Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

toots · 2022-07-10T19:12:37Z

Thanks for working on this y'all. I'm happy to report that the leak seems fixed on my end as well:

runtime/backtrace_byt.c

Octachron · 2022-07-13T15:41:53Z

runtime/domain.c

    caml_free_stack(domain_state->current_stack);
  }
+  caml_free_backtrace_buffer(domain_state->backtrace_buffer);
+  caml_free_gc_regs_buckets(domain_state->gc_regs_buckets);


This part is cleaning up some resource on the main domain thread and is not strictly speaking part of the systhread leak, isn't it?

Yes indeed. (This is a separated fourth commit.) We thought it made more sense to employ the new resource-cleanup in both places where they initialized, for threads and for domains. (Otherwise the code ends up in an arguably inconsistent state.)

+1 for cleaning up memory at domain exit too, not just thread exit. The title of this PR and the Changes entry could reflect this fact.

gasche · 2022-07-13T20:15:03Z

runtime/fiber.c

+    caml_stat_free(gc_regs_buckets);
+    gc_regs_buckets = next;
+  }
+}


Note: @Octachron and myself discussed this offline and we decided that it would be nice to have a documentation comment that explains what gc_regs and gc_regs_buckets are. I may submit this as a separate (complementary) PR.

Yes, some documentation about gc_regs_buckets would be great. I know what gc_regs does in OCaml 4, but I'm a bit lost as to the OCaml 5 implementation of gc_regs, what with the allocation via malloc and so on.

For the record, I proposed some documentation of gc_regs and gc_regs_buckets in #11437.

xavierleroy

Looks good to me overall, but I don't claim I understand the gc_regs_bucket business. Let's wait for @Octachron's approval before merging.

xavierleroy · 2022-07-15T12:48:33Z

runtime/domain.c

    caml_free_stack(domain_state->current_stack);
  }
+  caml_free_backtrace_buffer(domain_state->backtrace_buffer);
+  caml_free_gc_regs_buckets(domain_state->gc_regs_buckets);


+1 for cleaning up memory at domain exit too, not just thread exit. The title of this PR and the Changes entry could reflect this fact.

xavierleroy · 2022-07-15T12:50:13Z

runtime/fiber.c

+    caml_stat_free(gc_regs_buckets);
+    gc_regs_buckets = next;
+  }
+}


Yes, some documentation about gc_regs_buckets would be great. I know what gc_regs does in OCaml 4, but I'm a bit lost as to the OCaml 5 implementation of gc_regs, what with the allocation via malloc and so on.

Octachron · 2022-07-18T12:37:44Z

As far as I understand, caml_thread_reinitialize should also free the same resource as caml_thread_remove_info when it frees the list of threads within the domain.

gasche · 2022-07-18T13:26:32Z

As far as I understand, caml_thread_reinitialize should also free the same resource as caml_thread_remove_info when it frees the list of threads within the domain.

My understanding is rather that only "pthread resources" (mutexes and what not) need to be reset after a fork, not memory which we assume was correctly duplicated. (A hint is that there is currently no code to cleanup the call stack in reinitialize, while there is one in remove_info.)

Octachron · 2022-07-18T13:51:45Z

The reinitialize function is currently freeing the current stack too? Moreover, the function is also freeing the thread information structure, thus there is no pointer left pointing to the gc_reg_buffers or backtrace_buffer structures for secondary threads. What I am missing?

gasche · 2022-07-18T13:58:46Z

Ah, right, the reinitialize function appears to be trying to cleanup all threads that are not the active thread. I don't know what the systhreads+fork combination is supposed to be doing, but clearly you are right and it should share the cleanup logic. Sorry for missing this earlier.

Suggested-by: Florian Angeletti <florian.angeletti@inria.fr>

gasche · 2022-07-18T14:10:56Z

@Octachron this should have been addressed in cacc1cd. Thanks!
( @Engil I wonder if you could confirm that this is the right approach. )

Octachron

I agree that this PR fixes a leak.

Fix some leaks in systhreads (cherry picked from commit b8dbb53) The cherry-pick required solving a conflict coming from 159f3f3 (PR #11390) which is not (yet?) merged in 5.0.

gasche · 2022-07-19T08:45:26Z

Merged, and cherry-picked in 5.0 as 29e912c. I had to resolve a conflict coming from #11390 (which is not (yet?) in 5.0).

abbysmal · 2022-07-19T09:05:54Z

Sorry, I'm currently on leave (will be back on monday).
I think the change is correct, however I spotted one thing I think is strange?

gasche and others added 4 commits July 7, 2022 10:19

fiber: caml_free_gc_regs_buckets

37e7687

Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

backtrace: caml_free_backtrace_buffer

8606b90

Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

systhreads: fix leaks on thread termination

29701b7

Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

domain: more resource cleanups in domain_terminate

cd006c1

(there are probably other resource leaks in domain_terminate, but those are reused from the work on thread cleanup.) Co-authored-by: Enguerrand Decorne <decorne.en@gmail.com> Co-authored-by: Fabrice Buoro <fabrice@tarides.com>

gasche force-pushed the threads-leaks branch from af7ef9f to cd006c1 Compare July 7, 2022 08:21

gasche mentioned this pull request Jul 7, 2022

Memory leak when using threads #11289

Closed

Octachron reviewed Jul 13, 2022

View reviewed changes

runtime/backtrace_byt.c Show resolved Hide resolved

Octachron reviewed Jul 13, 2022

View reviewed changes

gasche commented Jul 13, 2022

View reviewed changes

xavierleroy approved these changes Jul 15, 2022

View reviewed changes

gasche mentioned this pull request Jul 15, 2022

Document gc_regs and gc_regs_buckets #11437

Merged

2 tasks

systhreads: also cleanup threads resource in _reinitialize

cacc1cd

Suggested-by: Florian Angeletti <florian.angeletti@inria.fr>

Octachron approved these changes Jul 19, 2022

View reviewed changes

gasche merged commit b8dbb53 into ocaml:trunk Jul 19, 2022

toots mentioned this pull request Aug 24, 2022

Add option to compile ocaml with leak sanitizer. ocaml/opam-repository#21667

Closed

Conversation

gasche commented Jul 6, 2022

Uh oh!

gadmm commented Jul 6, 2022

Uh oh!

gasche commented Jul 6, 2022

Uh oh!

toots commented Jul 10, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Jul 18, 2022

Uh oh!

gasche commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Octachron commented Jul 18, 2022

Uh oh!

gasche commented Jul 18, 2022

Uh oh!

gasche commented Jul 18, 2022

Uh oh!

Octachron left a comment

Choose a reason for hiding this comment

Uh oh!

gasche commented Jul 19, 2022

Uh oh!

abbysmal commented Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gasche commented Jul 18, 2022 •

edited

Loading