status-im / nim-websock

Websocket for Nim
83 stars 15 forks source link

Grow recvMsg buffer instead of pre-allocating 1MB #135

Closed iffy closed 1 year ago

iffy commented 1 year ago

I noticed that each websocket connection to my server was causing memory to increase by about 1MB, until it crashed, running out of memory. I determined that it was allocating a buffer of size WSDefaultFrameSize (1MB) ahead of each recvMsg call.

This PR forgoes the pre-allocated maximum frame size buffer in favor of growing the recvMsg return value by only the amount indicated in the websockets frame header and writing directly to the return value. Now, instead of memory growing by 1MB per connection, it grows by only ~20KB (which includes memory growth from my own code -- I don't know how much each contributes).

In my very minimal testing, tests/testwebsockets.nim runs slightly, but consistently faster on my machine. Same with nim c -r -d:release tests/testwebsockets.nim "should serialize long messages"

My only qualm with this change is that now recvMsg is more tightly coupled to the specific implementation of recv (e.g. setting ws.first = true) and also duplicates some code from recv. I'm open to suggestions for improvements.

Menduist commented 1 year ago

Hey, thanks for the PR! I tried an alternative here: #136 Let me know what you think!

iffy commented 1 year ago

That's a great improvement, yes! I tried passing a var but it couldn't be captured; I didn't even think of using a ref. I like this. Thanks!