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

audit implementation of std.io.poll #21565

Open
mlugg opened this issue Oct 2, 2024 · 1 comment
Open

audit implementation of std.io.poll #21565

mlugg opened this issue Oct 2, 2024 · 1 comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@mlugg
Copy link
Member

mlugg commented Oct 2, 2024

std.io.poll is implemented by writing the data from each stream to a std.LinearFifo. However, this implementation is problematic:

  • It requires poll to be able to allocate
  • It leads to issues with the Windows implementation (and any other target which requires using async IO rather than non-blocking IO) because the LinearFifo buffer does not have a stable order; incr-check enhancements, and CI for incremental test cases #21518 works around this by queuing the async read to a 1-byte temporary buffer, and performing larger immediate reads to the FIFO when that read completes, but this incurs syscall overhead
  • It gives users an API which may be overcomplicated for their use case; for instance, if a user of poll wants to process all bytes as soon as they come in, there's no point having a dynamic buffer in the middle

For these reasons, it may be more effective for poll to expose a lower-level API.

One idea would be for poll to read to an internal (or user-supplied) buffer, and for poll to directly return this information. For instance, it could return:

union(enum) {
    all_closed,
    timeout,
    data: struct {
        stream: StreamEnum,
        data: []const u8,
    },
},

The user of the API could choose to write data to LinearFifo if this provides a helpful interface. This would allow more flexibility to the user in terms of using, for instance, a statically-sized LinearFifo if a small upper bound can be placed on the amount of data that must be buffered at a time.

This issue isn't necessary suggesting to go with that exact idea; rather, it is just tracking that this function's implementation/API should be looked at more closely to figure out the optimal API for both users of the standard library and to incur minimal overhead.

@mlugg mlugg added enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. labels Oct 2, 2024
@mlugg mlugg added this to the 0.16.0 milestone Oct 2, 2024
@NicoElbers
Copy link
Contributor

I like the idea of returning a union instead of a bool, in my own code I have a wrapper that does basically that.

However I would suggest it'd be better for a user to either supply buffers per poll call, or some writer like interface at initialization to get buffers (like fifo.writeableWithSize()).

Otherwise you would have to copy over a buffer every time you did a poll, which is wasteful.

This would change the return time to something along the lines of:

union(enum) {
  all_closed,
  timeout,
  data: [self.poll_fds.len]struct {
     stream: StreamEnum,
     read_len: usize,
     is_closed: bool, // Might be nice to notify
  },
},

Adding an interface for this or having to supply your own buffers each call is more complex, but I think it outweighs having to, in many cases, copy over every byte you poll.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

2 participants