Skip to content

Add a faster Windows specific rb_io_wait() implementation#417

Merged
larskanis merged 1 commit intoged:masterfrom
larskanis:speedup-windows
Jan 22, 2022
Merged

Add a faster Windows specific rb_io_wait() implementation#417
larskanis merged 1 commit intoged:masterfrom
larskanis:speedup-windows

Conversation

@larskanis
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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