-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
Configurable limits #610
Comments
Things to be covered (directly or indirectly) by defined limits:
|
Shall I tackle this one? |
Hi, you're welcome! I guess we'd need to discuss implementation, I didn't try to go into great detail here and I suppose implementing memory limits would be not so trivial. I'll post my thoughts here. Meanwhile, feel free to start with this part (it's anyway should be dome first I think):
I was planning to open a separate issue for parse_size() and already prepared a text:
|
I've updated the issue description and simplified the set of limits a bit. Here are some thoughts regarding implementation. As mentioned in the issue, we want to limit memory per-socket, per-session, and globally - so far it's the simplest yet reliable way I can think of. These three limits will cover the whole flow:
Generally, roc_netio allocates packet and adds it to network queue. Then roc_pipeline fetches it and routes it to session, or maybe moves it to prebuf queue and routes to session later (see #609). Please also take a look at this two pages:
So I came up with the following limits:
Here is how the user may use this limits:
Depending on use case, the user may want to set only some of the limits, or all of them for most fine-grained control. Besides packets, session pipeline allocates other things: pipeline elements, sample buffers, and other temporary buffers (e.g. for FEC). However, in the end, all allocations are done either via IArena or via IPool, and they are very good candidates where we can implement limits. Since there are multiple consumers that are subject to the same limit, it feel natural that we need to develop a limiter that is independent of consumer, that will be shared by pools, arenas, etc. We currently have core::RateLimiter. By analogy, we can implement core::MemoryLimiter. It can have methods like Then we could implement decorators for arena and pool: LimitedArena and LimitedPool. LimitedArena would have a reference to inner IArena + a reference to MemoryLimiter. Before making an allocation, it would ask limiter whether it can acquire more memory. And in deallocation it will tell limiter that memory is releases. Same for LimitedPool. This will significantly simplify memory limiting for sessions. Currently all sessions share same arena & pool. Now we just need to pass to each session its own instance of LimitedArena and LimitedPool, like this:
We can also extend this approach to add global limit too:
The same approach can be used for sockets: we can create per-socket LimitedPool wrapper for shared pool. There is one more thing that we need to handle. When packet is routed from network/prebuf queue to a session, it actually becomes subject of other limit. Before routing, packet should be covered by per-socker limit, and after routing, it should be covered by per-session limit. To ahieve this, we can store a reference to its owning MemoryLimiter in the packet. When roc_piepline routes packet to a session, it should release packet from its original per-socket limiter and acquire it in its new per-session limiter. We could add a method like It should be easy to split implementation into multiple steps/PRs. E.g. first add MemoryLimiter + tests, then LimitedArena, then LimitedPool, then implement per-session limits, then per-socket, then global. And before all this, it would be nice to implement "refactor current options" section first, so that new options will be consisten with existing ones. In the end we'll also need to expose limits in C API. Let me know what do you think about all this! |
BTW, if it feels a bit too big, feel free to work on only part of the issue - the issue is not urgent and really any help is appreciated here. |
One more change that we'll need to do to make this approach work. We have Since now roc_pipeline will need access to pools (to wrap them with LimitedPool adapter), we should change it:
|
Thanks for the awesome detail. Appreciate it. I'll go with your suggestions. I haven't gone into enough codebase areas to provide any opinions on the approach, but will let you know, if anything comes up. I'll start on this probably tomorrow. I think 1 PR for the initial renaming work is the first thing. I'll then see if I can break up the remaining big chunk of work into separate PRs. |
One more small thought: since now we'll have more than one implementation of IPool (Pool and LimitedPool), it makes sense to rename Pool to something more specific, say, SlabPool. I've updated the task. |
I will start work on this. I'll create a PR for the classes first (with tests) and then another one to integrate them into system. |
Great! I think "factories" part from task description may be extracted into separate PR too. |
Starting on this. |
This issue tracks progress of implementing various limits.
Steps:
The text was updated successfully, but these errors were encountered: