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

[loader-v2] Refactor how environment is stored in unsync storages and remove V1 from tests #15280

Closed

Conversation

georgemitenkov
Copy link
Contributor

@georgemitenkov georgemitenkov commented Nov 14, 2024

Description

This PR refactors UnsyncModuleStorage and UnsyncCodeStorage so that they do not own the runtime environment. The goal is to make them stateless, which would allow us to get rid of clones and Arcs for AptosEnvironment. Currently, this is tricky to do because storages consume the environment clone.

On this PR:

  • Changed InMemoryStorage to also store environment. This is helpful because it allows to store deserializer config inside, which is needed anyway. This way storage interfaces are unified: adapter, Block-STM's latest view and this - all store a reference to some bytes + environment.
  • Changed StateViewAdapter to also store environment.
  • Removed environment from Unsync...Storages because now it is provided by the trait bound on S: ... + WithRuntimeEnvironment. This way we do not need to own it there. Moved tests to integration-tests/code_storage_tests.rs.
  • Adapted all tests to work with that. Things become cleaner!

Bonus: as loader V2 is default, removed legacy V1 flows. For loader_tests.rs in integration-tests I think we can remove some (cc @vgao1996) because size check is removed. For concurrent tests, will bring them back separately (tracked as a TODO).

How Has This Been Tested?

Existing tests, this PR just moves some code around.

Type of Change

  • Refactoring

Which Components or Systems Does This Change Impact?

  • Move/Aptos Virtual Machine

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Nov 14, 2024

⏱️ 4h 6m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
rust-cargo-deny 35m 🟩🟩🟩🟩 (+15 more)
check-dynamic-deps 29m 🟩🟩🟩🟩🟩 (+15 more)
rust-move-tests 13m 🟩
rust-move-tests 13m 🟩
rust-move-tests 13m 🟩
rust-move-tests 13m 🟩
rust-move-tests 12m 🟩
rust-move-tests 12m 🟩
rust-move-tests 12m 🟩
rust-move-tests 11m
general-lints 10m 🟩🟩🟩🟩 (+15 more)
rust-move-tests 9m 🟩
rust-move-tests 9m 🟥
rust-move-tests 9m 🟩
semgrep/ci 8m 🟩🟩🟩🟩🟩 (+15 more)

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

georgemitenkov commented Nov 14, 2024

@georgemitenkov georgemitenkov changed the title Loader V2 refactoring: - Unify imports - Move environment from storages into ModuleBytesStorages - Move unsync code storage implementation tests - Remove V1 loader test flows [loader-v2] Refactor how environment is stored in unsync storages and remove V1 from tests Nov 14, 2024
@georgemitenkov georgemitenkov force-pushed the george/loader-v2-todos-2 branch 5 times, most recently from 1c23787 to cd35e7d Compare November 15, 2024 15:30
@georgemitenkov georgemitenkov force-pushed the george/loader-v2-todos-2 branch 2 times, most recently from 01c6a08 to f4e1b3f Compare November 19, 2024 14:22
  - Unify imports
  - Move environment from storages into ModuleBytesStorages
  - Move unsync code storage implementation tests
  - Remove V1 loader test flows
@georgemitenkov
Copy link
Contributor Author

Breaking PR into multiple

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.

1 participant