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

Limit threads per zenoh session #95

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Limit threads per zenoh session #95

merged 2 commits into from
Jan 22, 2024

Conversation

Yadunund
Copy link
Member

@Yadunund Yadunund commented Jan 20, 2024

Fix #94 by passing "--no-default-features;--features=zenoh/transport_tcp" to the cargo command invoked when building zenoh-c. The default session config is also updated to limit the threads to 1.

Special thanks to @cottsay for advising on how to deal with the ;s in cmake

cc: @Mallets

@Yadunund Yadunund requested a review from clalancette January 20, 2024 04:38
@Mallets
Copy link

Mallets commented Jan 22, 2024

The Zenoh configuration looks good to me.

For tracking purposing, I link here a forthcoming PR on Zenoh (eclipse-zenoh/zenoh#566) which will impact the configuration of the Zenoh runtime. Note the the PR will impact only the configuration file, not CMake.

Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides what I said inline, do we need a similar configuration for the router build/config file? I don't know the answer, so just asking here.

Comment on lines 188 to 190
// Number of threads dedicated to transmission
// By default, the number of threads is calculated as follows: 1 + ((#cores - 1) / 4)
threads: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the CMakeLists.txt, we have a nice comment explaining why we want to limit the threads. Can we have a similar comment here? It might also be a good idea to link to the PR that Luca mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've spoken to @Mallets and it's better to limit it there too although the router is only used to forward discovery data. Also added the note in the configs ec36e0f

@clalancette clalancette merged commit 06f29d0 into rolling Jan 22, 2024
5 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yadu/limit-threads branch January 22, 2024 14:24
@Yadunund Yadunund mentioned this pull request Jan 24, 2024
3 tasks
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.

Thread creation error when running many nodes that spawn a lot of threads
3 participants