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

Added configurable mechanism for manually setting static event loop to use for new handles #2450

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

lminiero
Copy link
Member

For some context, please refer to the exchange this comment belongs to, as it's where the idea came from.

To summarize, normally when you use the static event_loops configuration, new handles are automatically added to loops following a round robin distribution. This patch allows you to configure Janus so that you can also optionally use the Janus API itself to tell the core which loop a new handle you're attaching should be added to. The discussion above provides some information on why it may be useful in some cases.

To enable the functionality, event_loops must be configured, and allow_loop_indication set to true (it's false by default). In that case, adding a loop_index to an attach request will instruct the core to manually add the handle to the specified core, rather than doing it automatically. Providing an invalid index will fall back to round robin.

This PR is based on the sendmmsg branch, as #2295 is where we're trying to work on most optimizations. Please test and provide feedback, as that's the fastest way to get this merged soon.

@lminiero
Copy link
Member Author

Merging in the sendmmsg branch.

@lminiero lminiero merged commit e89694e into sendmmsg Dec 11, 2020
@lminiero lminiero deleted the loop-indication branch December 11, 2020 11:22
@fancycode
Copy link
Contributor

As #2295 is on hold, any chance this PR can be merged to master? It sounds like a good optimization on its own.

@lminiero
Copy link
Member Author

lminiero commented Jun 4, 2021

I actually forgot about this... I'll check how easy it is to backport to master, since this has already been merged in that other branch.

@lminiero
Copy link
Member Author

lminiero commented Jun 4, 2021

Done. I don't have time to test this, and to be honest it's very low priority for me. If you do care about this, please test and provide feedback. If I don't get any, I'll assume people don't care and just close it (no reason to merge and maintain it otherwise).

@fancycode
Copy link
Contributor

Thanks a lot! I'll test once I get to implement the API and will provide feedback in the other PR.

Out of curiosity: how do you configure servers to handle lots of streams? With dynamically created loops or a static event_loops configuration? There seems to be no "best practices" document (or at least I could not find one).

@cb22
Copy link
Contributor

cb22 commented Jun 9, 2021

@fancycode FWIW, the way we run in production is with some tooling around routing different users to different janus instances, than running multiple instances each with event_loops = 1.

The per process overhead is quite low (a few MB RAM), it essentially does the same thing as mentioned in the comment linked to above (I think! I'll admit I don't have hard data here) and has the added benefit of meaning a crash / issue affects fewer users than if everyone were on a single instance.

lminiero added a commit that referenced this pull request Jun 25, 2021
lminiero added a commit that referenced this pull request Sep 1, 2021
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.

3 participants