fpco / streaming-commons

Common lower-level functions needed by various streaming data libraries
MIT License
36 stars 41 forks source link

Hardcoded default Recv buffer size #54

Closed erebe closed 4 years ago

erebe commented 4 years ago

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 https://github.com/fpco/streaming-commons/commit/8e38589efb60d38e10ece0c7987186efd852fffc#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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Thanks :)