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

Simulation result staleness checking #1595

Merged
merged 5 commits into from
Nov 12, 2024
Merged

Conversation

JoelCourtney
Copy link
Contributor

Description

This PR implements a staleness check for the internal procedural scheduling implementations. The editable plan instance keeps track of sim results that it has produced using weak references, and can dynamically update their staleness if the plan is changed after it was simulated. The process is this:

  1. InMemoryEditablePlan has a set of weak references to simulation results objects that are currently up-to-date. I used weak references because if the user can't access it anymore, staleness doesn't matter and we might as well let it get gc'ed.
  2. When the user gets simulation results, either through simulation or by getting the latest, it always checks for plan equality between the returned results and the current plan, even if we just simulated. If it is up-to-date, a weak ref is added to the set.
  3. When an edit is made, the sim results in the current set are marked stale; then the set is reset to new reference to an empty set.
  4. When a commit is made, the commit object takes shared ownership of the set. If a new simulation is run (step 2) the plan can still add to the set while it is still jointly owned by the commit. Then when an edit is made (step 3) the commit will become the sole owner of the set.
  5. When changes are rolled back, any sim results currently in the plan's set are marked stale, the previous commit's sim results are marked not stale, then the plan will resume joint ownership of the previous commit's set.

The joint ownership freaks me out a wee bit, but I think it's safe because the commits are only used to keep the previous sets from getting gc'ed in the event of a rollback. Only the plan object actually mutates the set.

Verification

I added some unit tests to the scheduler-driver.

Documentation

This doesn't need to be explained to the user, but I've copied the above description into the doc comment on InMemoryEditablePlan.

Future work

  • The stateless scheduler will probably need something like this too.
  • I'm expecting that when I implement deletion, we will get false positives of staleness, because I've intentionally stayed away from constantly performing plan equality checks and storing a bunch of copies of the plan. Maybe that was premature optimization. The false positive would happen like this:
    1. simulate
    2. add activity; sim results marked stale
    3. delete activity (not rollback); sim results not marked un-stale.
    4. sim results now say they are stale when they actually aren't.

@JoelCourtney JoelCourtney self-assigned this Nov 8, 2024
Copy link
Contributor

@Mythicaeda Mythicaeda left a comment

Choose a reason for hiding this comment

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

LGTM! Does moving Commit inside of the InMemoryEditablePlan help ease any anxieties about the shared ownership?

@JoelCourtney
Copy link
Contributor Author

Does moving Commit inside of the InMemoryEditablePlan help ease any anxieties about the shared ownership?

Nope. I just did it because the Commit class isn't used anywhere else, so I didn't want to make it look like it was part of a public interface.

@JoelCourtney JoelCourtney merged commit f271123 into develop Nov 12, 2024
10 checks passed
@JoelCourtney JoelCourtney deleted the feat/sim-result-staleness branch November 12, 2024 01:01
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