Page MenuHomePhabricator

[OCaml] Omit unnecessary GC root registrations
ClosedPublic

Authored by jberdine on Mar 28 2021, 3:33 PM.

Details

Summary

The current code does not follow the simple interface to the OCaml GC,
where GC roots are registered conservatively, only initializing
allocations are performed, etc. This is intentional, as stated in the
opening file comments. On the other hand, the current code does
register GC roots in many situations where it is not strictly
necessary. This diff omits many of them.

Diff Detail

Event Timeline

jberdine created this revision.Mar 28 2021, 3:33 PM
jberdine requested review of this revision.Mar 28 2021, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2021, 3:33 PM

I intend to do some additional testing of this, but put this diff up in the meantime for feedback and perhaps also some wider testing.

vaivaswatha accepted this revision.Mar 29 2021, 10:05 AM

Similar to my comment in https://reviews.llvm.org/D99472, I don't understand this a 100%, but it seems sane to me.

Also, let me know if I can help you test these patches in anyway.

This revision is now accepted and ready to land.Mar 29 2021, 10:05 AM

I have been testing this stack of diffs with a sort of stress test that involves running a system that consumes bitcode and uses the ocaml api to traverse and translate it. The testing has used a range of OCaml GC settings in order to make the GC work much harder and trigger much more often than usual (e.g. Gc.set {(Gc.get ()) with minor_heap_size= 1024; space_overhead= 20}). The reasoning here is that if the bindings are not playing nice with the GC, then there is a higher chance to uncover such issues if the GC runs much more often. This testing has not revealed any issues.

@vaivaswatha if it is not too much overhead in your application, it would be helpful if you could arc patch these diffs, add a call to Gc.set as above in your initialization code, and just run your application to see if there are any crashes.

I have been testing this stack of diffs with a sort of stress test that involves running a system that consumes bitcode and uses the ocaml api to traverse and translate it. The testing has used a range of OCaml GC settings in order to make the GC work much harder and trigger much more often than usual (e.g. Gc.set {(Gc.get ()) with minor_heap_size= 1024; space_overhead= 20}). The reasoning here is that if the bindings are not playing nice with the GC, then there is a higher chance to uncover such issues if the GC runs much more often. This testing has not revealed any issues.

@vaivaswatha if it is not too much overhead in your application, it would be helpful if you could arc patch these diffs, add a call to Gc.set as above in your initialization code, and just run your application to see if there are any crashes.

@jberdine I'll surely try it out. What do you mean by initialization code? Can you send me an example of where you're setting it (if that's open source), I'll try and mimic that in my application.

btw, if you have GitHub links to the patches here (like you did for your review suggestions for my debuginfo patch), please send that. it's easier to just cherry-pick git commits. Otherwise I'll try arch patch.

See https://github.com/jberdine/llvm-project/tree/ocaml for this stack on github.

For the init code, I just mean to add that Gc.set call somewhere in your code that uses the LLVM bindings before you do much with Llvm.

See https://github.com/jberdine/llvm-project/tree/ocaml for this stack on github.

For the init code, I just mean to add that Gc.set call somewhere in your code that uses the LLVM bindings before you do much with Llvm.

@jberdine I built your branch locally and linked to it from my project that uses LLVM (and its OCaml bindings). I added the call to Gc.set before calling any of the LLVM functions. Everything worked fine and my testsuite succeeded. Let me know if you want me to try out anything else. Thank you.

Thank you very much @vaivaswatha, that is very helpful!

This revision was landed with ongoing or failed builds.Apr 5 2021, 2:59 AM
This revision was automatically updated to reflect the committed changes.