On Wed, May 31, 2023 at 04:44:49PM +0200, Laszlo Ersek wrote: > On 5/31/23 13:13, Richard W.M. Jones wrote: > > On Wed, May 31, 2023 at 12:32:49PM +0200, Laszlo Ersek wrote: > >> On 5/31/23 11:12, Richard W.M. Jones wrote: > >>> On Sat, May 27, 2023 at 03:32:36PM +0200, Jürgen Hötzel wrote: > >>>> Fixes failing implice_close test on OCaml 5. > >>>> --- > >>>> ocaml/t/guestfs_065_implicit_close.ml | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/ocaml/t/guestfs_065_implicit_close.ml > >>>> b/ocaml/t/guestfs_065_implicit_close.ml > >>>> index 567e550b4..5e00c21ac 100644 > >>>> --- a/ocaml/t/guestfs_065_implicit_close.ml > >>>> +++ b/ocaml/t/guestfs_065_implicit_close.ml > >>>> @@ -30,8 +30,8 @@ let () = > >>>> *) > >>>> > >>>> (* This should cause the GC to close the handle. *) > >>>> -let () = Gc.compact () > >>>> +let () = Gc.full_major () > >>>> > >>>> let () = assert (!close_invoked = 1) > >>>> > >>>> -let () = Gc.compact () > >>>> +let () = Gc.full_major () > >>> > >>> I don't understand this patch at all. If there a test failing we need > >>> to diagnose why it is failing, not paper over the symptoms. > >>> > >>> What is the exact failure? > >> > >> Well my assumption is that (a) we need to force a garbage collection for > >> the (unreachable) handle to be closed actually (from earlier, nothing > >> new regarding that), but (b) with OCaml 5, "compact" is not strong > >> enough for that, while "full_major" is. Whether that means OCaml 5 > >> changed the semantics of these functions, or that even with OCaml 4 > >> we've only (consistently) lucky with "compact", I can't tell. > > > > So it turns out there is a difference between OCaml 4.14 and 5 here. > > > > In 4.14: > > > > Gc.compact finishes the current major cycle and then compacts the > > heap: > > > > https://github.com/ocaml/ocaml/blob/74fe398bbe2e53db21a998356b042c232d42a4d8/runtime/gc_ctrl.c#L625 > > > > Gc.full_major finishes the major cycle and then sometimes compacts the > > heap based on a threshold of how much heap is being used: > > > > https://github.com/ocaml/ocaml/blob/74fe398bbe2e53db21a998356b042c232d42a4d8/runtime/gc_ctrl.c#L579 > > > > So Gc.full_major is (kind of) a subset of Gc.compact, which (mostly) > > matches the documentation. > > > > In 5.0: > > > > Gc.compact finishes the major cycle, and as far as I can tell doesn't > > compact the heap (bug?!): > > > > https://github.com/ocaml/ocaml/blob/ffb2022797986324213891a59c02af46269b5c17/runtime/gc_ctrl.c#L297 > > > > But the big difference is Gc.full_major: > > > > https://github.com/ocaml/ocaml/blob/ffb2022797986324213891a59c02af46269b5c17/runtime/gc_ctrl.c#L261 > > > > As you can see from a comment in the code, "[the new garbage > > collector] can require up to 3 GC cycles for a currently-unreachable > > object to be collected", and Gc.full_major does this. (I don't > > believe this is true for the old GC in OCaml <= 4.) > > > > So in 5.0 full_major is quite a different operation from compact. It > > runs multiple major cycles to make sure everything is collected, and > > doesn't compact the heap, but then neither does Gc.compact which is no > > longer a superset of Gc.full_major. > > > > The patch there is correct, for OCaml 5, but breaks OCaml 4, > > Right, from your explanation above, this is what I've been expecting -- > what works for OCaml 5 may not work for OCaml 4. > > In fact I don't understand the OCaml runtime's developers -- this is a > compatibility breaking change! Every project that uses the Gc module > will have to add compat code (version checking) now.
Yes, it's not a good change. About the patch, we could try to conditionalize it on Sys.ocaml_release.Sys.major >= 5 (see /usr/lib64/ocaml/sys.mli) Rich. > > so it > > should probably have some kind of conditional on the OCaml version. > > It also needs a much clearer explanation. > > Thanks > Laszlo -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
