Refactoring of event queue #390

byeonggiljun · 2024-03-08T00:25:54Z

In this PR, the event queue stores events using pqueue_tag to sort them in tagged order. Consequently, events scheduled with a tag with a non-zero microstep are handled the same as other events (next_q is deleted).

Some functionalities in pqueue_tag.c and pqueue_base.c are also changed.

The motivation for this refactoring is:

to prevent a federate sends NET (0, 1) when the federate has received T_MSG (0, 0). Normally, we can infer that the next event's tag is one microstep later than the current tag if the next event's time is the same as the current time. For example, if a next event's time is 1 sec and the current tag is (1 sec, 1), that event needs to be executed at (1 sec, 2) because every event at (1 sec, 1) must be popped before advancing to (1 sec, 1). The code below (in get_next_event_tag) shows that inference. However, an exception occurs at the start tag. At the start tag (0, 0), the event queue can contain an event with (0, 0). We still infer that the event's tag is (0, 1) and send NET (0, 1), which is wrong. If events are sorted by tag, not time, we can use each event's tag as the target tag.

reactor-c/core/threaded/reactor_threaded.c

Lines 290 to 293 in 7427d98

    
           if (next_tag.time == env->current_tag.time) { 
        
             LF_PRINT_DEBUG("Earliest event matches current time. Incrementing microstep. Event is dummy: %d.", 
        
                            event->is_dummy); 
        
             next_tag.microstep = env->current_tag.microstep + 1;

to make the scheduling process simpler. Because the events are sorted by time, not tag, we had to put chained dummy events to implement an event with a positive microstep. After this refactoring, we can schedule events with a tag because events are sorted by their tag. Thus, we can remove those chained dummy events and the next queue.

…on-zero microsteps events

…tes are same

…iority

hokeun

🚀

lib/schedule.c

byeonggiljun · 2024-03-19T15:28:38Z

Convert this to draft because mac-os tests are failing.

petervdonovan · 2024-04-02T04:08:45Z

The test that is failing has both decentralized execution and physical actions. It is very hard to make such a test both useful (in the sense of catching bugs) and non-flaky. In particular, DistributedLoopedPhysicalActionDecentralized checks that messages from the sender are processed in the correct order, which won't (necessarily) be true if there are STP violations.

I don't have a strong opinion about how to fix this. We have discussed pipe-dream kinds of improvements to the testing framework that could solve this sort of thing, but in the near term, I guess we can choose from a few unappealing options, such as moving the test to failing, not checking that messages are received in the right order, or deferring the check until the end after the tardy message has been received. I suppose I would lean toward the second option. The third of these could be rather complex, and it is already difficult to interpret the tests.

byeonggiljun · 2024-04-02T15:41:59Z

Thank you, @petervdonovan! I also agree that the test doesn't have to check the order of messages. I'll make a simple LF PR if it's the reason for the error.

Update: I found an error and fixed it. The problem was not in the test. And I think now this PR is ready for merging.

edwardalee

I need to review this more carefully, but I have one high-level question:
You changed the semantics of the cmppri comparison functions.
They used to return 0 for a < b and non-zero otherwise. You now have them returning -1 for a < b, 0 for a==b, and 1 for a > b.

Why did you make this change? The pqueue implementation doesn't need this, I think, and this change is what resulted in most of the changes in the base classes. Is this change necessary?

core/utils/pqueue_tag.c

byeonggiljun · 2024-04-10T05:12:57Z

Why did you make this change? The pqueue implementation doesn't need this, I think, and this change is what resulted in most of the changes in the base classes. Is this change necessary?

Thank you for starting the review, @edwardalee! I have a reason for that change although I'm not sure that change is the only solution.

Please look at the code below. This is a part of find_equal_same_priority in pqueue_base.c. This function is needed here (lib/schedule.c#L201-L205) to check if there is an already scheduled event with the same trigger and the same tag.

reactor-c/core/utils/pqueue_base.c

Line 100 in ec06c93

if (q->getpri(curr) == q->getpri(e) && q->eqelem(curr, e)) {

We use the == operator to check whether two priorities are identical. This works for the time-ordered queue. Nonetheless, pqueue_tag's priority is a pointer to a tag and the == operator does not work. Also, currently the cmp_pri (thiz, that) function only checks whether thiz > that or not. This is why I made the cmp_pri function return 0 if two priorities are the same. As a result, we can check the equality of the two priorities as below.

reactor-c/core/utils/pqueue_base.c

Line 133 in 0200a84

if (q->cmppri(q->getpri(curr), q->getpri(e)) == 0 && q->eqelem(curr, e)) {

edwardalee

This is a very nice piece of work! I love that quite a bit complicated code is removed. I've made some suggestions for further code removal and some comment updates. All of these are optional.

core/utils/pqueue_support.h

include/core/utils/pqueue_base.h

lib/schedule.c

include/core/reactor_common.h

include/core/lf_types.h

core/utils/pqueue_tag.c

Co-authored-by: Edward A. Lee <[email protected]>

functions for event

Now, we can know that an event is a dummy event if the trigger is NULL

byeonggiljun · 2024-04-11T19:50:49Z

Seems this PR is ready to be merged. Let me put this into the merge queue.

erlingrj

Hi @byeonggiljun and @edwardalee, I have a few questions about this. This seems like a good design, but there is something weird about how the priority queue is implemented where we are forced to cast between points and integers and it is not really portable.

I think we need to have pqueue_pri_t as a void *, this would work with no problem for the pqueue_tag.c as you have implemented it here, but for the pqueue.c that is used for the reaction queue, it would not work directly. But it should also adapt this design where reactions are sorted based on the values of their fields and never based on the integer values of the pointers.

erlingrj · 2024-04-12T06:45:50Z

core/environment.c

+//////////////////
+// Local functions, not intended for use outside this file.
+
+/**
+ * @brief Callback function to determine whether two events have the same trigger.
+ * This function is used by event queue and recycle.
+ * Return 1 if the triggers are identical, 0 otherwise.
+ * @param event1 A pointer to an event.
+ * @param event2 A pointer to an event.
+ */
+static int event_matches(void* event1, void* event2) {
+  return (((event_t*)event1)->trigger == ((event_t*)event2)->trigger);
+}
+
+/**
+ * @brief Callback function to print information about an event.
+ * This function is used by event queue and recycle.
+ * @param element A pointer to an event.
+ */
+static void print_event(void* event) {
+  event_t* e = (event_t*)event;
+  LF_PRINT_DEBUG("tag: " PRINTF_TAG ", trigger: %p, token: %p", e->base.tag.time, e->base.tag.microstep,
+                 (void*)e->trigger, (void*)e->token);
+}
+


These function definition seem a little out-of-place here in environment.c why not have them in pqueue.c or pqueue_tag.c?

The rationale for this is to avoid creating unnecessary dependencies.
Neither pqueue.c nor pqueue_tag.c have any dependence on event_t.
They are more generic. It is in evironment.c that pqueue_tag is adapted to contain items of type event_t. The way it does that is by providing these two functions as arguments to pqueue_tag_init_customize.
These functions are made static, so this design of the event queue is entirely defined in environment.c.

OK I see, thanks for the explanation.

erlingrj · 2024-04-12T07:34:38Z

core/utils/pqueue_tag.c

@@ -75,11 +64,21 @@ static void pqueue_tag_print_element(void* element) {
 //////////////////
 // Functions defined in pqueue_tag.h.

+int pqueue_tag_compare(pqueue_pri_t priority1, pqueue_pri_t priority2) {
+  return (lf_tag_compare(((pqueue_tag_element_t*)priority1)->tag, ((pqueue_tag_element_t*)priority2)->tag));


Here an unsigned long long is casted to an event_t * this will result in warnings on any architecture where sizeof(void *) != sizeof(unsigned long long)

I agree that pqueue_base should be changed so pqueue_pri_t is a void*. This, however, requires redesigning the reaction queue, which has a (probably premature) optimization that combines the deadline and the level into a single index that it uses for ordering reactions. I think this should be done as a separate PR. Do you want to open an issue? Probably we should benchmark the performance impact. There is likely to be a cost.

I think it is covered by this issue: #388

Use pqueue_tag instead of pqueue for event_q and simplfy scehduling n…

898fa1c

…on-zero microsteps events

byeonggiljun added the refactoring label Mar 8, 2024

byeonggiljun added 5 commits March 12, 2024 09:05

Schedule events with appropriate tag

52b5bbc

Apply clang-format

8e6226e

Merge branch 'main' into event-queue-refactoring

733f81b

Let pqueue_tag be able to compare payloads of elements

31a5b1b

Allow the priority comparison function to return 0 if received priori…

1d4d05c

…tes are same

byeonggiljun force-pushed the event-queue-refactoring branch from 62f2ab5 to 1d4d05c Compare March 12, 2024 23:20

byeonggiljun added 2 commits March 12, 2024 16:49

Cleanup codes

2ccd7cb

Add a function for finding an element in a priority queue with the pr…

ca62f29

…iority

byeonggiljun force-pushed the event-queue-refactoring branch from 02a2a38 to ca62f29 Compare March 13, 2024 02:14

byeonggiljun changed the title ~~Refactoring of event_q and scheduling for tags with non-zero microsteps~~ Refactoring of event queue Mar 13, 2024

Merge branch 'main' into event-queue-refactoring

64f21c4

byeonggiljun marked this pull request as ready for review March 13, 2024 03:01

byeonggiljun requested review from hokeun and lhstrh March 13, 2024 03:01

hokeun approved these changes Mar 13, 2024

View reviewed changes

lib/schedule.c Outdated Show resolved Hide resolved

byeonggiljun marked this pull request as draft March 19, 2024 15:28

Merge branch 'main' into event-queue-refactoring

d25282e

byeonggiljun added mac and removed mac labels Apr 1, 2024

Allow the mac default test

7fa757a

Correctly pile up super dense time

a3584f3

byeonggiljun force-pushed the event-queue-refactoring branch from e66e95f to a3584f3 Compare April 3, 2024 04:20

Revert change to CI and minor fix

0200a84

byeonggiljun marked this pull request as ready for review April 3, 2024 15:06

byeonggiljun requested a review from edwardalee April 3, 2024 18:52

byeonggiljun requested a review from petervdonovan April 3, 2024 18:52

edwardalee reviewed Apr 10, 2024

View reviewed changes

core/utils/pqueue_tag.c Outdated Show resolved Hide resolved

Update comments to align with the code

1bd320f

edwardalee approved these changes Apr 10, 2024

View reviewed changes

byeonggiljun and others added 6 commits April 10, 2024 17:00

Apply suggestions from code review

5ec3325

Co-authored-by: Edward A. Lee <[email protected]>

Add an authorship and remove unused functions

3b50275

Make pqueue_tag_init_customize more flexible and remove some callback

4b8d42d

functions for event

Minor fix

5c966b5

Remove the is_dummy flag from the event_q structure

739b696

Now, we can know that an event is a dummy event if the trigger is NULL

Move some callback functions for the event queue to environment.c

d10375d

byeonggiljun added this pull request to the merge queue Apr 11, 2024

Merged via the queue into main with commit 64c3f01 Apr 11, 2024
30 checks passed

byeonggiljun deleted the event-queue-refactoring branch April 11, 2024 23:21

erlingrj reviewed Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of event queue #390

Refactoring of event queue #390

byeonggiljun commented Mar 8, 2024 •

edited

Loading

hokeun left a comment

byeonggiljun commented Mar 19, 2024

petervdonovan commented Apr 2, 2024 •

edited

Loading

byeonggiljun commented Apr 2, 2024 •

edited

Loading

edwardalee left a comment

byeonggiljun commented Apr 10, 2024 •

edited

Loading

edwardalee left a comment

byeonggiljun commented Apr 11, 2024

erlingrj left a comment

erlingrj Apr 12, 2024

edwardalee Apr 12, 2024

erlingrj Apr 17, 2024

erlingrj Apr 12, 2024

edwardalee Apr 12, 2024

erlingrj Apr 17, 2024

	if (next_tag.time == env->current_tag.time) {
	LF_PRINT_DEBUG("Earliest event matches current time. Incrementing microstep. Event is dummy: %d.",
	event->is_dummy);
	next_tag.microstep = env->current_tag.microstep + 1;

Refactoring of event queue #390

Refactoring of event queue #390

Conversation

byeonggiljun commented Mar 8, 2024 • edited Loading

hokeun left a comment

Choose a reason for hiding this comment

byeonggiljun commented Mar 19, 2024

petervdonovan commented Apr 2, 2024 • edited Loading

byeonggiljun commented Apr 2, 2024 • edited Loading

edwardalee left a comment

Choose a reason for hiding this comment

byeonggiljun commented Apr 10, 2024 • edited Loading

edwardalee left a comment

Choose a reason for hiding this comment

byeonggiljun commented Apr 11, 2024

erlingrj left a comment

Choose a reason for hiding this comment

erlingrj Apr 12, 2024

Choose a reason for hiding this comment

edwardalee Apr 12, 2024

Choose a reason for hiding this comment

erlingrj Apr 17, 2024

Choose a reason for hiding this comment

erlingrj Apr 12, 2024

Choose a reason for hiding this comment

edwardalee Apr 12, 2024

Choose a reason for hiding this comment

erlingrj Apr 17, 2024

Choose a reason for hiding this comment

byeonggiljun commented Mar 8, 2024 •

edited

Loading

petervdonovan commented Apr 2, 2024 •

edited

Loading

byeonggiljun commented Apr 2, 2024 •

edited

Loading

byeonggiljun commented Apr 10, 2024 •

edited

Loading