-
Notifications
You must be signed in to change notification settings - Fork 178
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 ZRuntime
#893
Improve ZRuntime
#893
Conversation
fb9bfe4
to
8bc2ca1
Compare
f41279b
to
dae8c64
Compare
Previously, we introduce a #[tokio::main]
async fn main() {
let _z = zenoh_runtime::ZRuntimePoolGuard;
drop(_z); // Or something similar ways causing drop
let pub_session = zenoh::open(Config::default()).res().await.unwrap(); // panic!
...
} Instead, we can register a cleanup callback with |
c8c1d17
to
74631f9
Compare
The minimal number of worker threads passing the CI test is 3 with additional 2 blocking threads for rare blocking events. https://github.com/eclipse-zenoh/zenoh/actions/runs/8658067718/job/23741242526
(
rx: (handover: app),
acc: (handover: app),
net: (handover: app),
app: (worker_threads: 2, max_blocking_threads: 1),
tx: (worker_threads: 1, max_blocking_threads: 1),
) |
commons/zenoh-runtime/Cargo.toml
Outdated
libc = { workspace = true } | ||
ron = { workspace = true } | ||
serde = { workspace = true } | ||
zenoh_runtime_derive = { path = "zenoh-runtime-derive" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the path
directive work once published on crates.io
?
If we would need to publish the zenoh-runtime-derive
crate, then the CI should be updated accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that's needed. I prefer not to include them in the existing zenoh-macro to avoid polluting the library. What do you think? @Mallets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand what's needed. Is publishing to crates.io
needed or path = "..."
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to publish to crates.io.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would suggest to use zenoh-macros
if possible. Adding a new crate only for this would require also to update the release CI. But first the crate should be reserved to crates.io
and ownership shared with Eclipse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Let me handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed via db78a5a.
After introducing the new maco14 runner, the default worker threads to pass the CI seem to change. Need to do more investigation. |
According to the action logs, all of these combinations are troublesome, which means rx must be greater or equal to 2. ╭─────┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───┬───╮
│ tx │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │ 2 │
│ rx │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │ 1 │
│ app │ 1 │ 1 │ 1 │ 2 │ 2 │ 2 │ 2 │ 1 │ 1 │ 1 │ 1 │ 2 │ 2 │ 2 │ 2 │
│ acc │ 1 │ 2 │ 2 │ 1 │ 1 │ 2 │ 2 │ 1 │ 1 │ 2 │ 2 │ 1 │ 1 │ 2 │ 2 │
│ net │ 1 │ 1 │ 2 │ 1 │ 2 │ 1 │ 2 │ 1 │ 2 │ 1 │ 2 │ 1 │ 2 │ 1 │ 2 │
╰─────┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───╯ |
Combining the obervations of #893 (comment) and 036364f, the minimal worker threads to pass the CI are 5 + 3 ZENOH_RUNTIME (
rx: (worker_threads: 2, max_blocking_threads: 1),
acc: (handover: app),
net: (handover: app),
app: (worker_threads: 2, max_blocking_threads: 1),
tx: (worker_threads: 1, max_blocking_threads: 1)
) |
Closes #892.
max_blocking_threads
.ZRuntime
(currently 5 x 2 = 10).UPDATE: now it's one for each.Only the default of RX is two.AddReplaced by serde and rename.std::fmt::Display
forZRuntime
.ZENOH_RUNTIME = "(app: (worker_threads: 4), net: (worker_threads: 3, handover: app))"
.