-
Notifications
You must be signed in to change notification settings - Fork 102
[#1202] Add concurrency primitives crate to building blocks #1204
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
base: main
Are you sure you want to change the base?
[#1202] Add concurrency primitives crate to building blocks #1204
Conversation
|
|
||
| macro_rules! zero_copy_send_atomic { | ||
| ($type_name:ident, $base_type:ident) => { | ||
| paste::paste! { |
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.
This approach adds a new (compile-time) dependency paste to simplify the code for wrapping atomics in iceoryx2-pal-concurrency-sync in a new type so that iceoryx2-bb traits may be defined on it. If this new dependency is an issue, could potentially manually import all types as aliases for use in the macro instead...
44cab38 to
b2e7410
Compare
b2e7410 to
82b0377
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
+ Coverage 78.39% 78.46% +0.07%
==========================================
Files 411 413 +2
Lines 41008 41049 +41
Branches 1095 1095
==========================================
+ Hits 32149 32210 +61
+ Misses 8026 8006 -20
Partials 833 833
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
82b0377 to
88a0937
Compare
| use iceoryx2_bb_elementary_traits::placement_default::PlacementDefault; | ||
| use iceoryx2_bb_elementary_traits::zero_copy_send::ZeroCopySend; | ||
|
|
||
| macro_rules! zero_copy_send_atomic { |
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 think this approach is a bit cumbersome and introduces a dependency which we actually do not need. What do you think about the following approach:
iceoryx2-bb-elementarydepends directly oniceoryx2-pal-concurrency-syncas the only crate. In there we implementPlacementDefaultandZeroCopySendas it was before.iceoryx2-bb-concurrencyre-exports theiceoryx2-pal-concurrency-syncAtomic**'s with the new namespaceiceoryx2-bb-concurrency
With this we get rid of the paste dependency and do it like the Rust lib does it with core and the std re-exports. Also I do not see the benefit of removing the iceoryx2-pal-concurrency-sync dependency in iceoryx2-bb-elementary .
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.
Do you mean iceoryx2-bb-elementary-traits instead of iceoryx2-bb-elementary throughout this comment?
I think this approach is a bit cumbersome
If you mean the use of the "new type" wrapper, I can see how this could be undesirable if we need the atomics in iceoryx2-pal and iceoryx2-bb to be recognized as the same type in some instances, but do we have this use-case if we change the upper-layer crates use the iceoryx2-bb version everywhere?
Do we have the same requirement as std and core, where some use crates would like to seamlessly switch between depending on iceoryx2-bb and iceoryx2-pal?
get rid of the paste dependency
If paste is a problem (even though it is compile-time only), it can be removed by just writing some more boilerplate code by hand.
I do not see the benefit of removing the iceoryx2-pal-concurrency-sync dependency in iceoryx2-bb-elementary .
The motivation was to have a less tangled dependency tree. I was aiming to only have one crate in iceoryx2-bb depend on iceoryx2-pal-concurrency-sync.
elfenpiff
left a comment
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.
When possible, please use rather this approach: https://github.com/eclipse-iceoryx/iceoryx2/pull/1204/files#r2573007733
iceoryx2-bb/concurrency/Cargo.toml
Outdated
| paste = { workspace = true } | ||
|
|
||
| [dev-dependencies] | ||
| iceoryx2-bb-posix = { workspace = true } |
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.
This is a cyclic dependency and cannot be published to crates.io - even when it is a dev-dependency!
Since iceoryx2-bb-posix requires atomics, you must remove this dependency or iceoryx2-bb-posix dependends again on the iceoryx2-pal-concurrency-sync crate.
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.
As far as I can see only the spin_tests.rs depends on posix, see use iceoryx2_bb_posix::system_configuration::SystemInfo; It is used to acquire the number of cpu cores to parallelize the tests as much as the hardware allows.
But I think for something simple like a spinlock tests it is maybe a bit overkill and we can hardcode this to always 2 so that the tests use exactly two threads.
With this the dev-dependency to iceoryx2-bb-posix would be removed.
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.
Fixed
…sync in upper layers
…rom iceoryx2-bb-elementary-traits
8ce0787 to
7cbc40e
Compare
Notes for Reviewer
iceoryx2-bb-concurrencycrateiceoryx2-palIoxprefixiceoryx2-bb-concurrencyinstead oficeoryx2-pal-concurrency-syncSpinLocktoiceoryx2-bb-concurrencyand uses it instead of thespincrateA number of crates in upper layers of the architecture were depending directly on
iceoryx2-palfor concurrency primitives, which is not congruent with the defined architecture.A new crate is added to
iceoryx2-bbto re-export required concurrency primitives which are implemented iniceoryx2-pal. Only the primitives required by upper layers are currently re-exported, more can be added as needed.Some additional concurrency primitives useful to upper layers of
iceoryx2but not required iniceoryx2-palare also included in this crate.Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MITiox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Closes #1202