-
Notifications
You must be signed in to change notification settings - Fork 24
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
Immediate start of federates with STA offset under decentralized coordination #469
Conversation
WalkthroughThe recent changes in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
core/threaded/reactor_threaded.c (1)
Line range hint
209-245
: Handling of STA offset is robust, but could use clearer documentation.The function
wait_until
manages the STA offset well by adjusting the logical time to include it, which is crucial for correct synchronization in federated decentralized systems. However, the comments could be enhanced to clarify the role and impact of the STA offset in preventing overflows and ensuring timely starts.// Apply the STA to the logical time // Prevent an overflow - // If wait_time is not forever + // Adjust wait time to include STA offset, ensuring it does not exceed FOREVER
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/threaded/reactor_threaded.c (2 hunks)
Additional comments not posted (2)
core/threaded/reactor_threaded.c (2)
Line range hint
30-58
: Well-implemented tag barrier logic.The function
_lf_increment_tag_barrier_locked
correctly manages the tag barriers, ensuring logical time synchronization in federated environments. The checks and adjustments forfuture_tag
are appropriately handled.
Line range hint
97-124
: Effective management of theFOREVER
tag in barriers.The function
_lf_wait_on_tag_barrier
properly handles cases whereproposed_tag.time == FOREVER
, preventing unnecessary waits and aligning with the PR's objective of immediate thread start-up in federated environments whenSTP_offset
is set toFOREVER
.
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.
Why was the waiting done so far and why is it safe to simply remove it?
An STA_offset of T (which should be called STA, safe to advance) means that at physical time t+T the federate can assume it has received all messages with tags less than t. If t is the start time, the federate already knows it has received all messages with tags less than t (because there are no messages with tags less than t). Hence, at the start time, there is no need to wait. This change enables very simple use of decentralized coordination for a subset of LF programs that do not deal with time, but rather just deal with data flow. |
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 looks right to me.
Allow dataflow-like behavior when setting STP_offset to infinity
I adjusted the title of the PR to match what I think this does, but I'm not sure I got it right... |
@edwardalee please also update the description (it doesn't seem like this PR enables data-flow-like behavior as it only affects the behavior at startup?) |
In decentralized coordination, the behavior used to be to wait for
STP_offset
before starting the worker threads. With this change, the federates start immediately without waiting forSTP_offset
.When the user sets to
STP_offset
toFOREVER
, it enables dataflow-like behavior.Summary by CodeRabbit