Skip to content

Fix for slab buffer retention, leading to large memory consumption - #153

Merged
einaros merged 1 commit into
websockets:masterfrom
jmatthewsr-ms:master
Apr 9, 2013
Merged

Fix for slab buffer retention, leading to large memory consumption#153
einaros merged 1 commit into
websockets:masterfrom
jmatthewsr-ms:master

Conversation

@jmatthewsr-ms

Copy link
Copy Markdown
Contributor

@jmatthewsr-ms

Copy link
Copy Markdown
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

einaros commented Jan 26, 2013

Copy link
Copy Markdown
Contributor

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

@jmatthewsr-ms

Copy link
Copy Markdown
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

3rd-Eden commented Apr 9, 2013

Copy link
Copy Markdown
Member

@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

einaros commented Apr 9, 2013

Copy link
Copy Markdown
Contributor

@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

3rd-Eden commented Apr 9, 2013

Copy link
Copy Markdown
Member

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

@jmatthewsr-ms

Copy link
Copy Markdown
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
Copy Markdown

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