Closed patricoferris closed 8 months ago
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.
I've pushed the GADT-based interface in 989e0a9650481ad0f10240399c3814711ae3e37d; 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...
@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)?
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.
I think that's a great compromise.
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.
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!
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
Take 3 pushed; I've:
Stat
submodule.Still need:
Another round of bikeshedding on this version welcome! :-)
I haven't pushed it here just yet, but I have a WIP stat
benchmark https://github.com/ocaml-multicore/eio/commit/8d60d083a289148a6c1f912f429b79d3a83eaa41 (there's a rogue Sys.command "rm -r"
in there so you might want to double-check it before you run!)
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.
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.
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.
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.
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:
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 }
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 int64
s 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.
I've spit out the low-level support into #617 and #618 so that can be reviewed separately.
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.
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.
I squashed and rebased this on top of #620, so it contains just the bits missing from that:
bench_stat
benchmark (#630).exists
, is_file
, is_directory
(#623).statx
to replace fstat
(too slow for now).eio_stat
test (should probably be an example rather than a test).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.
Closing for now, as most of this PR got merged and it's unclear it we want the other bits at all.
(this PR was originally adding
stat
support, but that got split out and merged separately)This is mostly a draft until https://github.com/ocaml-multicore/ocaml-uring/pull/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.