Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

Improve system scheduler logic #54

Merged

Conversation

metrowaii
Copy link
Contributor

Closes #53

Copy link
Owner

@evaera evaera left a comment

Choose a reason for hiding this comment

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

This change was pretty difficult for me to grok at first review. I wasn't sure why this change fixed the case we're trying to fix, but I think I understand now. Part of the reason why I didn't understand at first is because this change is actually different than the one that I discussed in the matter channel.

I had originally suggested that we just ignore systems listed within after if those systems occur at an earlier priority than this system. This would mean that you could be specifying both after and priority in the same system, which seems kind of redundant.

I think the change made in this PR is a little bit too permissive though. In the test case, a system that runs at priority 0 with its after set to a system with a priority means that a priority 0 system is actually running after a system with priority 2. I don't think this makes sense.

Right now, the implicit priority of a system is 0 if one is not specified. I think we need to change this contract slightly such that systems with no priority set just don't have a priority. Alongside this change, we should probably make it so that specifying both priority and after together in the same system is an error. It doesn't make sense for a system with an earlier priority to run after another system with a later priority.

If possible, I'd like to clean. up this scheduling code to make it easier to understand. Specifically, the late assignment of the priority variable makes this code difficult to follow

Also, remember to update the CHANGELOG!

@metrowaii metrowaii changed the title Reorganized system ordering logic & added test case Improve system scheduler logic Mar 23, 2023
@metrowaii metrowaii requested a review from evaera March 23, 2023 23:41
@evaera evaera merged commit cdf0867 into evaera:main May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle case where priority of a system is earlier than systems defined in after
2 participants