polling icon indicating copy to clipboard operation
polling copied to clipboard

feat: add `ProcessSocketNotifications` backend

Open Berrysoft opened this issue 1 year ago • 7 comments

The original iocp backend is moved to iocp/wepoll. Although I would like to reuse the code, it seems that only dur2timeout is reused.

Closes #208

Closes #141

Some comments:

  • A feature iocp-psn is added. The psn backend is much like kqueue one, but different from the original wepoll one. Even the CompletionPacket is different now. I'm not sure if you like it, but my reason is to reduce the allocations:)
  • NtAssociateWaitCompletionPacket support is added to simplify the backend.
  • The psn backend doesn't support one socket being registered to multiple pollers. I have to disable some tests.
  • The edge trigger of psn backend is a little different from other platforms. I have added some comments about that.
  • ~~I haven't filtered the REMOVE event out, and it will be act like an empty event. I'm not sure if it will break some behaviors.~~

Some details:

  • No OVERLAPPED is used or allocated.
  • The Event::key is used as lpCompletionKey.
  • dwNumberOfBytesTransferred contains the event attributes.

Berrysoft avatar Jun 11 '24 11:06 Berrysoft

Well... I'm a little confused about why twice failed. I cannot reproduce it on my machine.

Berrysoft avatar Jun 11 '24 12:06 Berrysoft

Thanks for the PR! Please bear with me, this will take me some time to review.

notgull avatar Jun 11 '24 13:06 notgull

I found that twice sometimes fails. I've checked that the elapsed time is 991.075ms, less than 1s, but the timeout passed to ProcessSocketNotifications is exactly 1000, and it returns with WAIT_TIMEOUT, which means it should have been waiting for 1s. That's weird, but I think it's not my bug.

ProcessSocketNotifications just calls to GetQueuedCompletionStatusEx internally. It should behave the same like the wepoll backend. Maybe it just performs faster...

Berrysoft avatar Jun 11 '24 15:06 Berrysoft

The tests on i686 is another problem. I've reproduced the failing tests, but I don't understand why the same code works on x64 fails on x86.

Berrysoft avatar Jun 11 '24 15:06 Berrysoft

I'm a bit less enthusiastic about this PR at this point. For polling I'd like to keep differences between platforms to a minimum. Having there be weird additional rules for edge-triggered polling on Windows is something I'd like to keep out of the public API for this crate.

To clarify, I'd have have edge-triggered disabled entirely on Windows than have edge-triggered I/O have a different behavior on Windows than Linux. Although if the only value add of ProcessSocketNotifications is edge-triggered I/O I'm not sure it's worth it to have the extra complexity.

(Don't get me wrong, I appreciate the effort! It's just that from the initial issue it was implied that the addition of edge-triggered I/O to polling on Windows wouldn't add any issues).

notgull avatar Jun 12 '24 01:06 notgull

Well, I understand your choice. It's OK to not merge this PR, and I did this only for interest. Now we know the pros and cons of this implementation.

Pros:

  • Simplify code.
  • High performance? (Maybe even too high that cannot pass the twice test...)

Cons:

  • Different behavior in edge triggers.
  • Maybe bugs in WOW64.
  • Only support Windows 10 21H1+

Feel free to close this PR or just ignore it if it doesn't match your needs.

Berrysoft avatar Jun 12 '24 11:06 Berrysoft

  • High performance? (Maybe even too high that cannot pass the twice test...)

This looks like weirdness with the API, rather than performance.

Still, you're probably right. The benefits outweigh the costs. Thanks anyways!

notgull avatar Jun 15 '24 01:06 notgull