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

Priority queue refactoring #306

Merged
merged 16 commits into from Nov 25, 2023
Merged

Priority queue refactoring #306

merged 16 commits into from Nov 25, 2023

Conversation

edwardalee
Copy link
Contributor

@edwardalee edwardalee commented Nov 11, 2023

This is a refactoring and cleanup of the pqueue infrastructure with the addition of a priority queue that uses tag ordering.
Currently, this priority queue (a pqueue_tag_t) is not used anywhere except in unit tests.
The first place where it should be used is to replace the queue used by the in_transit_message_tags field of the federate_info_t struct maintained by the RTI for each federate. Also, @byeong-gil 's NDT optimization requires such a queue.

Eventually, I believe this tag sorted queue could replace the event queue and significantly simply it without a performance penalty.

@edwardalee edwardalee changed the title Pqueue refactoring Priority queue refactoring Nov 12, 2023
@edwardalee edwardalee marked this pull request as ready for review November 12, 2023 01:59
@erlingrj
Copy link
Collaborator

Will this address this issue of RTI sending mistaken PTAGs: lf-lang/lingua-franca#2084

@lhstrh
Copy link
Member

lhstrh commented Nov 14, 2023

Will this address this issue of RTI sending mistaken PTAGs: lf-lang/lingua-franca#2084

I don't think it does -- this is just a refactoring AFAIK.

@byeonggiljun
Copy link
Collaborator

byeonggiljun commented Nov 15, 2023

Thank you for refactoring this, @edwardalee! This is really helpful for the NDT optimization.

In this commit (fb024fa), I made the function pqueue_tag_insert_tag doesn't insert an existing tag again to save the memory. I modified the unit test to test it too. I think this is ok because the function is only used when elements don't contain any payloads. If a user wants to use a customized element that contains a payload as described in this comment, they can use the pqueue_tag_insert function.

(Update)

It is a queue, not a set, so I wonder whether we really want to build this into the insert function. Maybe this should be checked before inserting?

According to @lhstrh's comment above, I just added the search function (pqueue_tag_find_same_tag) instead of checking the duplication inside the insert function.

Copy link
Contributor Author

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

I will try to look at this more closely later, but some quick feedback: You are right that we will need the capability to prevent accumulation in the queue of elements with the same tag. This will be particularly important if tags corresponding to the timeout time or FOREVER are posted, but we could get an unbounded accumulation of these without them ever getting cleared.

However, the solution looks expensive to me. Can we avoid mallocing and then freeing an element that is used just to conduct the search? I think the element can be put on the stack.

And perhaps the de-duplication should not be done by default. How about an alternate insert function (insert if not present)? Also, I suspect that instead of searching the queue, a low-level algorithm would be much more efficient. Probably it would just be a small variant of the bubble_up function in pqeueue_base that would perform the de-duplication while doing the insertion, instead of doing it in two separate steps.

(Actually, on reflection, probably this low-level idea won't be so easy... The bubble_up function in pqueue_base is destructive, in that it changes the data structure as it goes. If it finds a duplicate after making all these changes, then it likely will have to undo all those changes. Probably the two-phase approach is actually better.)

@byeonggiljun
Copy link
Collaborator

Thank you for the feedback, @edwardalee! I modified the code based on your comment.

Can we avoid mallocing and then freeing an element that is used just to conduct the search?

Now, the function find_same_tag only require a tag and mallocing is not needed anymore. One disadvantage is we need another function when we want to compare a payload as well as the tag. That's why I named the function find_same_tag instead of find_equal_same_tag.

How about an alternate insert function (insert if not present)?

I made the function here (pqueue_tag_insert_tag_if_not_present).

@edwardalee
Copy link
Contributor Author

Thank you for the feedback, @edwardalee! I modified the code based on your comment.

Can we avoid mallocing and then freeing an element that is used just to conduct the search?

Now, the function find_same_tag only require a tag and mallocing is not needed anymore. One disadvantage is we need another function when we want to compare a payload as well as the tag. That's why I named the function find_same_tag instead of find_equal_same_tag.

How about an alternate insert function (insert if not present)?

I made the function here (pqueue_tag_insert_tag_if_not_present).

I found a way to do this without replicating code from pqueue_base and without using malloc (the elements are on the stack).

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Looks great! Let's merge this.

include/core/utils/pqueue_tag.h Outdated Show resolved Hide resolved
@lhstrh lhstrh merged commit 53c0bf7 into main Nov 25, 2023
28 checks passed
@lhstrh lhstrh deleted the pqueue-refactoring branch November 25, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup enhancement Enhancement of existing feature refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants