Add Unix.nonblock_single_write and Unix.nonblock_read #13024

craff · 2024-03-11T18:21:14Z

This function are around 30% faster when avoiding the copying which is necessary if one release the global lock.
They are very useful for fast modern server doing non blocking IO. This addresses issue #11992. Note I could not test the windows code!

craff · 2024-03-12T08:00:09Z

I did more serious benchmarks (see code) and the gain is between 10 and 20%, larger with larger buffer, arround 5% with 1 byte buffer, but very noisy measure! My laptop is not very stable for benchmark. I would be happy if someone could test on a stable computer for benchmark

Here is the test I use (nonblocking IO, without select):
bench.ml.gz

Compile with:
/usr/local/bin/ocamlopt -I +unix unix.cmxa bench.ml

Run with (second integer is buffer size, first is number of buffer written):
./a.out foo.txt 100000 65536

OlivierNicole · 2024-03-14T13:22:20Z

I obtain similar speedups in the benchmark (between 25 and 30 %). I am reviewing this PR.

craff · 2024-03-14T18:16:44Z

I obtain similar speedups in the benchmark (between 25 and 30 %). I am reviewing this PR.

My laptop was not so good, I am happy that you got 25-30%. I was considering write also the code for write/read on big array. Should this be in the same PR ?

OlivierNicole · 2024-03-15T11:27:35Z

We probably should wait for the opinion from a maintainer about the current design before extending it. Personally, I think the addition is pleasantly self-contained. Sure, the API doesn’t prevent its user from calling the functions on blocking file descriptors, but the documentation makes this danger very explicit. Preventing such blocking calls by storing the blocking/non-blockingness in the file descriptor would require a much larger PR and I’m not sure the gain would be worth the cost.

OlivierNicole

The approach seems sane, I just noticed a few issues in the details of the code. We should also test that the functions run as expected on Windows.

Edit.: thanks for the contribution!

otherlibs/unix/read_unix.c

otherlibs/unix/read_win32.c

otherlibs/unix/write_unix.c

otherlibs/unix/write_win32.c

otherlibs/unix/read_win32.c

otherlibs/unix/unix_win32.ml

otherlibs/unix/write_win32.c

otherlibs/unix/unixsupport_win32.c

otherlibs/unix/write_win32.c

Co-authored-by: Olivier Nicole <[email protected]>

craff · 2024-03-25T03:16:52Z

We really need to test this code on a windows platform. Just running the benchmark given above + may be a test for some errors.

OlivierNicole · 2024-03-27T14:09:40Z

I agree, and one way to do it would be to add a small test of these functions in tests/lib-unix/. Maybe a test of a domain performing a socket read/write at the same time as another domain forces a GC.

However I don’t want to encourage to implement without being sure that the PR has sufficient agreement. I would be more at peace if another maintainer validated the current design.

nojb

I left some technical comments. Going over the discussion in the motivating issue #11992, there was no opposition but also no clear opinion in favour by the other participants (@gasche and @xavierleroy). Technically, the PR seems sound, but the "safe" part of it is a bit unsatisfying (if we use this call with a blocking file descriptor, the whole system will block). @gasche, @xavierleroy: any opinions?

otherlibs/unix/read_unix.c

otherlibs/unix/unix_win32.ml

otherlibs/unix/unix_unix.ml

nojb · 2024-03-28T08:00:37Z

otherlibs/unix/unixsupport_unix.c

@@ -321,6 +321,12 @@ void caml_uerror(const char *cmdname, value cmdarg)
 caml_unix_error(errno, cmdname, cmdarg);
 }

+CAMLprim void caml_unix_uerror(value msg, value cmdarg) {
+ CAMLparam0();
+ caml_uerror(String_val(msg), cmdarg);


Looking at the source of caml_uerror, it looks like this is safe (the GC will not be triggered before String_val(msg) is copied), but it is subtle and fragile (a change in caml_uerror can silently cause a segfault here). Can we find a more robust way of writing this code?

nojb · 2024-03-28T08:01:52Z

otherlibs/unix/read_win32.c

+ if (Descr_kind_val(fd) == KIND_SOCKET) {
+ SOCKET s = Socket_val(fd);
+ ret = recv(s, &Byte(buf, ofs), len, 0);
+ if (ret == SOCKET_ERROR) ret = -1;


The error handling here is missing something: in case of error you need to call caml_win32_maperr(WSAGetLastError()) in order to set errno correctly. See what is done in the usual read call for inspiration.

nojb · 2024-03-28T08:04:09Z

otherlibs/unix/read_win32.c

+ // The write handle for an anonymous pipe has been closed. We match the
+ // Unix behavior, and treat this as a zero-read instead of a Unix_error.
+ ret = 0;
+ }


Idem; here one should call caml_win32_maperr(GetLastError()).

nojb · 2024-03-28T08:06:30Z

otherlibs/unix/unix.mli

@@ -390,6 +390,12 @@ val read_bigarray :
 (** Same as {!read}, but read the data into a bigarray.
 @since 5.2 *)

+val nonblock_read : file_descr -> bytes -> int -> int -> int
+(** Same as {!read}, but does not release the global lock nor copy the
+ bytes read. It is only safe to use with non blocking file descriptor,


It is not completely clear what does "safe" means here. What about "It should only be used with ...".

nojb · 2024-03-28T08:07:22Z

otherlibs/unix/write_win32.c

+ if (Descr_kind_val(fd) == KIND_SOCKET) {
+ SOCKET s = Socket_val(fd);
+ ret = send(s, &Byte(buf, ofs), len, 0);
+ if (ret == SOCKET_ERROR) ret = -1;


Error handling in this function is missing the same as in the "read" function.

dbuenzli · 2024-04-21T19:41:33Z

(if we use this call with a blocking file descriptor, the whole system will block)

Indeed and as such I find the name to be quite misleading. It may feel absurd but I think blocking_{read,write} (or another perhaps another convention to find) would be more enlighting to grep for the day this actually happens.

craff · 2024-04-22T04:58:22Z

(if we use this call with a blocking file descriptor, the whole system will block)

Indeed and as such I find the name to be quite misleading. It may feel absurd but I think blocking_{read,write} (or another perhaps another convention to find) would be more enlighting to grep for the day this actually happens.

Using blocking is confusing because it is mainly useful with non-blocking socket
Using non-blocking is confusing because it will block the whole program if you are using blocking socket.

I really do not know a good name ;-)

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

dbuenzli · 2024-04-22T05:45:51Z

Here are a few thoughts:

People tend to tab complete without thinking much. As such between non_blocking and blocking I would rather use blocking. Experts will know what they are doing.
Thinking about ctypes's ?release_runtime_lock:bool argument. Maybe something themed that way, perhaps keep_runtime_lock_and_{read,write}. It's not as if one uses these functions everywhere I don't mind the long name.

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

OlivierNicole · 2024-04-29T13:04:46Z

I like keep_runtime_lock_and_{read,write}.

craff added 4 commits March 11, 2024 08:18

Add Unix.nonblock_single_write and Unix.nonblock_read

596b141

add pull request number in Changes

372597d

sanity

c8eb2c4

windows compilation fix

0faf927

craff force-pushed the write_read_for_non_block branch from 31629bb to 0faf927 Compare March 11, 2024 19:49

Add reviewer Olivier Nicole in Changes

6c82ae7

OlivierNicole requested changes Mar 15, 2024

View reviewed changes

craff and others added 13 commits March 24, 2024 16:12

Update otherlibs/unix/read_unix.c

4dd9b8c

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/write_unix.c

17d03ca

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/write_win32.c

f1acd9c

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/read_win32.c

639bc02

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/unix_win32.ml

04cc282

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/unix_win32.ml

16e4e22

Co-authored-by: Olivier Nicole <[email protected]>

Update otherlibs/unix/write_win32.c

afcfb19

Co-authored-by: Olivier Nicole <[email protected]>

Merge branch 'ocaml:trunk' into write_read_for_non_block

12a4e49

Update read_win32.c

4116bf4

Update write_win32.c

b9b6745

Update read_win32.c (typo)

7e1b677

Update read_win32.c (indentation)

71b2584

Update unixsupport_win32.c

18452b6

Update read_win32.c (tabs)

881112d

nojb reviewed Mar 28, 2024

View reviewed changes

gasche assigned damiendoligez Apr 3, 2024

craff and others added 2 commits April 21, 2024 19:00

Update otherlibs/unix/unix_win32.ml

ce39616

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

Merge branch 'ocaml:trunk' into write_read_for_non_block

86adb09

craff and others added 5 commits April 21, 2024 21:03

fast -> nonblock

f39dc75

lines too long

dc3ec14

Update otherlibs/unix/unix_win32.ml

2c0804c

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

Update otherlibs/unix/unix_unix.ml

8c7f6b1

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

Update otherlibs/unix/unix_unix.ml

9037794

Co-authored-by: Nicolás Ojeda Bär <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unix.nonblock_single_write and Unix.nonblock_read #13024

Add Unix.nonblock_single_write and Unix.nonblock_read #13024

craff commented Mar 11, 2024 •

edited

craff commented Mar 12, 2024 •

edited

OlivierNicole commented Mar 14, 2024

craff commented Mar 14, 2024

OlivierNicole commented Mar 15, 2024

OlivierNicole left a comment •

edited

craff commented Mar 25, 2024

OlivierNicole commented Mar 27, 2024

nojb left a comment

nojb Mar 28, 2024

nojb Mar 28, 2024

nojb Mar 28, 2024

nojb Mar 28, 2024

nojb Mar 28, 2024

dbuenzli commented Apr 21, 2024

craff commented Apr 22, 2024

dbuenzli commented Apr 22, 2024

OlivierNicole commented Apr 29, 2024

Add Unix.nonblock_single_write and Unix.nonblock_read #13024

Are you sure you want to change the base?

Add Unix.nonblock_single_write and Unix.nonblock_read #13024

Conversation

craff commented Mar 11, 2024 • edited

craff commented Mar 12, 2024 • edited

OlivierNicole commented Mar 14, 2024

craff commented Mar 14, 2024

OlivierNicole commented Mar 15, 2024

OlivierNicole left a comment • edited

Choose a reason for hiding this comment

craff commented Mar 25, 2024

OlivierNicole commented Mar 27, 2024

nojb left a comment

Choose a reason for hiding this comment

nojb Mar 28, 2024

Choose a reason for hiding this comment

nojb Mar 28, 2024

Choose a reason for hiding this comment

nojb Mar 28, 2024

Choose a reason for hiding this comment

nojb Mar 28, 2024

Choose a reason for hiding this comment

nojb Mar 28, 2024

Choose a reason for hiding this comment

dbuenzli commented Apr 21, 2024

craff commented Apr 22, 2024

dbuenzli commented Apr 22, 2024

OlivierNicole commented Apr 29, 2024

craff commented Mar 11, 2024 •

edited

craff commented Mar 12, 2024 •

edited

OlivierNicole left a comment •

edited