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

Improve performance & lower memory usage #175

Closed
DiegoRBaquero opened this issue Sep 16, 2016 · 18 comments
Closed

Improve performance & lower memory usage #175

DiegoRBaquero opened this issue Sep 16, 2016 · 18 comments

Comments

@DiegoRBaquero
Copy link
Member

Although we have made several improvements and clear memory leaks, everything can always be improved.

In order for the webtorrent-webrtc-based projects to reach more users easily, the trackers need to be able to scale with greater performance.

At the time of writing, 1K connected users go for about 500MB of RAM and each connected user is around 0.45MB (BTorrent's tracker)

@feross
Copy link
Member

feross commented Oct 6, 2016

@DiegoRBaquero Agreed, this is important.

From your testing, do you know which objects are using the most memory?

@DiegoRBaquero
Copy link
Member Author

@feross Took a heapdump: https://webseed.btorrent.xyz/heapdump-1587638310.868854.heapsnapshot

Sintel in the tracker memory:
sintel

Other ways to approach this problem:

  • REST tracker over HTTP2
  • FIFO peers stack

@DiegoRBaquero
Copy link
Member Author

DiegoRBaquero commented Oct 14, 2016

@feross

Tested uWS and it was a HUGE improvement (from 450MB to 30MB RAM for 800+ peers). The only thing lost is the real IP address (which is never used, actually protects peers' privacy) and the full headers of the request (Again, protects peers' privacy from trackers' operators but would not allow filtering or blocking).

@alxhotel @jakefb You might want to check this out.

@yciabaud
Copy link
Contributor

That looks interesting! Nice work! Have you looked at the heap when using µWS?
Do you thing we could make significant memory optimizations in our codebase?

@DiegoRBaquero
Copy link
Member Author

@yciabaud I didn't even bother, but I could get a heapshot if you'd like to see if there's anything else we can improve.

We are only left with clearing 1-dead-peer-only zombie infoHashes (#164)

By the way, guys, could I get comments on: https://medium.com/@diegorbaquero/%C2%B5ws-as-your-next-websocket-library-d34209686357#.twgf337zc

Thanks

@lpinca
Copy link
Contributor

lpinca commented Oct 14, 2016

@DiegoRBaquero interesting. Did you try to disable permessage-deflate in both ws and uws? I know that the overhead for ws is pretty big. Also to reduce memory usage a little you can try to null the upgradeReq object when the 'connection' event is emitted, uws does this under the hood.

wss.on('connection', (ws) => {
  ws.upgradeReq = null;
});

I'm actually not sure if this works as expected.
It might also be interesting to see if installing ws from master makes any difference. There are some improvements, including breaking changes, that have not been released yet.

npm i websockets/ws bufferutil

@lpinca
Copy link
Contributor

lpinca commented Oct 14, 2016

I also see that the clientTracking option for ws is set to true (default value). It seems that this lib uses its own way to track clients so you may want to set it to false.

@DiegoRBaquero
Copy link
Member Author

@lpinca Had not try either the options you describe, had I seen them in the docs por better performance I would have tried them before.

I'll be monitoring uWS performance for the weekend and PR the change on monday if everything is cool.

@lpinca
Copy link
Contributor

lpinca commented Oct 16, 2016

@alexhultman

I'm actually not sure if this works as expected.

I'm not sure because there might be other references that prevent the upgradeReq object from being GC'd and yes it's not exactly the same of uws as ws uses net.Socket so that object (net.Socket) will never be GC'd.

Well, you certainly seem to know a lot about the upgradeReq since you have been insinuating all over that ws could easily scale to µWS levels if upgradeReq was removed.

I've never said that ws could scale at the same levels of uws, if I did show me where please. I've only said that nulling the upgradeReq object in ws could reduce memory usage and that your numbers are wrong and they still are. permessage-deflate seems to have serious issues in ws, see websockets/ws#804.

Instead of acting like some sort of dictator inside of the Primus camp? I have told you ages ago that you have to remove the upgradeReq in the µWS transformer

This makes me sad... Did you actually read why we can't do that? I guess not. This is what we are doing now: https://github.com/primus/primus/blob/uws/transformers/uws/server.js#L92-L95. If you know how to improve it further given our constraints please tell us or send a pr. You have banned me from uWebSockets but I haven't banned you from Primus because I think every contribution is valuable even the ones you don't agree with and the ones that are not easy to handle but I'm the dictator, ok.
Want to discuss about Primus? I'm all for it but it's not relevant to this issue. Let's do it somewhere else.

I've only suggested how to improve ws performance here.

@yciabaud
Copy link
Contributor

@DiegoRBaquero can we move forward on this? Do you still think we should switch to uWS? Did you test disabling permessage-deflate?

@DiegoRBaquero
Copy link
Member Author

@alxhotel @jakefb Could you guys update to latest and test memory consumption with per message deflate and client tracking off in ws?

@alxhotel
Copy link
Member

@DiegoRBaquero Done! Results from the last 12 hours are aprox: 5x less memory consumption and 3x more network bandwidth.

@DiegoRBaquero
Copy link
Member Author

@alxhotel Were there any restarts due to errors?

I think I caused a restart when loading /stats, which evicts old peers.

@ghost
Copy link

ghost commented Feb 11, 2017

If you want a sweetspot between low memory usage and low network usage you can spend some time optimizing the WebTorrent protocol. I assume it is based on JSON right now and that is very ineffective vs something binary. I bet you can improve it a ton in size.

@feross
Copy link
Member

feross commented Feb 14, 2017

@alexhultman We could try switching back to raw bencoding since that's what the other tracker implementations (http and udp) use. But we would need to consider a transition period where we supported both bencoding/json for several months until clients are updated.

@alxhotel
Copy link
Member

@DiegoRBaquero Yep. See issue #205

@Simplici
Copy link

Simplici commented Aug 3, 2017

I seeded a 4G file, it had token almost 8G memory.

@github-actions
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants