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

Delete im #585

Merged
merged 3 commits into from Dec 8, 2022
Merged

Delete im #585

merged 3 commits into from Dec 8, 2022

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Nov 27, 2022

This still has some TODOs in the form of error codes to consolidate/reassign, but the code is otherwise I think as it should be.

This PR deletes the im library previously backing the Host collection objects (maps and vecs), and also reworks the comparison system to avoid #579. It replaces both vecs and maps with simple eagerly-copied std::Vecs (maps are sorted std::Vec<(K,V)>s). It also eliminates all the WeakHost and EnvVal usage in the Host; with a little more work plus catching up the SDK it should be possible to remove EnvVal entirely. In addition to removing WeakHost it also removes all the auxiliary pointers to Budget stored in "metered" host objects; a single Budget and/or Host is passed in to each operation that needs it.

The only odd part of this is that maps and vecs no longer "share substructure" via the BTree/RRB code in im -- every edit makes a full copy -- but some consideration suggests this probably isn't a huge issue:

  • The host vecs and maps still "share substructure" of any host objects they refer to; they're just slabs of RawVals, so are still "shallow" in that sense (and the storage map entries are changed from Box to Rc, so cloning them is "shallow" now too).
  • For small maps (less than 64 entries) im would be holding everything in a single BTree or RRB node anyways, which means this is no less efficient. The sharing only kicked in on larger structures, and ... we're likely not going to have big maps or vecs available to users.
  • Since we're removing EnvVal (which used to comprise the map and vec entries) the data consumption actually shrinks with this change (again so long as you're dealing with small maps or vecs).
  • I've been fairly careful to make all the operations that build new maps or vecs do so with a single exact-size allocation, again making these smaller for small maps and also relatively efficient. We can make them more efficient still by going to a per-host arena allocator in the future, if we want.

Comments welcome!

@graydon graydon force-pushed the delete-im branch 4 times, most recently from b147557 to 45c251b Compare November 29, 2022 04:49
@graydon graydon marked this pull request as ready for review December 6, 2022 00:39
@graydon graydon requested review from sisuresh and a team as code owners December 6, 2022 00:39
@leighmcculloch
Copy link
Member

Is it happening?

@graydon
Copy link
Contributor Author

graydon commented Dec 6, 2022

@leighmcculloch I believe so, if you would be inclined to review! I have pushed a rebase and it should be ready now.

(It'd be good to do soonish since I will be out after thursday, and this will bitrot fairly quickly)

Copy link
Contributor

@jayz22 jayz22 left a comment

Choose a reason for hiding this comment

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

Half way through my review, will continue tomorrow.

soroban-env-common/src/compare.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/comparison.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_vector.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_vector.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_vector.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_vector.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_vector.rs Outdated Show resolved Hide resolved
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

👍🏻. My knowledge of some of the intricacies is limited now so I think this should probably get an ✅ from another person, but it looks good to me overall.

soroban-env-common/src/raw_val.rs Show resolved Hide resolved
soroban-env-host/src/host/comparison.rs Show resolved Hide resolved
@graydon graydon enabled auto-merge (squash) December 8, 2022 23:47
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.

None yet

4 participants