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

Add a faster Windows specific rb_io_wait() implementation #417

Merged
merged 1 commit into from
Jan 22, 2022

Conversation

larskanis
Copy link
Collaborator

It is based on the code that was removed in commit 6c885e8

Fixes #416

CC: @jirubio

It is based on the code that was removed in commit
ged@6c885e8

Fixes ged#416
@larskanis larskanis merged commit bba8786 into ged:master Jan 22, 2022
@ariccio
Copy link

ariccio commented Jan 24, 2022

Nit: both WSACreateEvent and WSACloseEvent can fail.

I'm basically the only programmer I've ever met who actually checks these kinds of return codes when writing low level code, but you'd be shocked at the number of bugs it's found, and the number of times it's saved my butt when debugging mysterious problems. Code I've written like this has even found bugs in WINE.

Perhaps you could rb_raise an rb_eConnectionBad, an rb_ePGerror, or a new and more specific exception type, in that case?

I usually find myself wrapping calls to handle-closing functions like WSACloseEvent (or plain old CloseHandle) in a little shim that checks the value, where I usually just std::terminate/abort (since this shouldn't happen unless I missed something), but aborting wouldn't make much sense here. If anything ever goes wrong that causes these to fail and it's not bubbled up to the interpreter, I bet someone will have a bad day :)

If you're compiling with MSVC, you can even annotate the param with _Frees_ptr_, (or _Post_ptr_invalid_, __drv_freesMem) to get static analysis to detect accidental use of closed event handles. You probably don't need this bit, but it's a cool fun fact that I haven't had an excuse to share for a few years now!

@larskanis larskanis deleted the speedup-windows branch January 25, 2022 15:05
@larskanis
Copy link
Collaborator Author

@ariccio Thank you for your notes! I totally agree with you, but I judge this patch as a workaround only and would better invest my time into solving the slowness in ruby core. The code here worked for 11 years now.

@larskanis
Copy link
Collaborator Author

@ariccio If you provide a PR I'll merge it.

larskanis added a commit that referenced this pull request Jan 28, 2022
This fails on Windows, since the introduction of #417
@ariccio
Copy link

ariccio commented Jan 28, 2022

I appreciate it! But my local environment is not setup right for compiling ruby modules right now. You're probably right that it's no big deal. I'll come back to this when I can find time!

Thanks for maintaining this package.

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.

1.3.0 version slow on Windows
2 participants