Minor clean-ups in runtime/io.c and runtime/caml/io.h by xavierleroy · Pull Request #10136 · ocaml/ocaml

xavierleroy · 2021-01-09T17:25:25Z

While investigating #9786, I came across some minor clean-ups in the buffered I/O code:

Remove obsolete support for Cash
Channels opened from C should better not be flushed by Stdlib.flush_all
caml_ml_close_channel and caml_ml_out_channels_list can be written more clearly.

Cash (http://pauillac.inria.fr/cash/) was an experiment in using OCaml for shell scripting. The project is no longer active and the old code does not compile with OCaml 4.

dra27 · 2021-01-10T21:13:14Z

Commit-by-commit: the caml_ml_close_channel and Tag_cons changes LGTM. struct channel is not hidden by CAML_INTERNALS so I don't think we should delete fields - wouldn't it be better just to leave them set to 0 and change the comment in io.h that they are unused and always 0 (i.e. remove the reference to Cash)?

The caml_all_opened_channels change looks correct for what it says, but is that correct in terms of caml_thread_reinitialize - can't channels opened in C also potentially have mutexes on them which need removing?

gadmm

I wondered about the possible impact of the change to caml_all_opened_channels, and concluded that it is fine because opening and closing channels from C is not exposed in the public API. It all looks good to me (apart from @dra27's pending caml_thread_reinitialise question above). A minor comment below.

gadmm · 2021-01-12T16:21:19Z

runtime/caml/io.h

-  int revealed;                 /* For Cash only */
-  int old_revealed;             /* For Cash only */
-  int refcount;                 /* For flush_all and for Cash */
+  int refcount;                 /* For flush_all */


refcount can go too. It is only increased inside caml_alloc_channel to prevent a race with finalisation (#8191), but finalisation no longer happens during allocation from C.

Upon closer inspection refcount needs to stay, but the comment /* prevent finalization during next alloc */ should be fixed to better match the purpose of the reference count :)

The test for refcount > 0 inside caml_close_channel is now pointless and can be changed into an assertion refcount == 0.

I would document

int refcount; /* Number of custom blocks owning the struct */

and remove CHANNEL_FLAG_MANAGED_BY_GC.

gadmm · 2021-01-12T16:56:44Z

struct channel is not hidden by CAML_INTERNALS so I don't think we should delete fields

Here it looks like it is?

dra27 · 2021-01-12T17:08:44Z

Oops, I either didn’t scroll up far enough or was being ditsy - the struct is indeed internal!

Control flow can be simplified, removing the `do_syscall` variable.

To match `Val_emptylist`.

These I/O operations used to be defined as macros in <caml/io.h>, but they are not speed-critical, so it's cleaner to have them as functions, like all other I/O operations in this file.

xavierleroy · 2021-01-21T10:56:11Z

Thank you @dra27 and @gadmm for the helpful comments.

The caml_all_opened_channels change looks correct for what it says, but is that correct in terms of caml_thread_reinitialize - can't channels opened in C also potentially have mutexes on them which need removing?

Well spotted. Maybe one day we'll use such channels in a multithreaded context. I changed the implementation to keep all channels in the C list caml_all_opened_channels, but to omit the channels opened in C from the list built by caml_ml_out_channels_list and used by Stdlib.flush_all.

I updated the comments following @gadmm's suggestions.

One last cleanup: caml_getch and caml_putch used to be macros defined in <caml/io.h>. I see no reasons to keep them exposed as macros, so I turned them into functions, like the other I/O functions in this interface.

dra27

A whitespace nit (I assume?) - I guess you were going to push Changes just prior to merging (the changes are internal, but probably worth having one for naughty people who define CAML_INTERNALS and get singed...)

dra27 · 2021-01-21T20:05:14Z

runtime/io.c

+       Testing channel->fd >= 0 looks unnecessary, as
       caml_ml_close_channel changes max when setting fd to -1. */
-    if (channel->max == NULL) {
+    if (channel -> max == NULL


Suggested change

if (channel -> max == NULL

if (channel->max == NULL

?

Oups! Apologies for this strange typo. I guess those spaces were destined for another window but windows focus wasn't where I expected it... I squashed the fix with the original commit, so that it doesn't show up.

gadmm · 2021-01-21T21:09:49Z

Looks good to me too, but I have two comments:

I believe that you can still remove CHANNEL_FLAG_MANAGED_BY_GC entirely, replacing it with a test refcount > 0, since it is redundant again. This includes otherlibs/win32unix/channels.c.
More a curiosity about the compiler coding style, and since this is about cleaning things up: why do you keep macros Putch and Getch instead of turning them into inline functions? Or if they are not speed-critical as you wrote, why not just use normal functions?

Previously, output channels opened from C were also reported in this list, causing `Stdlib.flush_all` to flush them. This feels wrong, as channels opened from C are entirely managed from C code. With this commit, output channels opened from C always have a reference count of 0: `caml_alloc_channel` is no longer called with one of these channels, and it's the only function that increases refcount. Hence the refcount check in `caml_close_channel` can be removed.

`caml_allc_custom_mem` cannot trigger finalization any longer.

xavierleroy · 2021-01-24T15:55:38Z

I believe that you can still remove CHANNEL_FLAG_MANAGED_BY_GC entirely, replacing it with a test refcount > 0, since it is redundant again. This includes otherlibs/win32unix/channels.c.

I think you're right, but to me this wouldn't make the code clearer. When there is no clear advantage to a code change, I tend to keep the original code.

More a curiosity about the compiler coding style, and since this is about cleaning things up: why do you keep macros Putch and Getch instead of turning them into inline functions? Or if they are not speed-critical as you wrote, why not just use normal functions?

The main uses of these functions outside io.c is in the debugger interface, so that's not critical. On the other hand there are some uses in io.c (caml_getword and caml_putword) where inlining "get char" and "put char" gives nicer code.

This would be a fine use of "extern inline" C99 functions, but I don't know how to achieve the same effect with MSVC. Plus, keeping the macros (just moving them from io.h to io.c) makes the change smaller.

xavierleroy · 2021-01-24T17:45:18Z

Changes entry added. CI is good. Merging!

gadmm · 2021-01-24T17:48:44Z

Thanks for the replies, it would be nice to know for extern+inline functions. I was already wondering the portability of this for something unrelated and I was puzzled that I could not already find instances of it in the codebase.

… rebase

… rebase Mentioned in ocaml#10831 review comment on runtime/io.c:552.

…se (#10975) Mentioned in #10831 review comment on runtime/io.c:552. Co-authored-by: sabine <[email protected]>

gadmm reviewed Jan 12, 2021

View reviewed changes

xavierleroy added 3 commits January 21, 2021 11:19

Refactor caml_ml_close_channel

c2f42c7

Control flow can be simplified, removing the `do_syscall` variable.

Use Tag_cons from <caml/mlvalues.h>

bf8e44c

To match `Val_emptylist`.

Turn caml_getch and caml_putch into functions

f7e32a3

These I/O operations used to be defined as macros in <caml/io.h>, but they are not speed-critical, so it's cleaner to have them as functions, like all other I/O operations in this file.

xavierleroy force-pushed the io-cleanup branch from 5267953 to 20e597d Compare January 21, 2021 10:45

dra27 approved these changes Jan 21, 2021

View reviewed changes

xavierleroy added 4 commits January 24, 2021 16:45

Remove misleading comment

6021888

`caml_allc_custom_mem` cannot trigger finalization any longer.

Document refcount field better

07c3295

Changes entry for ocaml#10136

691655c

xavierleroy force-pushed the io-cleanup branch from 85d1fde to 691655c Compare January 24, 2021 15:48

xavierleroy merged commit 8342ea7 into ocaml:trunk Jan 24, 2021

xavierleroy deleted the io-cleanup branch July 18, 2021 16:31

abbysmal mentioned this pull request Oct 27, 2021

runtime: remove unused fields from io.h ocaml-multicore/ocaml-multicore#724

Merged

sabine added a commit to sabine/ocaml that referenced this pull request Feb 1, 2022

add missing changes from ocaml#10136 that may have been lost during a…

a2901c6

… rebase

sabine added a commit to sabine/ocaml that referenced this pull request Feb 1, 2022

add missing changes from ocaml#10136 that may have been lost during a…

64e4aed

… rebase Mentioned in ocaml#10831 review comment on runtime/io.c:552.

sabine mentioned this pull request Feb 1, 2022

add missing changes from #10136 that may have been lost during a rebase #10975

Merged

xavierleroy pushed a commit that referenced this pull request Feb 1, 2022

add missing changes from #10136 that may have been lost during a reba…

9a7b6ac

…se (#10975) Mentioned in #10831 review comment on runtime/io.c:552. Co-authored-by: sabine <[email protected]>

Conversation

xavierleroy commented Jan 9, 2021

Uh oh!

dra27 commented Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gadmm left a comment

Choose a reason for hiding this comment

Uh oh!

gadmm Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

gadmm Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gadmm Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gadmm commented Jan 12, 2021

Uh oh!

dra27 commented Jan 12, 2021

Uh oh!

xavierleroy commented Jan 21, 2021

Uh oh!

dra27 left a comment

Choose a reason for hiding this comment

Uh oh!

dra27 Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

xavierleroy Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

gadmm commented Jan 21, 2021

Uh oh!

xavierleroy commented Jan 24, 2021

Uh oh!

xavierleroy commented Jan 24, 2021

Uh oh!

gadmm commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dra27 commented Jan 10, 2021 •

edited

Loading

gadmm Jan 12, 2021 •

edited

Loading

gadmm Jan 12, 2021 •

edited

Loading