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

Optimization of LTC signals #445

Merged
merged 17 commits into from
Aug 1, 2024
Merged

Optimization of LTC signals #445

merged 17 commits into from
Aug 1, 2024

Conversation

byeonggiljun
Copy link
Collaborator

@byeonggiljun byeonggiljun commented Jun 17, 2024

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.

image

In the current implementation, federate B sends LTC (100 msec) when B completes 100 msec. Then, the RTI can grant TAG (100 msec) with LTC (100 msec). However, if B does not send LTC (100 msec) (because A does not send any tagged messages with 100 msec), the RTI has to look up A as well as B to calculate C's EIMT to grant TAG (100 msec). Yet, I argue that this impact is negligible because we almost never see a million federates connected together.

Summary by CodeRabbit

  • Refactor

    • Renamed functions and updated log messages from "latest tag complete" (LTC) to "latest tag confirmed" (LTC) to clarify event meaning.
  • New Features

    • Introduced a conditional logic to handle federated centralized mode, setting a flag for message processing confirmation.
  • Chores

    • Updated comments and variable names for better clarity and consistency in the code.
    • Changed branch name reference from "master" to "LTC-optimization."

@byeonggiljun byeonggiljun added federated enhancement Enhancement of existing feature labels Jun 17, 2024
Copy link
Contributor

coderabbitai bot commented Jun 21, 2024

Walkthrough

The 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 FEDERATED_CENTRALIZED mode, impacting flag settings within the environment structure. No significant alterations to function signatures or core functionality were made, except for name changes and minor adjustments in logic flow.

Changes

File(s) Change Summary
core/environment.c, core/reactor_common.c, core/threaded/reactor_threaded.c Introduced conditional logic for setting need_to_send_LTC based on FEDERATED_CENTRALIZED.
core/federated/RTI/rti_common.c, core/federated/RTI/rti_common.h, core/federated/RTI/rti_remote.c, core/federated/RTI/rti_remote.h Updated nomenclature from "latest tag complete" to "latest tag confirmed" in log messages, function names, and comments.
core/federated/federate.c, include/core/federated/federate.h Renamed function lf_latest_tag_complete to lf_latest_tag_confirmed, updated log messages, and made minor variable name changes.
include/core/environment.h Added bool need_to_send_LTC to the environment_t struct under the FEDERATED condition.
include/core/federated/network/net_common.h Renamed message type MSG_TYPE_LATEST_TAG_COMPLETE to MSG_TYPE_LATEST_TAG_CONFIRMED.
lingua-franca-ref.txt Transitioned from a branch named "master" to "LTC-optimization."

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@byeonggiljun byeonggiljun marked this pull request as ready for review June 21, 2024 01:51
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 for last_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

Commits

Files that changed from the base of the PR and between 587a8f9 and b614e44.

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 of need_to_send_LTC field in environment_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 of need_to_send_LTC.

The conditional block added for initializing need_to_send_LTC under FEDERATED_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 to handle_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 in lf_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 Sending

The change to set env->need_to_send_LTC to false 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 semantics

The function handle_latest_tag_confirmed has been renamed from handle_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 changes

The 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.

core/federated/federate.c Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between b614e44 and ea72401.

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between ea72401 and 20641bf.

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 the need_to_send_LTC flag.

The need_to_send_LTC flag is set to true 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: Initializes need_to_send_LTC to false.
  • core/reactor_common.c: Resets need_to_send_LTC to false under FEDERATED_CENTRALIZED.
  • include/core/environment.h: Declares the need_to_send_LTC flag.
  • core/threaded/reactor_threaded.c: Sets need_to_send_LTC to true when the current reaction is an input reaction.
  • core/federated/federate.c: Checks need_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.c

Length of output: 3628

@lhstrh lhstrh requested a review from edwardalee July 1, 2024 08:32
Copy link
Contributor

@edwardalee edwardalee left a 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 (?).

core/federated/federate.c Outdated Show resolved Hide resolved
lingua-franca-ref.txt Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 20641bf and 9900291.

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 of need_to_send_LTC.

The code changes are approved. The conditional block correctly sets env->need_to_send_LTC to false if FEDERATED_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 in core/reactor_common.c.

  • Declaration: include/core/environment.h
  • Assignments:
    • core/environment.c (within environment_init_federated function)
    • core/reactor_common.c (under #ifdef FEDERATED_CENTRALIZED)
    • core/threaded/reactor_threaded.c (set to true 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 3

Length 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.c

Length of output: 995

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 20641bf and 9900291.

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 of need_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 to false under the FEDERATED_CENTRALIZED condition.
  • core/environment.c: The variable is initialized to false during the environment struct initialization under the FEDERATED_CENTRALIZED condition.
  • core/threaded/reactor_threaded.c: The variable is set to true if a certain condition within an input reaction is met under the FEDERATED_CENTRALIZED condition.
  • core/federated/federate.c: The variable is checked, and if false, 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.c

Length of output: 2655

core/threaded/reactor_threaded.c (2)

885-885: Confirm the necessity of the FEDERATED_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 the need_to_send_LTC flag logic.

The added code sets the need_to_send_LTC flag to true 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 to true 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 214
  • core/environment.c, line 121
  • core/federated/federate.c, line 2211
Scripts 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.c

Length 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.c

Length of output: 2555

core/federated/RTI/rti_remote.c Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Jul 1, 2024

I noticed a segfault in the tests. Let's defer merging this until that has been sorted out...

@byeonggiljun
Copy link
Collaborator Author

@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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 9900291 and 2982f7f.

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 to lf_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 name lf_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

core/federated/federate.c Outdated Show resolved Hide resolved
core/federated/federate.c Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 2982f7f and eb7d2ae.

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>
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between eb7d2ae and d8232ee.

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between d8232ee and 28e8580.

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 to MSG_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.

core/federated/federate.c Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 28e8580 and 01fda32.

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 01fda32 and 56ea78b.

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 the need_to_send_LTC flag accordingly. This change aligns with the PR's objective to optimize LTC signals.

@lhstrh
Copy link
Member

lhstrh commented Jul 25, 2024

@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?

@byeonggiljun
Copy link
Collaborator Author

@lhstrh, I resolved every conversation except for the one regarding lingua-franca-ref.txt which should be resolved after the PR lingua-franca#2346 being merged.

@lhstrh lhstrh enabled auto-merge July 31, 2024 22:40
@lhstrh lhstrh added this pull request to the merge queue Jul 31, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 31, 2024
@byeonggiljun byeonggiljun added this pull request to the merge queue Aug 1, 2024
Merged via the queue into main with commit 543b738 Aug 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature federated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants