Skip to content

Conversation

jmatthewsr-ms
Copy link
Contributor

@jmatthewsr-ms
Copy link
Contributor Author

Putting this on hold as I believe that a node core fix is the most appropriate. Buffer can also retain an 8k slab on it's own, ideally node core should emit a buffer that is not backed by any slab.

@einaros
Copy link
Contributor

einaros commented Jan 26, 2013

Thanks for looking into it, though. Have you sent a pullreq to the joyent/node repo?

@jmatthewsr-ms
Copy link
Contributor Author

nodejs/node-v0.x-archive#4660

Not getting very far at the moment though. If you feel this should be a node core change then please add your comments. Thanks.

@3rd-Eden
Copy link
Member

3rd-Eden commented Apr 9, 2013

@einaros I think it makes sense to pull this anyways, as it's probably going to take a while before node fixes their shit.

I've seen great reduction in memory usage for the http-proxy without any side affects in terms of latency or handshake performance.

@einaros
Copy link
Contributor

einaros commented Apr 9, 2013

@3rd-Eden, I'm good with that. I'll merge it later today.

Also, you've got push access to the repo now, so you are free to make such changes as well :)

@einaros einaros reopened this Apr 9, 2013
einaros added a commit that referenced this pull request Apr 9, 2013
Fix for slab buffer retention, leading to large memory consumption
@einaros einaros merged commit 6a6b4ef into websockets:master Apr 9, 2013
@3rd-Eden
Copy link
Member

3rd-Eden commented Apr 9, 2013

@einaros Yeah, thanks for that :) Just wanted to get your opinion on this.

@jmatthewsr-ms
Copy link
Contributor Author

Note that this is being worked on in node core: nodejs/node-v0.x-archive#4964.

The buffer copy in this fix may not be required once node core buffer is fixed. The fix here shouldn't hurt performance if left in, but worth checking once node core is updated.

@trevnorris
Copy link

for anyone who might want to test, the SlabAllocator has been completely removed from my working branch (https://github.com/trevnorris/node/tree/buffer-buffet). Would be interested to know if this fixes the memory leak.

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

Successfully merging this pull request may close these issues.

4 participants