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.

Sign up for GitHub

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

Jump to bottom

feat: add ProcessSocketNotifications backend #210

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

feat: add ProcessSocketNotifications backend #210

wants to merge 7 commits into from

Conversation

Berrysoft
Copy link

@Berrysoft Berrysoft commented Jun 11, 2024 •
edited
Loading

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
Copy link
Author

Berrysoft commented Jun 11, 2024 •
edited
Loading

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

@notgull
Copy link
Member

notgull commented Jun 11, 2024

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

@Berrysoft
Copy link
Author

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
Copy link
Author

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.

@notgull
Copy link
Member

notgull commented Jun 12, 2024 •
edited
Loading

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).

@Berrysoft
Copy link
Author

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.

@notgull
Copy link
Member

notgull commented Jun 15, 2024

  • 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@Berrysoft @notgull