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

Hardcoded default Recv buffer size #54

Closed
erebe opened this issue Jan 4, 2020 · 3 comments
Closed

Hardcoded default Recv buffer size #54

erebe opened this issue Jan 4, 2020 · 3 comments

Comments

@erebe
Copy link
Contributor

erebe commented Jan 4, 2020

Hello,

While I was investigating for some performance improvements, I found out that streaming-commons hardcode the default socket recv buffer size to 32kb https://github.com/fpco/streaming-commons/blob/master/Data/Streaming/Network.hs#L267
It was introduced by this commit
8e38589#diff-8c54fc2d40ad45803c6889efbb0192bbR278

The problem is that this 32kb can now be too low for new system which default to higher values, like 128Kb, and cause unnecessary CPU usage and too many syscall.
This hardcoded value also impair flexibility as it forbid the system administrator to modify kernel settings, like /proc/sys/net/ipv4/tcp_rmem, and let the application benefit of it.

The default read buffer size should be based on the kernel setting instead of an hardcoded value.
If I make a PR to something like this, do you think it has a chance to be merged ?

 {-# NOINLINE defaultReadBufferSize #-}   
defaultReadBufferSize ::  Int
defaultReadBufferSize = unsafeDupablePerformIO $
  bracket (N.socket N.AF_INET N.Stream 0) N.close (\sock -> N.getSocketOption  sock N.RecvBuffer)
@snoyberg
Copy link
Member

snoyberg commented Jan 4, 2020

I'm not opposed to putting in some kind of a better default based on the system, but I don't think the diagnosis here is correct. You're saying that the buffer size is hard coded, but the commit you link to provides a setReadBufferSize to modify the buffer size.

@erebe
Copy link
Contributor Author

erebe commented Jan 4, 2020

I'm not opposed to putting in some kind of a better default based on the system, but I don't think the diagnosis here is correct

Ok I will propose a PR and let you judge

You're saying that the buffer size is hard coded, but the commit you link to provides a setReadBufferSize to modify the buffer size.

Indeed, but one have to think about using it in the first place. My comment is more about the default setting being hardcoded instead of relying on the system value

erebe pushed a commit to erebe/streaming-commons that referenced this issue Jan 4, 2020
erebe pushed a commit to erebe/streaming-commons that referenced this issue Jan 5, 2020
erebe pushed a commit to erebe/streaming-commons that referenced this issue Jan 5, 2020
snoyberg added a commit that referenced this issue Jan 5, 2020
#54 Use system value for defaultReadBufferSize
@erebe
Copy link
Contributor Author

erebe commented Jan 5, 2020

Thanks :)

@erebe erebe closed this as completed Jan 5, 2020
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

No branches or pull requests

2 participants