-
Couldn't load subscription status.
- Fork 579
Simulator: refactor and simplify InteractionPlan
#3774
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
Open
pedrocarlo
wants to merge
13
commits into
tursodatabase:main
Choose a base branch
from
pedrocarlo:sim-refactor-interaction
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Simulator: refactor and simplify InteractionPlan
#3774
pedrocarlo
wants to merge
13
commits into
tursodatabase:main
from
pedrocarlo:sim-refactor-interaction
+2,099
−2,200
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…`Remaining` to a metrics file
|
Nyrkiö Report for Commit: a0c7abb No performance changes detected. Remember that Nyrkiö results become more precise when more commits are merged. So please check back in a few days. |
will track `Interaction` instead of `Interactions` in the Plan, this change will impossibilitate the serialization of the InteractionPlan with Serde Json. - make --load just load the previous cli args
2bec7a0 to
4808a9b
Compare
Modify `generation/property.rs` to use the Builder - add additional metadata to `Interaction` to give more context for shrinking and iterating over interactions that originated from the same interaction. - add Iterator like utilities for `InteractionPlan` to facilitate iterating over interactions that came from the same property:
to calculate metrics per generation step - simplify generation as we now only store `Interaction`. So now we can funnel most of the logic for interaction generation, metric update, and Interaction append in the `PlanGenerator::next`.
…te over properties, instead of handrolling property iteration
4808a9b to
eb0327b
Compare
eb0327b to
7a4498f
Compare
7a4498f to
a0c7abb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.

Depends on #3775 - to remove noise from this PR.
Motivation
In my continued efforts in making the simulator more accessible and simpler to work with, I have over time simplified and optimized some parts of the codebase like query generation and decision making so that more people from the community can contribute and enhance the simulator. This PR is one more step in that direction.
Before this PR, our
InteractionPlanstoredVec<Interactions>.Interactionsare a higher level collection that will generate a list ofInteraction(yes I know the naming can be slightly confusing sometimes. Maybe we can change it later as well. Especially becauseInteractionsare mainly justProperty). However, this architecture imposed a problem when MVCC enters the picture. MVCC requires us to make sure that DDL statements are executed serially. To avoid adding even more complexity to plan generation, I opted on previous PRs to check before emitting anInteractionfor execution, if the interaction is a DDL statement, and if it is, I emit aCommitfor each connection still in a transaction. This worked slightly fine, but as we do not store the actual execution of interactions in the interaction plan, only the higher levelInteractions, this meant that I had to do some workarounds to modify theInteractionsinside the plan to persist theCommitI generated on demand.Problem
However, I was stupid and overlooked the fact that for certain properties that allow queries to be generated in the middle (referenced as extensional queries in the code), we cannot specify the connection that should execute that query, meaning if a DDL statement occurred there, the simulator could emit the query but could not save it properly in the plan to reproduce in shrinking. So to correct and make interaction generation/emission less brittle, I refactored the
InteractionPlanso that it storesVec<Interaction>instead.Implications
Interactionis not currently serializable usingSerdedue to the fact that it stores a function inAssertion. This means that we cannot serialize the plan into aplan.json. Which to me is honestly fine, as the only things that usedplan.jsonwas--loadand--watchoptions. Which are options almost nobody really used.For load, instead of generating the whole plan it just read the plan from disk. The workaround for that right now is just load the
cli_optsthat were last run for that particular seed and use those exact options to run the simulation.For watch, currently there is not workaround but, @alpaylan told me has some plans to make assertions serializable by embedding a custom language into the
plan.sqlfile, meaning we will probably not need a json file at all to store the interaction plan. And this embedded language will make it much easier to bring back a more proper watch mode.The current shrinking algorithms all have some notion of properties and removal of properties, but
Interactiondo not have this concept. So I added some metadata to interactions and a origin ID to eachInteractionso that we can search through the list of interactions using binary search to get all of the interactions that are part of the sameProperty. To support this, I added anInteractionBuilderand some utilities to iterate and remove properties in theInteractionPlanConclusion
Overall, this code simplifies emission of interactions and ensures the
InteractionPlanalways stores the actual interactions that get executed. This also decouples more query generation logic from query emission logic.