-
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
Optimization of LTC signals #445
Conversation
…messages with the current tag
349fb9a
to
0fd91de
Compare
a5cf49c
to
442f78f
Compare
199bab6
to
fb99cc5
Compare
WalkthroughThe changes primarily focus on updating the nomenclature and logic for handling "latest tag complete (LTC)" messages throughout the federated system, replacing it with "latest tag confirmed (LTC)." Additionally, the updates introduce conditional logic specific to 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 Configuration 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: 1
Outside diff range and nitpick comments (1)
include/core/federated/federate.h (1)
153-158
: Clarify the documentation forlast_sent_LTC
.The comment explains the purpose of tracking the
last_sent_LTC
to avoid redundant messages. However, the explanation could be clearer about the conditions under which a duplicate LTC might be sent and why it's problematic. Consider revising the comment for clarity and completeness.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- core/environment.c (1 hunks)
- core/federated/RTI/rti_common.c (1 hunks)
- core/federated/RTI/rti_common.h (1 hunks)
- core/federated/RTI/rti_remote.c (2 hunks)
- core/federated/RTI/rti_remote.h (1 hunks)
- core/federated/federate.c (3 hunks)
- core/reactor_common.c (1 hunks)
- core/threaded/reactor_threaded.c (1 hunks)
- include/core/environment.h (1 hunks)
- include/core/federated/federate.h (3 hunks)
- include/core/federated/network/net_common.h (2 hunks)
- lingua-franca-ref.txt (1 hunks)
Files skipped from review due to trivial changes (2)
- include/core/federated/network/net_common.h
- lingua-franca-ref.txt
Additional comments not posted (9)
include/core/environment.h (1)
90-90
: Addition ofneed_to_send_LTC
field inenvironment_t
structure.The addition of the
need_to_send_LTC
field appears to be well-integrated with the existing structure. Ensure that all federated environments properly initialize this field to avoid undefined behavior.core/environment.c (1)
120-123
: Conditional initialization ofneed_to_send_LTC
.The conditional block added for initializing
need_to_send_LTC
underFEDERATED_CENTRALIZED
is clear and concise. However, ensure that this field is appropriately used in the logic of federate signal handling to fully leverage its purpose.core/federated/RTI/rti_remote.h (1)
220-227
: Renaming of LTC handling function.The renaming of
handle_latest_tag_complete
tohandle_latest_tag_confirmed
is consistent with the new terminology and the PR's objectives. Ensure that all references and calls to this function across the project are updated to reflect this change.core/federated/RTI/rti_common.c (1)
67-67
: Updated log message to reflect terminology change from "complete" to "confirmed".This change aligns with the PR's objectives of renaming LTC signals to reflect their intended use more accurately.
include/core/federated/federate.h (1)
Line range hint
294-303
: Optimize the handling of LTC messages inlf_latest_tag_confirmed
.This function now includes logic to prevent sending redundant LTC messages, which aligns with the PR's goal to reduce unnecessary network traffic. However, ensure that the mutex lock mentioned in the comment is appropriately handled in all calling contexts to avoid potential race conditions.
core/reactor_common.c (1)
213-215
: Optimization of LTC Signal SendingThe change to set
env->need_to_send_LTC
tofalse
at the beginning of the time step for federated centralized environments is a good optimization. It helps in reducing unnecessary LTC signals, aligning with the PR's objective. Ensure that this flag is properly managed elsewhere in the code to prevent any side effects.core/threaded/reactor_threaded.c (1)
883-889
: Optimization of LTC signaling approved.The added logic correctly sets the
need_to_send_LTC
flag when processing an input reaction, which aligns with the PR's goal to optimize LTC signals by only sending them when necessary. This should help in reducing unnecessary network traffic and optimize the federated system's performance.core/federated/RTI/rti_remote.c (2)
480-480
: Rename function to align with new LTC semanticsThe function
handle_latest_tag_confirmed
has been renamed fromhandle_latest_tag_complete
, aligning with the new semantics of LTC as "latest tag confirmed". This is consistent with the PR's goal to optimize the handling of these signals.
1113-1114
: Update message handling to reflect LTC changesThe message type for handling LTC has been updated to
MSG_TYPE_LATEST_TAG_CONFIRMED
. This change correctly reflects the updated semantics and ensures that the system handles the LTC messages under the new logic.
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/federated/federate.c (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/federated/federate.c
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/threaded/reactor_threaded.c (1 hunks)
Additional comments not posted (1)
core/threaded/reactor_threaded.c (1)
883-889
: Verify the usage of theneed_to_send_LTC
flag.The
need_to_send_LTC
flag is set totrue
if the current reaction is an input reaction. Ensure that this flag is used correctly elsewhere in the codebase to confirm that the federate has successfully received and processed tagged messages with the current tag.Verification successful
The usage of the
need_to_send_LTC
flag is verified and correct.The
need_to_send_LTC
flag is appropriately set and checked in the codebase to control the sending of the Latest Tag Confirmed (LTC) message. The logic ensures that the LTC is only sent when necessary, confirming that the federate has successfully received and processed tagged messages with the current tag.
core/environment.c
: Initializesneed_to_send_LTC
tofalse
.core/reactor_common.c
: Resetsneed_to_send_LTC
tofalse
underFEDERATED_CENTRALIZED
.include/core/environment.h
: Declares theneed_to_send_LTC
flag.core/threaded/reactor_threaded.c
: Setsneed_to_send_LTC
totrue
when the current reaction is an input reaction.core/federated/federate.c
: Checksneed_to_send_LTC
before deciding to send the LTC.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `need_to_send_LTC` flag in the codebase. # Test: Search for the usage of `need_to_send_LTC`. Expect: Correct handling and usage of the flag. rg --type-add 'c,h:*.{c,h}' --type c --pcre2 '\bneed_to_send_LTC\b'Length of output: 129
Script:
#!/bin/bash # Description: Verify the usage of the `need_to_send_LTC` flag in the codebase. # Correct the command to add custom file types and search for the usage of `need_to_send_LTC`. rg --type-add 'c:*.{c,h}' --type c '\bneed_to_send_LTC\b'Length of output: 347
Script:
#!/bin/bash # Description: Extract context around the usage of the `need_to_send_LTC` flag in relevant files. # Extract lines around the usage of `need_to_send_LTC` in each identified file. rg --type-add 'c:*.{c,h}' --type c -C 5 '\bneed_to_send_LTC\b' core/environment.c core/reactor_common.c include/core/environment.h core/threaded/reactor_threaded.c core/federated/federate.cLength of output: 3628
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.
LGTM! This seems to refer to an LTC-optimization
branch of lingua-franca, but I don't see a corresponding PR in lingua-franca (?).
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- core/federated/RTI/rti_remote.c (2 hunks)
- core/reactor_common.c (1 hunks)
- core/threaded/reactor_threaded.c (1 hunks)
Files not summarized due to errors (1)
- core/federated/RTI/rti_remote.c: Error: Server error. Please try again later.
Files skipped from review as they are similar to previous changes (1)
- core/threaded/reactor_threaded.c
Additional comments not posted (3)
core/reactor_common.c (1)
213-215
: LGTM! But verify the usage ofneed_to_send_LTC
.The code changes are approved. The conditional block correctly sets
env->need_to_send_LTC
tofalse
ifFEDERATED_CENTRALIZED
is defined.However, ensure that all usages of
need_to_send_LTC
are consistent with this change.Verification successful
The usage of
need_to_send_LTC
is consistent with the recent changes.The variable
need_to_send_LTC
is correctly set and used across the codebase, aligning with the new change incore/reactor_common.c
.
- Declaration:
include/core/environment.h
- Assignments:
core/environment.c
(withinenvironment_init_federated
function)core/reactor_common.c
(under#ifdef FEDERATED_CENTRALIZED
)core/threaded/reactor_threaded.c
(set totrue
within a conditional block)- Conditional check:
core/federated/federate.c
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `need_to_send_LTC` are consistent with the change. # Test: Search for the variable usage. Expect: Consistent usage across the codebase. rg --type-add 'c:*.c' --type-add 'h:*.h' --type c,h 'need_to_send_LTC'Length of output: 98
Script:
#!/bin/bash # Description: Verify all usages of `need_to_send_LTC` are consistent with the change. # Test: Search for the variable usage. Expect: Consistent usage across the codebase. rg --type-add 'c:*.c' --type c 'need_to_send_LTC' rg --type-add 'h:*.h' --type h 'need_to_send_LTC'Length of output: 439
Script:
#!/bin/bash # Description: Extract contexts of `need_to_send_LTC` for verification. # Extract the declaration echo "Declaration in include/core/environment.h:" rg --type h 'bool need_to_send_LTC;' include/core/environment.h # Extract assignments in core/environment.c echo "Assignments in core/environment.c:" rg --type c 'need_to_send_LTC = ' core/environment.c -A 3 -B 3 # Extract assignments in core/reactor_common.c echo "Assignments in core/reactor_common.c:" rg --type c 'need_to_send_LTC = ' core/reactor_common.c -A 3 -B 3 # Extract assignments in core/threaded/reactor_threaded.c echo "Assignments in core/threaded/reactor_threaded.c:" rg --type c 'need_to_send_LTC = ' core/threaded/reactor_threaded.c -A 3 -B 3 # Extract conditional check in core/federated/federate.c echo "Conditional check in core/federated/federate.c:" rg --type c 'if (!env->need_to_send_LTC)' core/federated/federate.c -A 3 -B 3Length of output: 1613
core/federated/RTI/rti_remote.c (2)
1113-1114
: LGTM!The new case for
MSG_TYPE_LATEST_TAG_CONFIRMED
is correctly added to the switch statement.
Line range hint
480-493
: LGTM! But verify the function usage in the codebase.The changes in the function name and log message are approved.
However, ensure that all function calls to
handle_latest_tag_confirmed
match the new signature.Verification successful
Function usage matches the new signature.
The function call to
handle_latest_tag_confirmed
matches the new signature, and there are no other instances of this function call in the codebase.
- core/federated/RTI/rti_remote.c:
handle_latest_tag_confirmed(my_fed);
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handle_latest_tag_confirmed` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'handle_latest_tag_confirmed'Length of output: 293
Script:
#!/bin/bash # Description: Verify the context of the function call to `handle_latest_tag_confirmed` matches the new signature. # Extract the lines around the function call to ensure it matches the new signature. rg --type c --context 5 'handle_latest_tag_confirmed' core/federated/RTI/rti_remote.cLength of output: 995
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: 1
Outside diff range and nitpick comments (3)
core/federated/RTI/rti_remote.c (3)
Line range hint
480-487
: Ensure proper error handling when reading from the socket.The function reads data from the socket without checking for errors. Consider handling potential errors to avoid undefined behavior.
- read_from_socket_fail_on_error(&fed->socket, sizeof(int64_t) + sizeof(uint32_t), buffer, NULL, - "RTI failed to read the content of the logical tag complete from federate %d.", - fed->enclave.id); + if (read_from_socket_fail_on_error(&fed->socket, sizeof(int64_t) + sizeof(uint32_t), buffer, NULL, + "RTI failed to read the content of the logical tag complete from federate %d.", + fed->enclave.id)) { + return; + }
Line range hint
484-484
: Clarify the log message.The log message refers to "logical tag complete" instead of "latest tag confirmed". Update the message for consistency.
- tracepoint_rti_from_federate(receive_LTC, fed->enclave.id, &completed); + tracepoint_rti_from_federate(receive_LTC, fed->enclave.id, &completed);
Line range hint
485-487
: Consider moving the mutex lock earlier.To ensure thread safety, acquire the mutex lock before calling
_logical_tag_complete
.- _logical_tag_complete(&(fed->enclave), completed); - // FIXME: Should this function be in the enclave version? LF_MUTEX_LOCK(&rti_mutex); + LF_MUTEX_LOCK(&rti_mutex); + _logical_tag_complete(&(fed->enclave), completed);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- core/federated/RTI/rti_remote.c (2 hunks)
- core/reactor_common.c (1 hunks)
- core/threaded/reactor_threaded.c (1 hunks)
Additional comments not posted (3)
core/reactor_common.c (1)
213-215
: LGTM! But verify the usage ofneed_to_send_LTC
.The code changes are approved.
However, ensure that all usages of
need_to_send_LTC
are correctly handled throughout the codebase.Verification successful
The usage of
need_to_send_LTC
is consistent and correctly handled throughout the codebase.
core/reactor_common.c
: The variable is set tofalse
under theFEDERATED_CENTRALIZED
condition.core/environment.c
: The variable is initialized tofalse
during the environment struct initialization under theFEDERATED_CENTRALIZED
condition.core/threaded/reactor_threaded.c
: The variable is set totrue
if a certain condition within an input reaction is met under theFEDERATED_CENTRALIZED
condition.core/federated/federate.c
: The variable is checked, and iffalse
, the function skips sending a Latest Tag Confirmed (LTC) message.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `need_to_send_LTC`. # Test: Search for the usage of `need_to_send_LTC`. Expect: Correct handling. rg --type c --type h $'need_to_send_LTC'Length of output: 327
Script:
#!/bin/bash # Extract and examine the relevant code snippets involving `need_to_send_LTC` from each file. # Extract from core/reactor_common.c echo "core/reactor_common.c:" rg --context 5 'need_to_send_LTC' core/reactor_common.c # Extract from core/environment.c echo -e "\ncore/environment.c:" rg --context 5 'need_to_send_LTC' core/environment.c # Extract from core/threaded/reactor_threaded.c echo -e "\ncore/threaded/reactor_threaded.c:" rg --context 5 'need_to_send_LTC' core/threaded/reactor_threaded.c # Extract from include/core/environment.h echo -e "\ninclude/core/environment.h:" rg --context 5 'need_to_send_LTC' include/core/environment.h # Extract from core/federated/federate.c echo -e "\ncore/federated/federate.c:" rg --context 5 'need_to_send_LTC' core/federated/federate.cLength of output: 2655
core/threaded/reactor_threaded.c (2)
885-885
: Confirm the necessity of theFEDERATED_CENTRALIZED
conditional block.Ensure that the
FEDERATED_CENTRALIZED
macro is defined and used correctly in the build configuration.Verification successful
The
FEDERATED_CENTRALIZED
macro is properly defined and used in the build configuration.
- Confirmed the definition in
core/CMakeLists.txt
.- Verified its usage across multiple relevant files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `FEDERATED_CENTRALIZED` macro in the build configuration. # Test: Search for the definition and usage of `FEDERATED_CENTRALIZED` macro. Expect: Definitions and usages. rg --type c --type h --word-regexp 'FEDERATED_CENTRALIZED'Length of output: 854
Script:
#!/bin/bash # Description: Verify the definition of `FEDERATED_CENTRALIZED` macro in the build configuration files. # Test: Search for the definition of `FEDERATED_CENTRALIZED` macro in common build configuration files such as Makefile, CMakeLists.txt, etc. rg --type make --type cmake --word-regexp 'FEDERATED_CENTRALIZED'Length of output: 115
886-891
: Ensure the correctness of theneed_to_send_LTC
flag logic.The added code sets the
need_to_send_LTC
flag totrue
if the current reaction is an input reaction. This ensures that the federate sends the LTC signal when it has received a tagged message with the current tag. Verify that this logic aligns with the intended behavior of the LTC optimization.Verification successful
The
need_to_send_LTC
flag logic is correct.The flag is initialized to
false
in centralized federated environments and set totrue
when a tagged message is received. The conditional check before sending the Latest Tag Confirmed (LTC) message ensures proper behavior.
core/reactor_common.c
, line 214core/environment.c
, line 121core/federated/federate.c
, line 2211Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `need_to_send_LTC` flag logic. # Test: Search for the usage of `need_to_send_LTC` in the codebase to ensure it aligns with the intended behavior. rg --type c --type h --word-regexp 'need_to_send_LTC'Length of output: 341
Script:
#!/bin/bash # Extract lines where `need_to_send_LTC` is set to `false` in `core/reactor_common.c` and `core/environment.c` rg --type c --type h --line-number 'need_to_send_LTC = false;' core/reactor_common.c core/environment.c # Extract the context (10 lines before and after) where `need_to_send_LTC` is checked in `core/federated/federate.c` rg --type c --type h --line-number -A 10 -B 10 'need_to_send_LTC' core/federated/federate.cLength of output: 1208
Script:
#!/bin/bash # Extract the context (10 lines before and after) where `need_to_send_LTC` is set to `false` in `core/environment.c` and `core/reactor_common.c` rg --type c --type h --line-number -A 10 -B 10 'need_to_send_LTC = false;' core/environment.c core/reactor_common.cLength of output: 2555
I noticed a segfault in the tests. Let's defer merging this until that has been sorted out... |
Co-authored-by: Edward A. Lee <[email protected]>
@edwardalee Thank you for reviewing this PR, professor! I'll add a companion LF PR right soon. Also, let me address a test failure, too. |
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/federated/federate.c (3 hunks)
Additional comments not posted (2)
core/federated/federate.c (2)
2209-2212
: Clarify log message.The log message can be made more concise and clear.
- LF_PRINT_LOG("Skip sending Latest Tag Confirmed (LTC) to the RTI because there was no tagged message with the " - "tag " PRINTF_TAG " that this federate has received.", - tag_to_send.time - start_time, tag_to_send.microstep); + LF_PRINT_LOG("Skipping LTC " PRINTF_TAG " to RTI: No tagged message received with this tag.", + tag_to_send.time - start_time, tag_to_send.microstep);
2202-2202
: Ensure consistent function naming.The function
lf_latest_tag_confirmed
should be consistently named across the codebase. Verify that all references tolf_latest_tag_complete
have been updated.Verification successful
Function naming verified.
All references to the function
lf_latest_tag_confirmed
have been updated consistently across the codebase. No occurrences of the old function namelf_latest_tag_complete
were found.
core/federated/federate.c
include/core/federated/federate.h
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `lf_latest_tag_confirmed` match the new name. # Test: Search for the function usage. Expect: Only occurrences of the new name. rg --type c $'lf_latest_tag_confirmed'Length of output: 352
Script:
#!/bin/bash # Description: Verify there are no outdated references to the old function name `lf_latest_tag_complete`. # Test: Search for the old function name. Expect: No occurrences of the old name. rg --type c $'lf_latest_tag_complete'Length of output: 36
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/reactor_common.c (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/reactor_common.c
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/federated/federate.c (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/federated/federate.c
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: 1
Outside diff range and nitpick comments (2)
core/federated/RTI/rti_remote.c (1)
Line range hint
480-488
:
Ensure proper error handling for the new message type.The function handles the new "latest tag confirmed" message type. Ensure that errors are appropriately handled.
- read_from_socket_fail_on_error(&fed->socket, sizeof(int64_t) + sizeof(uint32_t), buffer, NULL, - "RTI failed to read the content of the logical tag complete from federate %d.", - fed->enclave.id); + if (read_from_socket(&fed->socket, sizeof(int64_t) + sizeof(uint32_t), buffer)) { + lf_print_error("RTI failed to read the content of the logical tag confirmed from federate %d.", fed->enclave.id); + return; + }core/federated/federate.c (1)
2212-2215
: Improve log message readability.Consider rephrasing the log message for better readability.
- LF_PRINT_LOG("Skip sending Latest Tag Confirmed (LTC) to the RTI because there was no tagged message with the " - "tag " PRINTF_TAG " that this federate has received.", - tag_to_send.time - start_time, tag_to_send.microstep); + LF_PRINT_LOG("Skipping sending Latest Tag Confirmed (LTC) to the RTI because no tagged message with the " + "tag " PRINTF_TAG " has been received by this federate.", + tag_to_send.time - start_time, tag_to_send.microstep);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- core/federated/RTI/rti_remote.c (2 hunks)
- core/federated/federate.c (3 hunks)
- include/core/federated/network/net_common.h (2 hunks)
Additional comments not posted (2)
include/core/federated/network/net_common.h (1)
437-442
: LGTM! Ensure consistency across the codebase.The renaming of the message type from
MSG_TYPE_LATEST_TAG_COMPLETE
toMSG_TYPE_LATEST_TAG_CONFIRMED
is correct.However, verify that all references to the old message type in the codebase have been updated accordingly.
core/federated/federate.c (1)
2218-2220
: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
lf_latest_tag_confirmed
match the new signature.
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/federated/federate.c (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/federated/federate.c
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/threaded/reactor_threaded.c (1 hunks)
Additional comments not posted (1)
core/threaded/reactor_threaded.c (1)
884-891
: LGTM! Ensure correctness of the conditional block.The new conditional block correctly checks if
current_reaction_to_execute
is an input reaction and sets theneed_to_send_LTC
flag accordingly. This change aligns with the PR's objective to optimize LTC signals.
@byeonggiljun, it looks like this got approved, but there are still a few unresolved conversations, which will prevent this for getting merged. Could you please check on them? |
@lhstrh, I resolved every conversation except for the one regarding |
Optimization of LTC signals
We have a companion PR in lf-lang/lingua-franca#2346.
Currently, a federate sends the latest tag completed (LTC) signals every time the federate completes a tag. However, this is not necessary as the primary goal of LTC signals is to confirm that the federate has received and processed tagged messages with the current tag (and tags earlier than the current tag). Thus, I suggest renaming the signal's name to the latest tag confirmed (the abbreviation is still LTC) and making federates send the LTC signal only if the federates have received any tagged message with the current tag.
The other role of LTC signals is to allow its "immediate" downstream federates to advance their tags. The only downside of this change is that the RTI may take more time to compute TAG signals. For example, let's consider an example as described below.
In the current implementation, federate B sends
LTC (100 msec)
when B completes100 msec
. Then, the RTI can grantTAG (100 msec)
withLTC (100 msec)
. However, if B does not sendLTC (100 msec)
(because A does not send any tagged messages with100 msec
), the RTI has to look up A as well as B to calculate C'sEIMT
to grantTAG (100 msec)
. Yet, I argue that this impact is negligible because we almost never see a million federates connected together.Summary by CodeRabbit
Refactor
New Features
Chores