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

Potential issue in UDP ping-pong benchmark #38

Open
Corendos opened this issue Apr 6, 2023 · 0 comments
Open

Potential issue in UDP ping-pong benchmark #38

Corendos opened this issue Apr 6, 2023 · 0 comments
Labels
bug Something isn't working core

Comments

@Corendos
Copy link
Contributor

Corendos commented Apr 6, 2023

What's the issue

In the ping-udp1.zig benchmark, there is a potential issue that can arise depending on the order of execution of read/write callbacks.

If everything is fine, the write callback will run first, thus setting the associated Completion to a .dead state and ready to be used again.

However, if the read callback runs first, here is what happens:

  • During the read callback, we call the Pinger.write(...) method and enqueue a write operation using the Pinger.c_write field. This has the effect to put the completion in the list of completion to submit on the next tick
  • Then, when the write completion is executed, we set the state to the Completion to .dead
  • When the time has come to submit the awaiting completion, the c_write completion is discarded because its state is set to .dead
  • There is sort of a livelock, because the read operation is waiting for a write operation that will never happen

Context

I noticed that while working on the IOCP backend. Somehow, the read completion was handled before the write operation and it lead to the described behavior.

Potential solutions

Solution 1 - Simple but kind of misses the point of the bench

Instead of queuing the read operation at the same time as the write operation, we can wait for the write to be completed. However, this introduce artificial latency and despite making the behavior correct, it doesn't really bench anything as operation are serialized.

Solution 2

Another approach would be to wait for both callbacks to have been executed before enqueuing new operations. That would solve the issue of the c_write completion to be reused before its callback has been called.

@mitchellh mitchellh added bug Something isn't working core labels Apr 18, 2023
mitchellh added a commit that referenced this issue Sep 21, 2023
# Description
This PR brings support for Windows using the [I/O Completion
Port](https://learn.microsoft.com/en-us/windows/win32/fileio/i-o-completion-ports).

Closes #10 

# Notes for reviewers
Unfortunately, this is a pretty beefy PR. I tried to clean the git
history so that it's kind of reviewable commit by commit though.

My approach was similar to what @mitchellh suggested me: first the
backend core, then the watchers building on top of it.

I also added support for Windows tests in GH Actions as he told me he
didn't have access to a Windows machine.

Please note that this work is far from perfect, it was my first time
playing with IOCP so expect some weirdness/mistakes.

## Known pain-points/limitations
**Timers**
Timings rely on `GetQueuedCompletionStatusEx` timeout parameter. I read
multiple times that timings on Windows were, well, slightly inaccurate
(plot twist: that's a euphemism). I'm not confident enough to deep dive
into this rabbit hole, so if somebody has more knowledge on that, feel
free to improve my implementation.

**Async**
I have the feeling that there is a better way to implement the `Async`
watchers compared to what I did. I couldn't come up with a better way
than inspiring myself from the `WASI` backend.

The part I struggled the most with was the fact that `Async` can be
notified before being linked to a `Loop`. This requires to store the
fact that it's notified inside the `Async` struct, but then it opens up
a lot of other questions and failed to find a satisfying solution taking
care of all of them.

I'll try to come back to it with a fresh mind but I'm open to
suggestions in the meantime 😁

**Completion port `HANDLE` registering**
In order to be able to wait for asynchronous I/O, `HANDLE` needs to be
linked to a Completion Port. However, you can only register it once
otherwise, the linking call fails. I first tried to let the user handle
that by giving the possibility to link a handle to a `Loop`, but that
ended up being unreliable and cumbersome when implementing Watchers.

My final decision was to ignore the failure and suppose that it always
works. Win32 documentation is not what we could call exhaustive, so I
don't if it's possible for an association to fail on the first call to
`CreateIoCompletionPort` with a newly created `HANDLE`. Worst scenario
is that it fails silently and asynchronous wait on this `HANDLE` never
completes. If anyone has an idea as to how we could properly solve that,
don't hesitate 😁

**UDP Benchmark tweaks**
This is described in #38

**Process support**
I didn't implement support for `Process` watchers yet. I feel like it's
not required to merge this (already big) PR. I'll see if adding support
is possible in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

No branches or pull requests

2 participants