Alternative stat API #599

patricoferris · 2023-08-11T13:06:43Z

(this PR was originally adding stat support, but that got split out and merged separately)

This is mostly a draft until ocaml-multicore/ocaml-uring#95 is merged and released. But this is a continuation of https://github.com/talex5/eio/tree/stat adding support for linux. I'll see what I can do Windows...

This will also be useful for #594 as a fast "exists" check I think.

lib_eio_posix/eio_posix_stubs.c

talex5

uring 0.7 is out now. I pushed an update for that to this PR.

talex5 · 2023-08-19T14:00:27Z

lib_eio_linux/low_level.ml

@@ -366,6 +375,33 @@ let mkdir_beneath ~perm dir path =
  try eio_mkdirat parent leaf perm
  with Unix.Unix_error (code, name, arg) -> raise @@ Err.wrap_fs code name arg

+let float_of_time (s, ns) = Int64.to_float s +. (float ns /. 1e9)


OCaml itself makes sure the integer part isn't changed by rounding (ocaml/ocaml#9490). We should probably do that too (here and in eio_posix).

talex5 · 2023-08-19T14:01:58Z

lib_eio_posix/low_level.mli

@@ -36,7 +36,8 @@ val send_msg : fd -> ?fds:fd list -> ?dst:Unix.sockaddr -> Cstruct.t array -> in
 val getrandom : Cstruct.t -> unit

 val fstat : fd -> Unix.LargeFile.stats
-val lstat : string -> Unix.LargeFile.stats
+val lstat : string -> Unix.LargeFile.stats                      (* TODO: remove? *)


Should be able to remove this now.

avsm · 2023-08-20T09:10:04Z

I'm just playing around with Patrick's help on a revised version of this interface that can minimise allocations for stats at the Eio level as well.

module Stat = struct
  type kind = [ `Unknown | `Regular_file | `Socket ]
  type 'a typ =
  | Ino : int64 typ
  | Kind : kind typ
  and ('a, 'ty) t =
    | [] : ('ty, 'ty) t
    | (::) : 'a typ * ('b, 'ty) t -> ('a -> 'b, 'ty) t

  let ino = Ino
  let kind = Kind

  let _x = [ino; kind; ino]

  type r = [`Ino | `Kind]

  let extract : type a. a typ -> a = function
    | Ino -> 1L
    | Kind -> `Unknown

  let rec stat : type a b. (a, b) t -> a -> b =
    fun v acc ->
      match v with
      | v :: tl -> 
        let res = extract v in
        let f = acc res in
        stat tl f
      | [] -> acc
end

type just_ino = { ino : int64 }
let pp_ino ppf t = Format.fprintf ppf "%Ld" t.ino

let () =
  let ino = Stat.(stat [ ino; kind ] (fun ino _kind -> { ino })) in
  Format.printf "%a" pp_ino ino

With this, the stat call takes in a continuation which receives just the specified fields, and the Linux backend can then translate that into a statx while the posix/other backends just do a full stat. I'm just plumbing it all through to see what it looks like.

avsm · 2023-08-21T10:18:32Z

I've pushed the GADT-based interface in 989e0a9; comments welcome as to whether it's an improvement.

While it uses statx right now on eio_linux, it doesn't actually reduce memory usage since it allocates a fresh Uring.Statx.t on every stat call. We need #602 to maintain a pool of those buffers...

SGrondin · 2023-08-21T16:47:03Z

@avsm I've just read the suggested patch. While I am personally familiar with both GADTs and defining custom list types, because both of those things "leak" into the mli I think the suggested stat API is slightly too obscure for the more novice OCaml developers. A more experienced developer would see the API and realize it's that way to avoid allocations.

Yes, with a good example or two it would be fine. But it would mean forcing most users to refer to the docs anytime they want to use stat. In comparison, the classic stat API is fully discoverable through ocaml-lsp-server/merlin.

Since the stats call is used in a wide range of use cases, what about offering stat (returns the classic Stats.t record), exists (returns a boolean), and stats_apply (advanced, GADT-based, allocation-conscious function)?

avsm · 2023-08-21T16:58:10Z

I agree with the concerns about the obscurity of GADTs, but really do think that the default interface shouldn't be quite as inefficient as a complete retrieval of all the statistics, most of which are promptly thrown away.

Browsing through sherlocode for Unix.stat shows that a majority of the uses are of the form "(Unix.stat).field" to retrieve just one value. We could just provide single-field accessors (for which statx is pretty good) on the same lines as size right now, but for all the fields. Then the GADT interface would only be necessary when the developer wishes to retrieve multiple fields in one call.

SGrondin · 2023-08-21T17:09:03Z

I think that's a great compromise.

patricoferris

With statx support and the others, do you think it would also be good to provide a Path.exists that uses the statx whilst we're here?

patricoferris · 2023-08-21T17:59:38Z

lib_eio/path.mli

+(** [stat ~follow t] returns metadata about the file [t].
+
+    If [t] is a symlink, the information returned is about the target if [follow = true],
+    otherwise it is about the link itself. *)


Given the concerns about the GADT in the API, perhaps a small example in the documentation could help here?

Suggested change

otherwise it is about the link itself. *)

otherwise it is about the link itself.

For example, if you want to get a file's size along with what kind of file it is, you

can write:

{[

let kind, size =

Path.stat ~follow:true path File.Stat.[ kind; size ] (fun k s -> (k, s))

in

Eio.traceln "kind: %a, size: %Ld" File.Stat.pp_kind kind size

]}

*)

Added in 28169b5

SGrondin · 2023-08-22T14:29:18Z

Sorry for the slight bikeshedding, but do you think it would be an improvement to move the accessor functions into a Path.Stats or File.Stats submodule? I think that would make it easier to "browse" those accessor functions to find the one you're looking for. I know I personally don't know all the stats fields by name. It would also declutter the Path module, making it easier for newcomers to understand its role versus Fs, File, Flow, Resource, etc.

do you think it would also be good to provide a Path.exists that uses the statx whilst we're here?

I'd like to add my support for this suggestion. Otherwise it's something that a lot of users will cobble toghether, sometimes poorly by using read_dir or by catching an overly large range of exceptions in an attempt to detect a Not Found exception.

avsm · 2023-08-22T20:35:42Z

I'm playing around with rearranging the accessor functions in the module so that the advanced ones are at the bottom of the interface rather than the top. That makes the simpler ones much more discoverable...

In the meanwhile though, the blocker to adding Eio.Path.exists is that Unix.Unix_error(Unix.ENOENT,_,_) cannot be caught in the Eio.Path module since Unix isn't available!

talex5 · 2023-08-23T08:13:20Z

Unix.ENOENT should never appear outside of Eio_posix.Low_level. I'm using https://github.com/talex5/eio/blob/bdac846f34f2ade585886270772a4e3d065ef498/lib_eio/path.ml#L163-L165:

let exists ~follow path =
  try ignore (stat ~follow path : File.Stat.t); true
  with Exn.Io (Fs.E Not_found _, _) -> false

avsm · 2023-08-23T09:07:17Z

Take 3 pushed; I've:

removed Eio.File.Stat entirely and just moved the type definitions into Eio.File. There's no conflict with the other definitions. It makes everything much more discoverable (to address @SGrondin's comment) since the user doesn't need to know of the existence of a Stat submodule.
moved the complicated functions to the bottom of the mli file, so that the simpler accessor functions show up first. A simple change, but it makes a topdown reading of the odoc output much more beginner friendly.
Added the Path.exists* functions.

Still need:

Windows
test case updates to test the stat functions respect being confined (and do an EPERM exception there)
add Eio.Pool to the statx buffer handling in Eio_linux

Another round of bikeshedding on this version welcome! :-)

avsm · 2023-08-23T09:09:07Z

lib_eio_linux/eio_linux.mli

@@ -153,8 +153,11 @@ module Low_level : sig
  val await_writable : fd -> unit
  (** [await_writable fd] blocks until [fd] is writable (or has an error). *)

-  val fstat : fd -> Eio.File.Stat.t
-  (** Like {!Unix.LargeFile.fstat}. *)
+  val statx : ?buf:Uring.Statx.t -> ?fd:fd -> ?flags:Uring.Statx.Flags.t -> string -> ('a, 'b) Eio.File.stats -> 'a -> 'b


I'm not so clear on whether I hid dir_fd right here... need to double check.

avsm · 2023-08-23T09:09:28Z

lib_eio_linux/low_level.ml

-      ctime   = ust.st_ctime;
-    }
-  with Unix.Unix_error (code, name, arg) -> raise @@ Err.wrap_fs code name arg
+let float_of_time s ns = Int64.to_float s +. (float ns /. 1e9)


Still needs the rounding fix @talex5 pointed out in an earlier review.

avsm · 2023-08-23T09:11:41Z

lib_eio_posix/eio_posix_stubs.c

@@ -161,6 +163,68 @@ CAMLprim value caml_eio_posix_mkdirat(value v_fd, value v_path, value v_perm) {
  CAMLreturn(Val_unit);
 }

+static double double_of_timespec(struct timespec *t) {


Woops, @patricoferris @talex5 the obvious optimisation I just realised here is that we can also just pass around a stat buf in the Posix case, and have field accessors for it as noalloc just as we do for Eio_linux. This would avoid the unconditional copy into OCaml land for every field, and make the backends much more similar. The only difference then would be that the kernel is adding more fields into the stat buffer than requested when using Posix.

patricoferris · 2023-08-26T15:43:13Z

I haven't pushed it here just yet, but I have a WIP stat benchmark 8d60d08 (there's a rogue Sys.command "rm -r" in there so you might want to double-check it before you run!)

avsm · 2023-08-29T10:19:14Z

Somewhat unexpectedly, hacking in an Eio.Pool to reuse statx buffers in the invocation of Eio_linux.Low_level.stat seems to consistently slow down @patricoferris' benchmark by 10% or so, and allocate more memory.

diff --git a/lib_eio_linux/low_level.ml b/lib_eio_linux/low_level.ml
index 07c1041..05f7138 100644
--- a/lib_eio_linux/low_level.ml
+++ b/lib_eio_linux/low_level.ml
@@ -352,10 +352,17 @@ let statx ?buf ?fd ?(flags=Uring.Statx.Flags.empty) path f k =
 in
 fn f k
 
+let rr = ref 0
+let stat_pool = Eio.Pool.create 1000
+  (fun () ->
+     incr rr;
+     Printf.eprintf "%d\n%!" !rr;
+     Uring.Statx.create ())
 let stat dir_fd ?flags path f k =
-  match dir_fd with
-  | FD fd -> statx ~fd ?flags path f k
-  | Cwd | Fs -> statx ?flags path f k
+    Eio.Pool.use stat_pool (fun buf ->
+    match dir_fd with
+    | FD fd -> statx ~buf ~fd ?flags path f k
+    | Cwd | Fs -> statx ~buf ?flags path f k)

(This version just printfs to make sure that the pool isn't freeing/reallocation, which it isn't. The right number of statx buffers are indeed allocated). Not had a chance to investigate further.

patricoferris · 2023-09-08T19:42:23Z

Just a quick update to say I've consistently had issues with uring and big statting programs on a 5.15.0 kernel (it manifests as a Not_found exception but the file/directory definitely does exist). Don't see the issues on 6.2.0.

talex5 · 2023-09-21T11:29:12Z

I added a benchmark for fstat (#616) to see the effect of using the GADT vs a record. It seems to make no significant difference to performance, even when I remove the actual call to fstat and just use a fixed result in all cases. This is in the best case, accessing only one field. So this doesn't help performance on the OCaml side, but it does give the OS opportunities to optimise without penalising the OCaml side, which is good.

However, using uring for fstat is about 27x slower on my machine (Linux 6.1.52). Most of the time is spent in the kernel, and I think uring is pushing it to a worker thread. So we should probably leave that using the fstat system call for now.

patricoferris · 2023-09-21T14:34:15Z

Interesting, sorry for dropping the ball a little on this PR. I'm not sure if it is related at all, but I noticed considerably slower code in https://github.com/patricoferris/aperitif when comparing Eio and the plain OCaml sequential code (confusingly under miou/main_s). I didn't really have to dig deeper and there's no point yet comparing with miou as it uses parallelism and the Eio version doesn't.

talex5 · 2023-09-21T15:08:09Z

I'm not sure what to do with this PR. I'd like to get stat support in soon, but it's unclear if the change to a GADT is an improvement. Some possible problems:

May be confusing for users (as noted above).
Might encourage people to perform multiple stat operations if they want multiple fields.
Changes the current API.

Quite verbose if you do want all the fields, e.g.

Eio.File.stat file [Dev; Ino; Kind; Perm; Nlink; Uid; Gid; Rdev; Size; Atime; Ctime; Mtime]
@@ fun dev ino kind perm nlink uid gid rdev size atime ctime mtime ->
{ dev; ino; kind; perm; nlink; uid; gid; rdev; size; atime; ctime; mtime }

Easy to get the fields in the wrong order and not notice (e.g. switching ctime and atime).

I wonder if we should merge it now using the original type, and then add an optional argument later to say you only want some of the fields? In theory the current layout is a bit inefficient (with the int64s everywhere), but if you only want a few fields then it will only be on the minor heap and doesn't seem to be a problem. Also, OCaml might get better at laying records out in future.

talex5 · 2023-09-22T12:03:07Z

I've spit out the low-level support into #617 and #618 so that can be reviewed separately.

avsm · 2023-09-24T16:39:25Z

I'm ok with unblocking this PR with the current interface, but there are some important considerations we should keep in mind for the next rev that we've learnt from this.

uring support envelope: This is highly dependent on the kernel version, and so we should establish some principles for the lower and upper bounds of when we activate uring for users. Some functionality seems plain broken on older kernels (such as @patricoferris' report of 5.15 stat calls going wrong), and these should form the lower bound. Our bar for performance should probably be by testing on the very latest kernels available though, since uring does demand the bleeding edge for now. For example, I looked into @talex5's report of why the fstat syscall might be an order of magnitude faster than uring, and discovered this hot-off-the-press thread on why glibc's fstat is slow. I think the answer actually lies in the fact that we're using the sandboxed syscalls by default, and not in worker threads. This will take time to figure out, so I'll push a WIP benchmark I have to the uring repository and we can test it out on multiple kernels and see exactly what's going on under the hood.
memory usage: It does still irk me that we're copying into a minor heap record so unnecessarily. I don't buy the argument that OCaml will eventually lay out records better in the future -- that sort of layout transformation is a considerable distance away. However, I agree that we don't need to change the existing API until we have a clear understanding of the performance implications of the statx syscall before optimising anything else.
gadt interface: I still like it -- it's got individual accessor functions and the 'advanced bit' isn't needed by the common case. However, again I am fine deferring this until we get a better handle on the performance. I would like any proposal for a better API to be accompanied by a clear perf/memory benchmark that illustrates the benefit, and there's too much to disentangle in this PR for that to happen right now!

talex5 · 2023-09-24T16:55:50Z

Some functionality seems plain broken on older kernels (such as @patricoferris' report of 5.15 stat calls going wrong), and these should form the lower bound.

Yes, we need to work out what the minimum should be. I'm on 6.1 and I haven't seen that error yet.

I'm working on a minimal patch to support stat with the current record system here: https://github.com/ocaml-multicore/eio/compare/main...talex5:eio:stat?expand=1 (will open a PR soon).

Would be good to get some benchmarks indeed. I did some quick tests recently and it looked like using the GADT with lots of fields was slower than using the record, while using it with a single field was no faster. Not sure why, though.

Co-authored-by: Thomas Leonard <talex5@gmail.com> Co-authored-by: Patrick Ferris <patrick@sirref.org>

talex5 · 2023-09-26T10:19:41Z

I squashed and rebased this on top of #620, so it contains just the bits missing from that:

Using a GADT to select which fields to use.
bench_stat benchmark (Add Path.stat benchmark #630).
Documentation for fields (Document File.Stat record fields #621).
exists, is_file, is_directory (Add Eio.Path.{kind, is_file, is_directory} #623).
Using statx to replace fstat (too slow for now).
eio_stat test (should probably be an example rather than a test).

talex5 · 2023-09-26T12:44:18Z

Just a quick update to say I've consistently had issues with uring and big statting programs on a 5.15.0 kernel (it manifests as a Not_found exception but the file/directory definitely does exist). Don't see the issues on 6.2.0.

Probably related to this change in 5.18: io-uring: Make statx API stable (from @SGrondin's list at https://gist.github.com/SGrondin/be12fae9e4851fb864bc7d5de9297f1a).

Fixed by #624.

talex5 · 2023-12-12T09:20:53Z

Closing for now, as most of this PR got merged and it's unclear it we want the other bits at all.

talex5 reviewed Aug 14, 2023

View reviewed changes

lib_eio_posix/eio_posix_stubs.c Outdated Show resolved Hide resolved

talex5 reviewed Aug 15, 2023

View reviewed changes

lib_eio_posix/eio_posix_stubs.c Outdated Show resolved Hide resolved

talex5 mentioned this pull request Aug 16, 2023

Make MDX tests idempotent #601

Merged

talex5 force-pushed the stat branch from 992b5c5 to bfc4bf9 Compare August 18, 2023 08:17

patricoferris marked this pull request as ready for review August 18, 2023 08:28

talex5 force-pushed the stat branch 3 times, most recently from 8844bbc to ea7a60d Compare August 19, 2023 13:54

talex5 reviewed Aug 19, 2023

View reviewed changes

avsm force-pushed the stat branch from 989e0a9 to ae063ae Compare August 21, 2023 10:57

patricoferris commented Aug 21, 2023

View reviewed changes

avsm reviewed Aug 23, 2023

View reviewed changes

avsm force-pushed the stat branch from 6f8d307 to 33cab74 Compare September 8, 2023 18:22

talex5 mentioned this pull request Sep 20, 2023

Add File.stat benchmark #616

Merged

talex5 force-pushed the stat branch from 33cab74 to 7a985da Compare September 20, 2023 18:40

This was referenced Sep 22, 2023

Add Eio_linux.Low_level.statx #617

Merged

Add Eio_posix.Low_level.fstatat #618

Merged

talex5 mentioned this pull request Sep 25, 2023

Add Path.stat #620

Merged

Experiment: use GADT for stat

47b4cc2

Co-authored-by: Thomas Leonard <talex5@gmail.com> Co-authored-by: Patrick Ferris <patrick@sirref.org>

talex5 force-pushed the stat branch from 7a985da to 47b4cc2 Compare September 26, 2023 10:14

talex5 mentioned this pull request Sep 26, 2023

Document File.Stat record fields #621

Merged

talex5 mentioned this pull request Sep 26, 2023

Add Eio.Path.{kind, is_file, is_directory} #623

Merged

talex5 changed the title ~~Support statting~~ Alternative stat API Sep 27, 2023

talex5 mentioned this pull request Oct 8, 2023

Add Path.stat benchmark #630

Merged

talex5 closed this Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative stat API #599

Alternative stat API #599

patricoferris commented Aug 11, 2023 •

edited by talex5

Loading

talex5 left a comment

talex5 Aug 19, 2023

talex5 Aug 19, 2023

avsm commented Aug 20, 2023

avsm commented Aug 21, 2023

SGrondin commented Aug 21, 2023

avsm commented Aug 21, 2023

SGrondin commented Aug 21, 2023

patricoferris left a comment

patricoferris Aug 21, 2023 •

edited

Loading

avsm Aug 21, 2023

SGrondin commented Aug 22, 2023 •

edited

Loading

avsm commented Aug 22, 2023

talex5 commented Aug 23, 2023

avsm commented Aug 23, 2023 •

edited

Loading

avsm Aug 23, 2023

avsm Aug 23, 2023

avsm Aug 23, 2023

patricoferris commented Aug 26, 2023 •

edited

Loading

avsm commented Aug 29, 2023

patricoferris commented Sep 8, 2023

talex5 commented Sep 21, 2023

patricoferris commented Sep 21, 2023

talex5 commented Sep 21, 2023

talex5 commented Sep 22, 2023

avsm commented Sep 24, 2023

talex5 commented Sep 24, 2023

talex5 commented Sep 26, 2023 •

edited

Loading

talex5 commented Sep 26, 2023 •

edited

Loading

talex5 commented Dec 12, 2023

-    otherwise it is about the link itself. *)
+    otherwise it is about the link itself.
+    For example, if you want to get a file's size along with what kind of file it is, you
+    can write:
+    {[
+       let kind, size =
+         Path.stat ~follow:true path File.Stat.[ kind; size ] (fun k s -> (k, s))
+       in
+       Eio.traceln "kind: %a, size: %Ld" File.Stat.pp_kind kind size
+    ]}
+*)

Alternative stat API #599

Alternative stat API #599

Conversation

patricoferris commented Aug 11, 2023 • edited by talex5 Loading

talex5 left a comment

Choose a reason for hiding this comment

talex5 Aug 19, 2023

Choose a reason for hiding this comment

talex5 Aug 19, 2023

Choose a reason for hiding this comment

avsm commented Aug 20, 2023

avsm commented Aug 21, 2023

SGrondin commented Aug 21, 2023

avsm commented Aug 21, 2023

SGrondin commented Aug 21, 2023

patricoferris left a comment

Choose a reason for hiding this comment

patricoferris Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

avsm Aug 21, 2023

Choose a reason for hiding this comment

SGrondin commented Aug 22, 2023 • edited Loading

avsm commented Aug 22, 2023

talex5 commented Aug 23, 2023

avsm commented Aug 23, 2023 • edited Loading

avsm Aug 23, 2023

Choose a reason for hiding this comment

avsm Aug 23, 2023

Choose a reason for hiding this comment

avsm Aug 23, 2023

Choose a reason for hiding this comment

patricoferris commented Aug 26, 2023 • edited Loading

avsm commented Aug 29, 2023

patricoferris commented Sep 8, 2023

talex5 commented Sep 21, 2023

patricoferris commented Sep 21, 2023

talex5 commented Sep 21, 2023

talex5 commented Sep 22, 2023

avsm commented Sep 24, 2023

talex5 commented Sep 24, 2023

talex5 commented Sep 26, 2023 • edited Loading

talex5 commented Sep 26, 2023 • edited Loading

talex5 commented Dec 12, 2023

patricoferris commented Aug 11, 2023 •

edited by talex5

Loading

patricoferris Aug 21, 2023 •

edited

Loading

SGrondin commented Aug 22, 2023 •

edited

Loading

avsm commented Aug 23, 2023 •

edited

Loading

patricoferris commented Aug 26, 2023 •

edited

Loading

talex5 commented Sep 26, 2023 •

edited

Loading

talex5 commented Sep 26, 2023 •

edited

Loading