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

Implements PriorityTaskQueue with std's LinkedList rather than a custom linked list implementation #706

Merged
merged 3 commits into from
Apr 18, 2023

Conversation

michaelRichards99
Copy link
Contributor

Fixes #377.

@michaelRichards99
Copy link
Contributor Author

michaelRichards99 commented Apr 12, 2023

We tested with the existing micro benchmarks from the rusty-hermit project's benchmark directory.

With these changes, the scheduling time for 2 threads was an average of 2880 ticks.
With the custom implementation, it was an average of 2730 ticks.
This is only testing with 2 Tasks on the queue however, it would likely change with more tasks. We could write a benchmark for this if you would like to see more numbers.

Another potential issue with this implementation is that LinkedList's remove method is unstable, so we used a work around to avoid that.

@mkroening mkroening self-requested a review April 12, 2023 05:50
@mkroening mkroening self-assigned this Apr 12, 2023
@stlankes
Copy link
Contributor

Looks good! But I will discuss it with @mkroening Just a double check.

Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks a lot!

Two minor remarks, though the current code is definitely an improvement already. 👍

src/scheduler/task.rs Outdated Show resolved Hide resolved
src/scheduler/task.rs Outdated Show resolved Hide resolved
…d correctly set priority bitmask to reflect empty lists.
@michaelRichards99
Copy link
Contributor Author

Thank you for the feedback :). We've addressed those 2 changes and made sure the priority bitmap is properly set when removing a task from a list.

Thank you @ynchen2829 for authoring these changes.

Copy link
Member

@mkroening mkroening 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 to me!

Thanks a lot! :)

bors r+

@@ -272,6 +272,30 @@ impl PriorityTaskQueue {
task
}

/// Remove the task at index from the queue and return that task,
/// or None if the index is out of range or the list is empty.
fn remove_from_queue(
Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice! You even made this a fully fledged method. 👍

@bors
Copy link
Contributor

bors bot commented Apr 18, 2023

Build succeeded:

@bors bors bot merged commit 05fd31d into hermit-os:master Apr 18, 2023
8 checks passed
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.

Migrate PriorityTaskQueue to use std's LinkedList
3 participants