Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

noalloc #12

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

noalloc #12

wants to merge 5 commits into from

Conversation

craff
Copy link
Contributor

@craff craff commented Aug 6, 2023

Here is my PR with noalloc. For untagged, I finally think it is too ugly to use Obj.magic and probably not worth it.
So I probably will stop at that PR for now.

@@ -40,14 +40,19 @@ end

type t = Unix.file_descr (* epoll fd *)

external caml_polly_add : t -> Unix.file_descr -> Events.t -> unit
external caml_raise_unix_error : string -> 'a = "caml_raise_unix_error"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we need this. We can raise an exception simply from OCaml without calling into C first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because C will get the errno last set by the unix call and position it in the exception.
We probably should use that for eventfd though.

Copy link
Contributor Author

@craff craff Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, why to you define __FUNCTION__ in eventfd.read and write ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__FUNCTION__ is not defined in OCaml 4.08, unfortunately. I could have used __LOC__ instead.

lib/polly.ml Outdated
let add = caml_polly_add
let add t fd e =
let r = caml_polly_add t fd e in
if r = -1 then caml_raise_unix_error "Polly.caml_polly_add"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a bit more defensive to test for r < 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@@ -35,19 +35,25 @@ CAMLprim value caml_polly_create1(value val_unit)
CAMLreturn(val_res);
}

/* necessary because of changes from 4.X to 5.X in ocaml,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be cut as we can raise any exception we like from OCaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was meaning that uerror is a macro in 0Caml 5 and caml_unix_error (or something like that is a new function).
We have to define a function if we want external to work. My comment is not very clear, I update it.

@@ -40,14 +40,19 @@ end

type t = Unix.file_descr (* epoll fd *)

external caml_polly_add : t -> Unix.file_descr -> Events.t -> unit
external caml_raise_unix_error : string -> 'a = "caml_raise_unix_error"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__FUNCTION__ is not defined in OCaml 4.08, unfortunately. I could have used __LOC__ instead.

let del t fd = caml_polly_del t fd Events.empty
let del t fd =
let r = caml_polly_del t fd Events.empty in
if r < 0 then caml_raise_unix_error "Polly.del"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to preserve errno it should be returned by the C function. With multiple threads there is no guarantee that it is still valid for call_raise_unix_error to pick it up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code may work correctly as it is currently written, but it relies on some fragile properties.

errno is typically per-thread (if it wasn't then there would be no safe way to read it in a multi-threaded program).
But if any code would be executed inbetween return from epoll_ctl and reading the errno inside the other C stub then errno may have already been overwritten. This could happen if a signal is delivered to the program, which would be executed the next time OCaml checks for pending signals. The way the code is currently written it doesn't allocate between the call the caml_polly_... and the call to caml_raise..., but that may break easily if the code is changed in the future (e.g. someone adds a debugging statement).
Newer version of OCaml may use an [@poll error] attribute to detect these cases, but it'd be simpler and more robust to do as suggested and return -errno on error.

@craff
Copy link
Contributor Author

craff commented Aug 7, 2023 via email

@lindig
Copy link
Owner

lindig commented Aug 10, 2023

The errno is positive but it could be handled like this:

static value
caml_polly_ctl(value val_epfd, value val_fd, value val_events, int op)
{
  CAMLparam3(val_epfd, val_fd, val_events);
  int rc;
  struct epoll_event event = {
    .events = (uint32_t) Int_val(val_events),
    .data.fd = Int_val(val_fd)
  };
  rc = epoll_ctl(Int_val(val_epfd), op, Int_val(val_fd), &event);

  CAMLreturn(Val_int(rc >= 0 ? rc : -errno));
}

Now negative numbers carry the errno and they are returned in one go.

While `[@noalloc] might reduce a little overhead, I am not sure these optimizations are worth it. It would be better to spend effort to benchmark the actual wait calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants