Add BigArray.Genarray.free

[talex5]

Free the memory used by a bigarray, setting all dimensions to zero to prevent further use.

This is useful to ensure that memory-mapped files are closed promptly, without waiting for the garbage collector. Otherwise, mapping a large number of files may hit operating system limits on the number of open files.

It's also needed in MirageOS, where some Xen protocols require shared memory to be unmapped at certain points.

If e.g. sub_left is used to create multiple bigarrays backed by the same memory block, free must be called on all of them before the memory is actually released.

Test program:

(* ocamlc unix.cma bigarray.cma test.ml -o test && ./test *)

open Bigarray

let maps = ref []

let open_and_close () =
  let fd = Unix.(openfile "/etc/passwd" [O_RDONLY; O_NONBLOCK] 0o644) in
  let mapped = Array1.map_file fd char c_layout false 100 in
  (* Make bug more obvious by ensuring no GC *)
  maps := mapped :: !maps;
  (* Array1.free mapped; *)
  Unix.close fd

let () =
  let i = ref 0 in
  while true do
    incr i;
    open_and_close ();
    Printf.printf "Mapped %d files\n%!" !i
  done

For me, on Linux, this prints:

...
Mapped 35589 files
Mapped 35590 files
Fatal error: exception Unix.Unix_error(Unix.ENFILE, "open", "/etc/passwd")

[alainfrisch]

@xavierleroy implemented such a proposal some time ago and then reverted it. One problem with changing dynamically the dimensions is that it prevents interesting optimizations such as lifting/eliminating array bound checks (e.g. in the body of a for-loop iterating over indices of a bigarray). Or one can still assume that dimension are non mutable, at the risk of getting segfaults/bus error instead of proper out-of-bounds exceptions if the bigarray is accessed after being freed.

[gasche]

See MPR#4180, MPR#6962.

[talex5]

What's the current recommended solution then? I hit this twice recently. First in ocaml-git, which uses mmap to read files (and runs out, similar to the example posted above). Second, in a Mirage unikernel where I mapped a share memory region for a network device and started an Lwt thread watching it. When the device was disconnected, the memory was unmapped. Later, other data was mapped to the same address, and the old thread started trying to process it (which didn't end well).

[chambart]

Declare an new type that ensure that an access never occur after freeing.

module Unloadable : sig
  type t
  val map_file : string -> t
  val free : t -> unit
  val get : t -> int -> char
end = struct
  type t = { mutable freed : bool; data : (...) Bigarray.Array1.t }
  let map_file file = { freed = false; data = Bigarray.Array1.map_file ... }
  external unsafe_free_ba : (...) Bigarray.Array1.t -> unit = "..."
  let free t = t.freed <- true; unsafe_free_ba t.data
  let get t x =
    if t.freed then raise ...
    else Bigarray.Array1.get t.data x
end

If the indirection cost really matters you can imagine adding the 'freed' field to the C representation and add new primitives to check it when doing an access and implement this new module using it.

For the ocaml-git case, you also probably want to have a read-only type to allow sharing without doing the weak hash table dance, but that's another story.

[talex5]

The problem is that we expose the fact that we're using a Bigarray. For example, there is code that does:

let write fd t =
  Lwt_bytes.write fd t.Cstruct.buffer t.Cstruct.off t.Cstruct.len

If I hide the fact that t.buffer is a bigarray, I can't use it with Lwt (and I can't ensure that Lwt will check whether the Cstruct is freed without changing Bigarray itself).

[chambart]

On 04/01/2016 16:15, Thomas Leonard wrote:

The problem is that we expose the fact that we're using a Bigarray.
For example, there is code that does:

|let write fd t = Lwt_bytes.write fd t.Cstruct.buffer t.Cstruct.off
t.Cstruct.len |

If I hide the fact that t.buffer is a bigarray, I can't use it with
Lwt (and I can't ensure that Lwt will check whether the Cstruct is
freed without changing Bigarray itself).

—
Reply to this email directly or view it on GitHub
#389 (comment).

You can either:

• add an lwt_bytes_write function to your library and carefully check
  that this does not use the bigarray after returning;
• copy

[gasche]

For reference, I considered the following thought experiment: we could store in each bigarray an initialization-defined immutable bit that indicates whether or not its size may change in the future. The bigarray disposable/release/free function would fail dynamically on size-immutable arrays (ok, a dynamic check is not so nice), and bound-checking hoisting would only be allowed assuming this bit checked size-immutable (which is worth it as soon as at least two size-checks can be factorized; but admittedly such a there-is-potential-for-hoisting-logic may be harder to implement).

(You may think of phantom types for statical classification of may-be-resized, but the pain of transitioning end-user APIs could be a bit too large.)

The potential performance benefits of never-resizing bigarrays were discussed in MPR#5203. It may be the case that with the more aggressive inlining settings of flambda, a non-escaping analysis suffices to locally optimize bigarray accesses without imposing global non-resizability guarantees. It is, unfortunately, at least one release too soon to be able to know this.

[dbuenzli]

Just for reference, I think that it is since PR #115 that you can no longer play tricks with bigarray sizes (I used to do the same kind of thing for temporarily mapping GPU memory pointers).

[alainfrisch]

The potential performance benefits of never-resizing bigarrays were discussed in MPR#5203.

This was about lifting the access to the data pointer, which assumes that this pointer does not change. This property is not strictly related to resizability (which would affect the ability to lift bound checks).
Your proposal could apply to these two properties (and associated optimizations) in a similar way, or perhaps a single flag should control both.

and bound-checking hoisting would only be allowed assuming this bit checked size-immutable

I guess you are not proposing to compile the code twice and dispatch dynamically between the two versions according to the value of the flag; but rather to somehow let the programmer assert explicitly that the flag is set (causing a runtime failure if not, and allowing later optimizations). Is that right?

Something like:

let foo a =
  Bigarray.assert_immutable a;
  (* from this point, the compiler nows that 'a' has an immutable size and/or data-pointer *)
 ...

[talex5]

For reference, Jane Street's Bigstring library provides:

val unsafe_destroy : t -> unit

unsafe_destroy bstr destroys the bigstring by deallocating its associated data or, if memory-mapped, unmapping the corresponding file, and setting all dimensions to zero. This effectively frees the associated memory or address-space resources instantaneously. This feature helps working around a bug in the current OCaml runtime, which does not correctly estimate how aggressively to reclaim such resources.

This operation is safe unless you have passed the bigstring to another thread that is performing operations on it at the same time. Access to the bigstring after this operation will yield array bounds exceptions.
Raises Failure if the bigstring has already been deallocated (or deemed "external", which is treated equivalently), or if it has proxies, i.e. other bigstrings referring to the same data.

https://ocaml.janestreet.com/ocaml-core/109.24.01/doc/core/Bigstring.html#6_Destruction

[DemiMarie]

I think that the best solution would be for the compiler to assume that there are no calls to Bigarray.Genarray.free that it cannot see – that is, it should hoist accesses as long as it can see that nothing could change the bigarray's length. This might require interprocedural optimizations for best effect.

[gasche]

For what it's worth, I also think that the current policy has caused too much troubles for too little actual gain. We'll see how effectively flambda's inlining can help here, but I would be in favor of allowing explicit deallocation of bigarrays.

[DemiMarie]

@gasche I agree, especially since CSE and inlining should keep the performance penalty to a minimum.

[xavierleroy]

@gasche writes: "I would be in favor of allowing explicit deallocation of bigarrays." And turn off CSE on bigarray bounds and data pointer, degrading performance? Or keeping CSE and risk segfaults instead of "out of bound" exceptions when accessing a bigarray that has been freed? This is the dilemma...

[gasche]

And turn off CSE on bigarray bounds and data pointer, degrading performance?

Yes -- I feel that the qualitative difference in some interesting system workflows being made impossible is worse than the quantitative performance regressions that are being discussed. Supposedly we have better inlining capacities than we had previously, and in any case a local (post-inlining) escape analysis should be enough to restore the optimization in most cases. I don't know who has bigarray-intensive computations in production ( @alainfrisch ? ) but they could probably test this assumption.

Another route to take would be to declare Bigarray unfit to represent raw memory, and develop an alternative library to do what people have been doing with Bigarrays instead. This alternative should probably be in stdlib for people to actually accept to move to it. But if we have no one willing to do the work of organizing this (which would have a fairly invasive impact on a lot of third-party libraries), I personally think that giving up on the never-freed guarantee is the best move.

[ghost]

We definitely have bigarray-intensive computations in production. I think allowing bigarray to be freed without implementing this escape analysis would be pretty bad for us

[mshinwell]

I'm not sure I understand what's wrong with the unsafe_destroy solution. If the intended use cases of freeing Bigarrays are supposed to temporally match up with some external event, for example the disconnection of a device using memory-mapped I/O, a reasonable amount of care is going to be needed anyway. To my mind, it is therefore reasonable to place the burden on the programmer that after they call such an unsafe_destroy function, they don't manipulate the Bigarray subsequently. The main question that remains would seem to be: we would need to ensure that the compiler doesn't, for example, spill a Bigarray data pointer across a call to unsafe_destroy and then reload it and use it. However I suspect it is the case that such a code pattern would only arise if CSE has been in effect---and I don't think CSE should have this behaviour since registers of type Addr (e.g. Bigarray data pointers) are dropped across a call.

@xavierleroy Can you tell me where this reasoning is flawed?

[mshinwell]

@xavierleroy Ping, please.

[xavierleroy]

Nearly three years later, I have no idea what to do with this PR. I implemented bigarray deallocation a long time ago, then was convinced by @alainfrisch to take it out. Now I don't care and will let you guys fight it out.

[DemiMarie]

@talex5: since the main uses for this seem to involve memory mapped buffers, another thought came to mind: what about replacing the underlying buffer with a PROT_NONE mapping? That would not break any compiler optimizations, and any access after freeing would (deterministically) cause a segmentation fault.

[talex5]

what about replacing the underlying buffer with a PROT_NONE mapping?

That would certainly be more secure. However, it seems that people in this thread are not happy about segfaults at all, even deterministic ones (#389 (comment)):

Or keeping CSE and risk segfaults instead of "out of bound" exceptions when accessing a bigarray that has been freed?

[DemiMarie]

@talex5: what about catching the segfaults and turning them into an exception?

[gadmm]

Being able to convert a synchronous signal into an exception would be nice and open new opportunities in various scenarios, not just for freeing the backing memory of the bigarray. See the discussion at #5471 for (essentially) a previous attempt. It is unfortunate that the discussion is not fully available publicly though.

[xavierleroy]

Being able to convert a synchronous signal into an exception\

We've tried to do this for the last 25 years to convert "segmentation fault" signals to Stack_overflow exceptions, and it's a complete mess of machine and OS dependencies. One of the best things about OCaml 5 is that we got rid of that code. I wish it will never come back.