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

Let IO#sync only affect writes, introduce `IO#read_buffering?` and use it in urandom #6304

Merged
merged 1 commit into from Jul 1, 2018

Conversation

Projects
None yet
5 participants
@asterite
Contributor

asterite commented Jun 30, 2018

Fixes #6303

Being able to disable read buffering for IO::Buffered is good. However, having sync affect both reads and writes makes it easier to enable just buffering for one direction: for sockets it's better to not have write buffering by default, so whatever you write goes directly to the socket (you can always set sync = false and manually flush, which is what we do in a few places), but reads should always be buffered for performance reasons.

With this PR we could release 0.25.2 as a bugfix release for this performance regression.

Then for 0.26.0 we could rename sync to write_buffering, or just leave it as sync. Maybe write_buffering is more explicit, even though it's a breaking change. But then, disabling read buffering is a very uncommon use case (urandom... what else?), so maybe having sync and read_buffering as separate, asymmetric properties isn't that bad.

(I know I could have used getter or setter for this, but given that sync doesn't use them I decided to follow that style. If someone wants to do a follow up PR to "improve" that (it's not really an improvement, because macros are slower to compile and it's just one/two line differences), then please go ahead).

@RX14 RX14 added this to the Next milestone Jun 30, 2018

@RX14

RX14 approved these changes Jun 30, 2018

@asterite

This comment has been minimized.

Contributor

asterite commented Jun 30, 2018

By the way, to review this you can see 4e0dc9e. It's more or less the same code, except that when using sync for reads it now uses !read_buffering?

@sdogruyol sdogruyol merged commit 75ba915 into crystal-lang:master Jul 1, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sija

This comment has been minimized.

Contributor

Sija commented Jul 1, 2018

As this PR fixes serious perf regression IMO it should go into 0.25.2.

@asterite asterite deleted the asterite:bug/6303-io-sync-buffered-read branch Jul 6, 2018

@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment