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

fix: fix z_sub_thr termination and improve perf #1609

Closed

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Nov 25, 2024

Using Thread::unpark std::process::exit allows session to be closed, and callback final print to be executed.
Callback lock accounted for 2.5% of z_sub_thr perf. Using atomic and fine-grained lock remove the performance impact (even if it doesn't really change the global outcome).

Using `Thread::unpark` `std::process::exit` allows session to be
closed, and callback final print to be executed.
Callback lock accounted for 2.5% of z_sub_thr perf. Using atomic and
fine-grained lock remove the performance impact (even if it doesn't
really change the global outcome).
@wyfo wyfo requested review from Mallets and fuzzypixelz November 25, 2024 09:44
@wyfo wyfo added the internal Changes not included in the changelog label Nov 25, 2024
@eclipse-zenoh eclipse-zenoh deleted a comment from github-actions bot Nov 26, 2024
@Mallets
Copy link
Member

Mallets commented Nov 26, 2024

Any change on the example should be aligned with all Zenoh bindings.
All examples should provide the same way of providing measures.
So, should all the throughput examples be updated?

@Mallets Mallets added the invalid This doesn't seem right label Dec 5, 2024
@Mallets
Copy link
Member

Mallets commented Dec 5, 2024

Just to reiterate on the point. Any change in examples needs to be coordinated across all bindings.
This is particular important in the case of throughput examples that are made in such a way the measurement is performed in the same way in all Zenoh APIs.

I'm marking this PR as invalid for the time being till further actions are taken. Either closing the PR or providing updates to all other bindings.

@wyfo
Copy link
Contributor Author

wyfo commented Jan 16, 2025

The throughput examples are not really aligned across bindings.
Python is the only one working as expected, i.e. printing a recap at the end. The other ones declare a drop callback which is not executed, their behavior is at least misleading.
Also Rust is the only one to use a mutex (with callback_mut). C and C++ example doesn't do that (Python don't need obviously), so their is also an example alignment issue there, as well as a more serious issue of presenting example with "buggy code" of unsynchronized callback (which can be called multiple times concurrently).

That's why I think that both C and C++ example should be fixed and aligned on this more correct new Rust version.

@Mallets
Copy link
Member

Mallets commented Jan 16, 2025

Then we need a review of the examples. Again to reiterate on the point, any change in examples needs to be coordinated across all bindings. Modifying this example right now would also bring a difference w.r.t. historical measurements.

@wyfo
Copy link
Contributor Author

wyfo commented Jan 17, 2025

Modifying this example right now would also bring a difference w.r.t. historical measurements.

Ok, so it's better to close this PR anyway.

@wyfo wyfo closed this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes not included in the changelog invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants