-
Notifications
You must be signed in to change notification settings - Fork 463
[Performance Optimization] Use read instead of readfull #1026
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
base: main
Are you sure you want to change the base?
Conversation
|
I like this change, and I don't think a slight performance impact on older rubies is a material concern. |
5dcf1b4 to
4ebd9f8
Compare
|
@grcooper Do we know why we're seeing repeated test failures? |
|
On our fork in Shopify/Dalli we fixed this by removing the usages of I have another branch where I am thinking of replacing We also noticed a (kind of) bug that I think is leading to this behaviour not being as apparent, as everything is returned as a See: Shopify#31 So I think in order to merge this I want 3 other prs:
Those two should help this PR along. However, then we should also add the PR above (31 from our fork) to have more appropriate errors being raised and not just |
|
I started the rollout of some of the above fixes here: #1030 |
Replace the manual `readfull` loop with Ruby's built-in `IO#read` method. Modern Ruby (3.1+) handles non-blocking reads internally with proper timeout support via `IO#timeout=`, making the manual loop unnecessary. This provides ~7% performance improvement on read operations based on benchmarks from PR #1026. The `readfull` method is preserved in socket.rb for backwards compatibility but is no longer used internally. Based on work by @grcooper in PR #1026. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace the manual `readfull` loop with Ruby's built-in `IO#read` method. Modern Ruby (3.1+) handles non-blocking reads internally with proper timeout support via `IO#timeout=`, making the manual loop unnecessary. This provides ~7% performance improvement on read operations based on benchmarks from PR #1026. The `readfull` method is preserved in socket.rb for backwards compatibility but is no longer used internally. Based on work by @grcooper in PR #1026. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
I've slotted this for 5.0 where I'm dropping 3.1 support |
The latest versions of Ruby us the underlying
read(2)non-blocking socket command under the hood. We no longer need to useread_fullmanually and interrupt for simple reads from the socket as Ruby will do that for us.This might be a slight performance negative to people using threaded models on older versions of ruby.
This was mostly used in the
getcommand and you can see the biggest performance difference there:I have left
read_availableas it is still needed when we have multiple servers we are selecting from in aget_multiusing IO.select.