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

[vm] Delta writes in sequential execution #2135

Closed
wants to merge 3 commits into from

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Jul 21, 2022

Description

Slightly refactored delta operations so that Aggregator 1) can reuse apply_to functionality based on operation 2) can reuse unique serialization/deserialization for delta op values (This way it should also be easier to extract these deltas to ChangeSet, etc.). Added tests.

Implemented delta application for sequential execution. @gelash @zekun000 I am looking for a place to put tests in the form of a) generate writeset with deltas b) apply with fake executor / fake data store c) check result of application and/or VM status. I suppose e2e-tests/src/executor.rs might be the best place for that? Or a separate file?

Test Plan

  • Unit tests
  • Fake executor tests

This change is Reviewable

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

overall seems fine...

My bigger concern is on the first PR, where we use unreachable instead of error, where we have yet to implement the validation to make sure that a Delta::Op can't leave the VM / executor.

@georgemitenkov
Copy link
Contributor Author

@davidiw Speaking about unreachable: there are 3 places where this is the case:

  1. transaction replay: we match by AccessPath and since we don't use it for aggregator's state key - should never be exposed.
  2. in try_access_path_into_write_set_change and try_table_item_into_write_set_change - again, AccessPath is never used for aggregator.
  3. In InMemoryStateCalculator: this is used for storage and deltas should be converted by then.

I agree that this is not nice but at least it allows to proceed with implementation on executor side (and we not cresting deltas just yet anywhere, so fine for now?). I am working on moving this to ChangeSet or TransactionOutput (separate delta set) in parallel.

@georgemitenkov georgemitenkov force-pushed the george/sequential-delta-writes branch from a4e4102 to 9bcecc8 Compare July 26, 2022 11:01
@georgemitenkov
Copy link
Contributor Author

Will be rebasing this on #2222.

TODOs: prologue/epilogue

@davidiw davidiw deleted the george/sequential-delta-writes branch August 11, 2022 02:40
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