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

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

Merged
merged 1 commit into from
Apr 9, 2013

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.

nodejs/node-v0.x-archive#4660

@indexzero indexzero reopened this Mar 22, 2013
@indexzero
Copy link
Contributor

@jmatthewsr-ms I'm reopening after @3rd-Eden bought it to my attention because because it appears the fix for underlying issue in node core was not resolve. Is this correct?

@jmatthewsr-ms
Copy link
Contributor Author

Yes. My understanding is that this won't be fixed until possibly 0.12:
jmatthewsr-ms/node-slab-memory-issues#1
nodejs/node-v0.x-archive#4964

@trevnorris
Copy link

You're correct. It will probably take the entire v0.11 development cycle to vet the new allocator. Also note that it currently just mirrors the way Buffers pool data. The first step is just to get the thing working. This will require a substantial overhaul. Then once everything is allocating from the same source we can focus on improving the allocation algorithm.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Apr 9, 2013

@indexzero @mmalecki

I'd advise us to accept this pull request. I've been doing a lot of WebSocket proxy tests lately because I was interested in to seeing how our proxy solution compares to other proxies such as nginx and haproxy.

I've deployed the proxy on a 512mb joyent virtual machine running the latest ubuntu and hit it using observing/thor with:

thor --workers 6 --amount 2000 --concurrent 100 --messages 100 <url>

I saw a peak memory of 280mb before this patch. After applying this patch and re-running the command it saw a maximum of 102mb which is significant decrease. So even if this is going to be fixed in later version of Node. It makes sense to pull this asap.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Apr 9, 2013

Re-ran the test suite and everything is passing now, it was probably a failure on travis side that marked these commits as broken.

@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.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Apr 9, 2013

@jmatthewsr-ms yes, it's being worked on but it would only be made available in node 0.12, which still another stable release away and as we have no idea how long it will take before 0.12 is released, it makes sense to merge this in IMHO (as well as in all other projects).

@jmatthewsr-ms
Copy link
Contributor Author

Agreed.

indexzero added a commit that referenced this pull request Apr 9, 2013
Fix for slab buffer retention, leading to large memory consumption
@indexzero indexzero merged commit 3763dc9 into http-party:master Apr 9, 2013
@trevnorris
Copy link

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