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 ZRuntime #893

Merged

Conversation

YuanYuYuan
Copy link
Contributor

@YuanYuYuan YuanYuYuan commented Apr 2, 2024

Closes #892.

  • Document the configuration.
  • Extend the configuration of runtime like controlling the max_blocking_threads.
  • Runtime handover.
  • Optimize the minimal threads for ZRuntime(currently 5 x 2 = 10). UPDATE: now it's one for each. Only the default of RX is two.
  • Add std::fmt::Display for ZRuntime . Replaced by serde and rename.
  • Formalize the configuration via an environmental variable with ron. For example, we can configure it like ZENOH_RUNTIME = "(app: (worker_threads: 4), net: (worker_threads: 3, handover: app))".
  • Auto static variables clean up

@YuanYuYuan YuanYuYuan marked this pull request as draft April 3, 2024 07:08
@YuanYuYuan YuanYuYuan force-pushed the feat/configure-zenoh-runtime branch from fb9bfe4 to 8bc2ca1 Compare April 8, 2024 05:59
@YuanYuYuan YuanYuYuan force-pushed the feat/configure-zenoh-runtime branch from f41279b to dae8c64 Compare April 9, 2024 14:59
@YuanYuYuan
Copy link
Contributor Author

Previously, we introduce a ZRuntimePoolGuard to address the memory leak issue reported by valgrind.
But this doesn't cover the examples since we don't want to include them in our examples. Furthermore, users may abuse it like

#[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 libc::atexit. See this commit for the details.

@Mallets Mallets changed the base branch from main_old to main April 11, 2024 08:47
@YuanYuYuan YuanYuYuan force-pushed the feat/configure-zenoh-runtime branch from c8c1d17 to 74631f9 Compare April 12, 2024 03:30
@YuanYuYuan
Copy link
Contributor Author

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

ZENOH_RUNTIME:

(
    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),
)

@YuanYuYuan
Copy link
Contributor Author

Configuration documentation is added.
image

@YuanYuYuan YuanYuYuan marked this pull request as ready for review April 15, 2024 08:36
libc = { workspace = true }
ron = { workspace = true }
serde = { workspace = true }
zenoh_runtime_derive = { path = "zenoh-runtime-derive" }
Copy link
Member

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.

Copy link
Contributor Author

@YuanYuYuan YuanYuYuan Apr 16, 2024

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

Copy link
Member

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 = "..."?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@YuanYuYuan YuanYuYuan Apr 17, 2024

Choose a reason for hiding this comment

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

Completed via db78a5a.

@YuanYuYuan
Copy link
Contributor Author

YuanYuYuan commented Apr 17, 2024

After introducing the new maco14 runner, the default worker threads to pass the CI seem to change. Need to do more investigation.

@YuanYuYuan
Copy link
Contributor Author

YuanYuYuan commented Apr 18, 2024

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 │
╰─────┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───┴───╯

@YuanYuYuan
Copy link
Contributor Author

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)
)

@Mallets Mallets merged commit 0283aaa into eclipse-zenoh:main Apr 19, 2024
10 checks passed
@YuanYuYuan YuanYuYuan deleted the feat/configure-zenoh-runtime branch January 9, 2025 07:19
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.

Improve ZRuntime
2 participants