Libguestfs - May 2023 - [PATCH libguestfs 1/2] ocaml/implicit_close test: collect all currently unreachable blocks

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.

Laszlo

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, so it
should probably have some kind of conditional on the OCaml version.
It also needs a much clearer explanation.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

This bug and thread seem relevant:

https://github.com/ocaml/ocaml/issues/11812
https://discuss.ocaml.org/t/ocaml-5-gc-releasing-memory-back-to-the-os/11293

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

... And while I'm scrawling my throughts into this thread ...

What we intend here are two slightly different operations:

(A) Free every unreachable object.  That's what we want in this
specific place in the code.

(B) Provide a soft test that the OCaml heap hasn't been screwed
up because of bugs in our bindings.  That's what we use eg here:
https://gitlab.com/nbdkit/libnbd/-/blob/master/ocaml/tests/test_100_handle.ml#L24

In OCaml 4 Gc.compact provides (A) and (B), in particular compaction
is quite an aggressive & costly operation that requires examining and
moving every object on the heap.  In practice we found this quickly
reveals bugs in bindings.

OCaml 5 provides (A) through Gc.full_major--albeit not documented--and
doesn't seem to provide any way to do (B) as far as I can tell.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v