Skip to content

Remove dest_channel_id from NetworkChannel #117

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

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

LasseRosenow
Copy link
Collaborator

I think it is more reasonable to not duplicate this value if we anyways for example have the ports or file descriptors stored in the concrete channel implementation structs.

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Memory usage after merging this PR will be:

Memory Report

action_microstep_test_c

from to increase (%)
text 58964 58961 -0.01
data 752 752 0.00
bss 480 480 0.00
total 60196 60193 -0.00

action_overwrite_test_c

from to increase (%)
text 58812 58809 -0.01
data 744 744 0.00
bss 480 480 0.00
total 60036 60033 -0.00

action_test_c

from to increase (%)
text 58731 58728 -0.01
data 752 752 0.00
bss 480 480 0.00
total 59963 59960 -0.01

delayed_conn_test_c

from to increase (%)
text 58587 58584 -0.01
data 744 744 0.00
bss 480 480 0.00
total 59811 59808 -0.01

empty_action_test_c

from to increase (%)
text 58147 58144 -0.01
data 752 752 0.00
bss 480 480 0.00
total 59379 59376 -0.01

event_payload_pool_test_c

from to increase (%)
text 18330 18330 0.00
data 624 624 0.00
bss 320 320 0.00
total 19274 19274 0.00

event_queue_test_c

from to increase (%)
text 27425 27425 0.00
data 728 728 0.00
bss 480 480 0.00
total 28633 28633 0.00

multiple_startup_shutdown_test_c

from to increase (%)
text 57226 57223 -0.01
data 744 744 0.00
bss 11360 11360 0.00
total 69330 69327 -0.00

nanopb_test_c

from to increase (%)
text 42888 42888 0.00
data 904 904 0.00
bss 320 320 0.00
total 44112 44112 0.00

physical_action_test_c

from to increase (%)
text 60055 60052 -0.00
data 769 769 0.00
bss 10240 10240 0.00
total 71064 71061 -0.00

port_test_c

from to increase (%)
text 58462 58459 -0.01
data 744 744 0.00
bss 480 480 0.00
total 59686 59683 -0.01

reaction_queue_test_c

from to increase (%)
text 27137 27137 0.00
data 728 728 0.00
bss 480 480 0.00
total 28345 28345 0.00

request_shutdown_test_c

from to increase (%)
text 59686 59683 -0.01
data 744 744 0.00
bss 480 480 0.00
total 60910 60907 -0.00

shutdown_test_c

from to increase (%)
text 55769 55766 -0.01
data 752 752 0.00
bss 10912 10912 0.00
total 67433 67430 -0.00

startup_test_c

from to increase (%)
text 55100 55097 -0.01
data 752 752 0.00
bss 10688 10688 0.00
total 66540 66537 -0.00

tcp_channel_test_c

from to increase (%)
text 59981 60104 0.21
data 1176 1176 0.00
bss 11136 11136 0.00
total 72293 72416 0.17

timer_test_c

from to increase (%)
text 55001 54998 -0.01
data 744 744 0.00
bss 10720 10720 0.00
total 66465 66462 -0.00

@@ -486,6 +496,8 @@ void TcpIpChannel_ctor(TcpIpChannel *self, const char *host, unsigned short port
self->fd = 0;
self->state = NETWORK_CHANNEL_STATE_UNINITIALIZED;

self->super.get_dest_channel_id = TcpIpChannel_get_dest_channel_id;
self->super.get_connection_state = TcpIpChannel_get_connection_state;
Copy link
Collaborator Author

@LasseRosenow LasseRosenow Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the connection_state mapping was missing so I secretly added it here also 😈 hehe

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops! Thanks!

} else {
return self->fd;
}
}
Copy link
Collaborator Author

@LasseRosenow LasseRosenow Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be surprising because we were always talking about identifying TcpIpChannels using ports.
The issue here is that from the server perspective that is really easy. We have a port that we listen to per channel to each other federate. So we can just use our local listening port to identify messages between federates.

From the client side it is different. When we make a connection the operating system assigns a random port that is managed by the OS and could be retrieved using a POSIX API call by passing t he file descriptor.

File descriptors though do already exist for client and server side connections and are unique.
So using them seems an easy enough solution for now.

Please give feedback if you think that makes sense or not :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on how get_dest_channel_id is to be used by the runtime. In fact, I think it might not be used by the runtime at all, only by the lower levels of the NetworkChannel implementation. This suggests that it might not be the right decision to have this function as part of the generic NetworkChannel API, rather it some bookkeeping that must be done in the different concrete implementations?

Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but curios if we should have channel_id as part of the NetworkChannel abstraction at all...

@@ -486,6 +496,8 @@ void TcpIpChannel_ctor(TcpIpChannel *self, const char *host, unsigned short port
self->fd = 0;
self->state = NETWORK_CHANNEL_STATE_UNINITIALIZED;

self->super.get_dest_channel_id = TcpIpChannel_get_dest_channel_id;
self->super.get_connection_state = TcpIpChannel_get_connection_state;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops! Thanks!

@LasseRosenow LasseRosenow force-pushed the change-network-channel-dest-channel-id branch from 250a9f8 to 9921044 Compare November 11, 2024 17:34
Copy link
Contributor

Memory usage after merging this PR will be:

Memory Report

action_microstep_test_c

from to increase (%)
text 58934 58931 -0.01
data 752 752 0.00
bss 480 480 0.00
total 60166 60163 -0.00

action_overwrite_test_c

from to increase (%)
text 58782 58779 -0.01
data 744 744 0.00
bss 480 480 0.00
total 60006 60003 -0.00

action_test_c

from to increase (%)
text 58701 58698 -0.01
data 752 752 0.00
bss 480 480 0.00
total 59933 59930 -0.01

delayed_conn_test_c

from to increase (%)
text 58557 58554 -0.01
data 744 744 0.00
bss 480 480 0.00
total 59781 59778 -0.01

empty_action_test_c

from to increase (%)
text 58117 58114 -0.01
data 752 752 0.00
bss 480 480 0.00
total 59349 59346 -0.01

event_payload_pool_test_c

from to increase (%)
text 18330 18330 0.00
data 624 624 0.00
bss 320 320 0.00
total 19274 19274 0.00

event_queue_test_c

from to increase (%)
text 27425 27425 0.00
data 728 728 0.00
bss 480 480 0.00
total 28633 28633 0.00

multiple_startup_shutdown_test_c

from to increase (%)
text 57196 57193 -0.01
data 744 744 0.00
bss 11360 11360 0.00
total 69300 69297 -0.00

nanopb_test_c

from to increase (%)
text 42888 42888 0.00
data 904 904 0.00
bss 320 320 0.00
total 44112 44112 0.00

physical_action_test_c

from to increase (%)
text 60025 60022 -0.00
data 769 769 0.00
bss 10240 10240 0.00
total 71034 71031 -0.00

port_test_c

from to increase (%)
text 58432 58429 -0.01
data 744 744 0.00
bss 480 480 0.00
total 59656 59653 -0.01

reaction_queue_test_c

from to increase (%)
text 27137 27137 0.00
data 728 728 0.00
bss 480 480 0.00
total 28345 28345 0.00

request_shutdown_test_c

from to increase (%)
text 59656 59653 -0.01
data 744 744 0.00
bss 480 480 0.00
total 60880 60877 -0.00

shutdown_test_c

from to increase (%)
text 55739 55736 -0.01
data 752 752 0.00
bss 10912 10912 0.00
total 67403 67400 -0.00

startup_test_c

from to increase (%)
text 55070 55067 -0.01
data 752 752 0.00
bss 10688 10688 0.00
total 66510 66507 -0.00

tcp_channel_test_c

from to increase (%)
text 60007 60005 -0.00
data 1176 1176 0.00
bss 11136 11136 0.00
total 72319 72317 -0.00

timer_test_c

from to increase (%)
text 54971 54968 -0.01
data 744 744 0.00
bss 10720 10720 0.00
total 66435 66432 -0.00

Copy link
Contributor

Coverage after merging change-network-channel-dest-channel-id into main will be

68.85%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   action.c90.20%84.62%100%91.67%24, 43–46, 49, 51, 53, 63–64
   builtin_triggers.c90.24%70%100%96.43%14, 18, 37, 40
   connection.c80.69%53.85%100%89.69%10, 101, 107, 11, 120–121, 133–134, 14, 14, 140, 142, 17–18, 18, 18–19, 21, 23–24, 29, 44, 47, 52, 57–59, 94
   environment.c79.35%68.75%76.92%82.54%10–12, 12, 12–13, 15, 18–20, 41, 48–49, 89–91
   event.c95.35%92.86%100%96.15%14–15
   federated.c0%0%0%0%10, 101–104, 106, 106, 106–109, 111, 113–114, 114, 114, 114–118, 12, 120–121, 125–126, 126, 126–127, 129–130, 132, 136–137, 139, 14, 14, 14, 140–141, 144, 146–149, 15, 150–151, 153–155, 158–159, 16, 160, 160, 160–161, 161, 161–163, 165, 168–169, 17, 171–175, 177–179, 18, 180–182, 184, 184, 184–187, 189, 189, 189, 19, 19, 19, 190–191, 191, 191–192, 196–197, 197, 197, 20, 200–201, 205–207, 209, 209, 209, 211–215, 218, 218, 218–221, 224–225, 225, 225–226, 228–229, 23, 232–233, 238–239, 239, 239, 24, 240, 242, 244, 244, 244–247, 247, 247, 247, 247–249, 25, 25, 25, 250–259, 26, 263, 266, 266, 266–268, 27, 27, 27, 272, 275–276, 276, 276, 276–279, 28, 280–284, 286, 288–289, 29, 290, 290, 290–291, 293, 293, 293–295, 297–299, 30, 30, 30, 301, 303, 307–309, 31, 310–317, 319, 32, 32, 32, 32, 320, 323–326, 328, 328, 328–329, 33, 333–334, 334, 334, 336, 338–339, 339, 339, 34, 340, 340, 340–341, 341, 341–342, 342, 342–343, 343, 343, 345, 345, 345–346, 346, 346–347, 347, 347, 349, 35–37, 39, 39, 39, 39, 39–40, 42–45, 50, 50, 50–51, 54, 58–59, 61–64, 66, 66, 66–67, 67, 67, 69, 69, 69–71, 71, 71–73, 77–78, 82–83, 85–88, 9, 90, 92, 92, 92–93, 93, 93–94, 94, 94–95, 95, 95, 98–99
   logging.c87.50%80%100%88.64%24, 37–39, 46, 59–60
   port.c90.91%58.33%100%100%10, 15, 19, 24–25
   queues.c91.12%82.14%100%95.05%107, 112, 118, 21–23, 46–47, 59–60, 83–87
   reaction.c90.41%75%100%97.78%18, 20, 24, 43–44, 54, 56
   reactor.c35.40%16.67%80%48.81%10, 10, 10, 109, 11, 11, 11, 110, 12–13, 13, 13–14, 14, 14–15, 15, 15–16, 16, 16–17, 17, 17–19, 19, 19–20, 20, 20, 23, 23, 23–25, 25, 25–26, 26, 26, 28, 28, 28–31, 31, 31–32, 32, 32–33, 33, 33–35, 35, 35–36, 36, 36, 40, 40, 40–43, 43, 43–44, 44, 44–45, 45, 45–47, 47, 47–48, 48, 48, 52, 52, 52–53, 53, 53–54, 56, 69–70, 70, 70–71, 87, 9, 92–93, 93, 93–94
   scheduler.c81.79%67.05%94.12%87.02%102, 104, 104, 111, 125, 173, 176, 176, 176–179, 181–182, 182, 182–183, 203, 223–224, 230–232, 27, 276–277, 281, 285–286, 304, 31, 52–54, 54, 54–56, 56, 56, 58, 58, 58–59, 61–62, 62, 62–63, 69–72, 80–81, 85
   serialization.c50%50%50%50%16–17, 26–27, 33–35, 38–40
   tag.c40.19%31.48%60%47.92%14, 14–15, 17, 17–18, 23–24, 24, 24, 24, 24–25, 27, 27, 27, 27, 27–28, 30, 30, 30–31, 33–34, 34, 34–35, 37, 37, 37, 37, 37–38, 40, 40, 40, 40, 40–41, 43, 53–54, 63, 63–64, 83–85, 85, 85, 85, 85, 85, 85, 85, 85, 85, 85–87, 89
   timer.c94.59%66.67%100%100%14, 25
   trigger.c100%100%100%100%
src/platform/posix
   posix.c80.19%55.56%86.67%84.93%104, 16, 18, 20–21, 34–36, 38–40, 48–49, 62, 67, 76, 79, 92, 98
   tcp_ip_channel.c68.16%56.69%94.12%71.79%114–115, 120–121, 125–126, 145, 149, 149, 149, 153–154, 167–168, 171–172, 174, 174, 174, 176–177, 179–180, 187–188, 190, 193, 208, 213–214, 219–222, 225, 228, 231, 238–241, 243, 243, 243–245, 247–250, 253–254, 256, 284–286, 293, 298–300, 300, 300–301, 303–306, 315, 315–317, 32–33, 339–342, 342,

@LasseRosenow
Copy link
Collaborator Author

Okay I rebased this and removed the conn_channel_id from the NetworkChannel

The other small change is that I renamed the get_connection_state function in the TcpIpChannel to match the name of the NetworkChannel interface.

@LasseRosenow LasseRosenow changed the title Add get_dest_channel_id getter to NetworkChannel Remove dest_channel_id from NetworkChannel Nov 11, 2024
Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@erlingrj erlingrj merged commit fde8ec6 into main Nov 13, 2024
6 checks passed
@LasseRosenow LasseRosenow deleted the change-network-channel-dest-channel-id branch November 27, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants