[move][stdlib] Add efficient BigOrderedMap implementation #14753

igor-aptos · 2024-09-25T16:31:30Z

Description

Having efficient and concurrent large-sized "Map" and "OrderedMap" implementations is useful across variety of needs.

Current SmartTable implementation has various limitations, from it being sequential, to it not having smart ways to deal with collisions. It cannot be improved - as the structs are unmodifiable, and so should be deprecated fully.

In this PR:

provide efficient "OrderedMap" implementation. BTreeMap is chosen as it is best suited for the onchain storage layout - where majority of cost comes from loading and writing to storage items, and there is no partial read/write of them. It also rebalances in a way that reduces amount writes needed
writing to keys that are not close enough, to a BTreeMap with multiple layers, is generally parallel. More elements in the BTreeMap, the more operations will be parallel.
is an enum, so can be evolved in the future as needed
has an option to pre-pay and pre-allocate storage slots, and to reuse them, achieving predictable gas charges.
- defined potentially generally useful SlotsStorage, to manage indexed storage slots for datastructures that need it. Easier to use and more flexible/configurable than directly using Table. (which is also an enum, and so can be evolved / new strategies can be added)

How Has This Been Tested?

provided extensive unit tests. will probably consolidate and remove some before committing (to not make CI slower/more expensive), but for development those were useful.

Key Areas to Review

Original implementation is @grao1991 's (and that is the first commit in the stack), on top of it:
inline values in the leaf nodes. make max degree of inner nodes separately configurable from max degree of leaf nodes.
only modify necessary nodes, to reduce costs and achieve parallelism.
made keys generic. removed drop+copy requirement on the values
fixed indexing of nodes, and extracted (potentially generally useful SlotsStorage), allowing preallocating/reusing storage slots.

Type of Change

New feature

Which Components or Systems Does This Change Impact?

Aptos Framework

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

trunk-io · 2024-09-25T16:31:34Z

⏱️ 1h 25m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-unit-coverage	14m	🟩
rust-move-unit-coverage	12m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-unit-coverage	10m	🟩
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
general-lints	7m	🟩 🟩 🟩 🟩
check-dynamic-deps	5m	🟩 🟩 🟩 🟩
rust-move-tests	4m	🟥
rust-move-tests	4m	🟥
rust-move-tests	4m	🟥
rust-move-tests	4m	🟥
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	45s	🟩 🟩 🟩 🟩
file_change_determinator	40s	🟩 🟩 🟩 🟩
permission-check	11s	🟩 🟩 🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-09-25T16:44:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.4%. Comparing base (76e0b76) to head (e5ee36d).

Additional details and impacted files

@@                Coverage Diff                @@
##           igor/ordered_map   #14753   +/-   ##
=================================================
  Coverage              57.4%    57.4%           
=================================================
  Files                   859      859           
  Lines                211663   211663           
=================================================
  Hits                 121527   121527           
  Misses                90136    90136

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

aptos-move/framework/aptos-stdlib/sources/data_structures/slots_storage.move

msmouse · 2024-09-26T01:12:43Z

The world with Enums is beautiful 🚀

lightmark

haven't fully review the tree code with some comments first.

lightmark · 2024-09-26T10:42:37Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+    /// Destroys the tree if it's empty, otherwise aborts.
+    public fun destroy_empty<K: store, V: store>(self: BTreeMap<K, V>) {
+        let BTreeMap::V1 { nodes, root_index, min_leaf_index: _, max_leaf_index: _, inner_max_degree: _, leaf_max_degree: _ } = self;
+        aptos_std::debug::print(&nodes);


aptos-move/framework/aptos-stdlib/sources/data_structures/slots_storage.move

lightmark · 2024-09-26T15:32:21Z

aptos-move/framework/aptos-stdlib/sources/data_structures/slots_storage.move

+    enum SlotsStorage<T: store> has store {
+        Simple {
+            slots: Table<u64, Link<T>>,
+            new_slot_index: u64,


why do we need the non-reuse version?

if one app does all inserts and removals, they might want to get the refunds back, instead of worrying about amortization

lightmark · 2024-09-26T15:48:43Z

aptos-move/framework/move-stdlib/sources/vector.move

+    /// Returns a newly allocated vector containing the elements in the range [at, len).
+    /// After the call, the original vector will be left containing the elements [0, at)
+    /// with its previous capacity unchanged.
+    public fun split_off<Element>(self: &mut vector<Element>, at: u64): vector<Element> {


This deserves a native functions, so does the next one.

implemented in the stack

lightmark · 2024-09-26T15:49:38Z

aptos-move/framework/aptos-stdlib/sources/table.move

@@ -87,7 +87,7 @@ module aptos_std::table {
        drop_unchecked_box<K, V, Box<V>>(self)
    }

-    public(friend) fun destroy<K: copy + drop, V>(self: Table<K, V>) {
+    public fun destroy_empty<K: copy + drop, V>(self: Table<K, V>) {


IIRC, this function does NOT check the Table is empty...
Could we confirm with @wrwg ?

there's no way to check if a table is empty or not, that's why we didn't expose destroy_empty as a public function

lightmark · 2024-09-26T16:08:02Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+    /// An iterator to iterate all keys in the BTreeMap.
+    enum Iterator<K> has copy, drop {
+        End,
+        Some {


name: None and Next?

It's important it is End.

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

zekun000 · 2024-09-26T21:19:01Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+    }
+
+    // Returns true iff the iterator is an end iterator.
+    public fun is_end_iter<K: store, V: store>(_tree: &BTreeMap<K, V>, iter: &Iterator<K>): bool {


this feels more like inline func than actual func?

cannot be done publicly, as enum variants are not public. also some implementation might actually need to do something here.

zekun000 · 2024-09-26T21:21:37Z

aptos-move/framework/aptos-stdlib/sources/table.move

@@ -87,7 +87,7 @@ module aptos_std::table {
        drop_unchecked_box<K, V, Box<V>>(self)
    }

-    public(friend) fun destroy<K: copy + drop, V>(self: Table<K, V>) {
+    public fun destroy_empty<K: copy + drop, V>(self: Table<K, V>) {


there's no way to check if a table is empty or not, that's why we didn't expose destroy_empty as a public function

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

lightmark · 2024-09-30T12:24:36Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+        Inner {
+            // The max key of its child, or the key of the current node if it is a leaf node.
+            max_key: K,
+            // The node index of it's child, or NULL_INDEX if the current node is a leaf node.


This enum is somehow confusing to me. Could you draw a ascii diagram to give an example?

NULL_INDEX if the current node is a leaf node

If this is a leaf node, it should be Leaf, right?

lightmark · 2024-09-30T12:25:34Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+    public fun new_with_config<K: store, V: store>(inner_max_degree: u16, leaf_max_degree: u16, reuse_slots: bool, num_to_preallocate: u64): BTreeMap<K, V> {
+        assert!(inner_max_degree == 0 || inner_max_degree >= DEFAULT_INNER_MIN_DEGREE, E_INVALID_PARAMETER);
+        assert!(leaf_max_degree == 0 || leaf_max_degree >= DEFAULT_LEAF_MIN_DEGREE, E_INVALID_PARAMETER);
+        let nodes = if (reuse_slots) {


any reason we don't want to reuse slots?

lightmark · 2024-09-30T13:22:39Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+            let new_node_children = children.split_off(target_size - 1);
+            children.insert(l, child);
+            new_node_children
+        } else {
+            children.insert(l, child);
+            children.split_off(target_size)


why do we need two cases? Just

children.insert(l, child); children.split_off(target_size)

is good. unless it's for perf.

it was for perf. since it is only on rebalancing, cleaned it up.

lightmark · 2024-09-30T13:42:46Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+        let BTreeMap::V1 { nodes, root_index, min_leaf_index: _, max_leaf_index: _, inner_max_degree: _, leaf_max_degree: _ } = self;
+        aptos_std::debug::print(&nodes);
+        nodes.remove(root_index).destroy_empty_node();
+        nodes.destroy_empty();


As we discussed, how to gc?

lightmark · 2024-09-30T14:45:34Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+            self.nodes.borrow_mut(*prev).next = left_node_index;
+        };
+
+        if (!*is_leaf) {


comments... update the non-leaf children parents pointer to the left node, which was the original node with new index.

lightmark · 2024-09-30T14:48:52Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+        let left_node_slot = self.nodes.create_transient_slot();
+        let left_node_index = left_node_slot.get_index();
+        right_node.next = *next;
+        *next = node_index;


please comment
// node_index == right_node_index;

lightmark · 2024-09-30T14:51:08Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+        };
+
+        // # of children in the current node exceeds the threshold, need to split into two nodes.
+        let (right_node_slot, node) = self.nodes.transiently_remove(node_index);


why not reuse this node as the left node?

lightmark · 2024-09-30T15:49:33Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+            // The brother node has enough elements, borrow an element from the brother node.
+            brother_size = brother_size - 1;
+            if (brother_index == next) {
+                let borrowed_element = brother_children.remove(0);


we need a native remove too.

lightmark · 2024-09-30T15:51:27Z

aptos-move/framework/aptos-stdlib/sources/data_structures/btree_map.move

+                };
+                let borrowed_max_key = borrowed_element.max_key;
+                children.push_back(borrowed_element);
+                current_size = current_size + 1;


unnecessary? just make current_size - 1 at the following line?

…lues

igor-aptos requested review from davidiw, grao1991, gelash, georgemitenkov and ziaptos September 25, 2024 16:31

igor-aptos requested review from junkil-park, movekevin and wrwg as code owners September 25, 2024 16:31

igor-aptos requested a review from lightmark September 25, 2024 16:32

igor-aptos force-pushed the igor/vector_utilities branch from 4ae8d66 to 6e1add6 Compare September 25, 2024 20:31

igor-aptos force-pushed the igor/btree_map branch 2 times, most recently from 3ffc3e4 to 95aaf51 Compare September 25, 2024 23:15

msmouse reviewed Sep 26, 2024

View reviewed changes

lightmark reviewed Sep 26, 2024

View reviewed changes

igor-aptos force-pushed the igor/vector_utilities branch from 6e1add6 to 9e1d6af Compare September 26, 2024 17:26

igor-aptos force-pushed the igor/btree_map branch from 95aaf51 to c540209 Compare September 26, 2024 17:26

zekun000 reviewed Sep 26, 2024

View reviewed changes

lightmark reviewed Sep 30, 2024

View reviewed changes

igor-aptos force-pushed the igor/vector_utilities branch from 9e1d6af to 0d2ea8f Compare October 3, 2024 20:19

igor-aptos changed the base branch from igor/vector_utilities to main October 3, 2024 20:20

igor-aptos force-pushed the igor/btree_map branch from c540209 to bde797a Compare October 4, 2024 20:29

igor-aptos requested a review from vgao1996 as a code owner October 4, 2024 20:29

igor-aptos changed the title ~~[move][stdlib] Add efficient BTreeMap implementation~~ [move][stdlib] Add efficient BigOrderedMap implementation Oct 4, 2024

igor-aptos changed the base branch from main to igor/ordered_map October 4, 2024 20:31

igor-aptos force-pushed the igor/btree_map branch from bde797a to bd71740 Compare October 4, 2024 20:50

igor-aptos force-pushed the igor/ordered_map branch from 563d169 to d58cde2 Compare October 4, 2024 22:38

igor-aptos force-pushed the igor/btree_map branch from bd71740 to 09cdccf Compare October 4, 2024 22:41

igor-aptos force-pushed the igor/ordered_map branch from d849f67 to db1b82e Compare October 15, 2024 20:20

igor-aptos force-pushed the igor/btree_map branch from 499f622 to fe2da31 Compare October 15, 2024 20:20

igor-aptos force-pushed the igor/ordered_map branch from db1b82e to c9b6157 Compare October 16, 2024 21:11

igor-aptos force-pushed the igor/btree_map branch from fe2da31 to 17ccbb2 Compare October 16, 2024 21:12

igor-aptos force-pushed the igor/ordered_map branch from c9b6157 to 76e0b76 Compare October 16, 2024 21:18

igor-aptos force-pushed the igor/btree_map branch from 17ccbb2 to e5ee36d Compare October 16, 2024 21:19

igor-aptos force-pushed the igor/ordered_map branch from 76e0b76 to da9d4df Compare October 21, 2024 23:27

igor-aptos force-pushed the igor/btree_map branch from e5ee36d to 7799fbf Compare October 21, 2024 23:27

igor-aptos force-pushed the igor/ordered_map branch from da9d4df to e621db5 Compare October 22, 2024 23:11

igor-aptos force-pushed the igor/btree_map branch from 7799fbf to e4ddea5 Compare October 22, 2024 23:11

igor-aptos force-pushed the igor/ordered_map branch from e621db5 to 3fab948 Compare November 9, 2024 00:23

igor-aptos force-pushed the igor/btree_map branch from e4ddea5 to e8a2248 Compare November 9, 2024 00:23

igor-aptos added 16 commits November 12, 2024 10:06

original Guotang's commit

fe72e6c

Update BTreeMap to only modify leaf node on happy path, and inline va…

5d46708

…lues

removed drop+copy requirement on value

9ee9e6b

making key generic

ced715e

split order (max_degree) for inner and leaf nodes

1458371

using move 2 syntax

eadcf7c

fixing slots, and creating SlotsStorage, to add reusing slots afterwards

f4318be

add reusable/preinitialized storage slots

c06fa77

updating function naming to be consistent

24a5fe3

addressing review comments

c993ba1

Separate BigOrderedMap and OrderedMap

32c2932

Documentation and method names. handling sizes

2f19df0

inlining v1

ebe52c4

remove 'parent' field in Node

a129e11

inline root node directly

ad50fc9

shortcircuit when inlined (i.e. when root is leaf)

76262a8

igor-aptos force-pushed the igor/ordered_map branch from 3fab948 to 33d382b Compare November 12, 2024 18:06

igor-aptos force-pushed the igor/btree_map branch from e8a2248 to 76262a8 Compare November 12, 2024 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move][stdlib] Add efficient BigOrderedMap implementation #14753

[move][stdlib] Add efficient BigOrderedMap implementation #14753

igor-aptos commented Sep 25, 2024

trunk-io bot commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

msmouse commented Sep 26, 2024

lightmark left a comment

lightmark Sep 26, 2024

lightmark Sep 26, 2024

igor-aptos Oct 4, 2024

lightmark Sep 26, 2024

igor-aptos Oct 4, 2024

lightmark Sep 26, 2024

zekun000 Sep 26, 2024

igor-aptos Oct 4, 2024

lightmark Sep 26, 2024

igor-aptos Oct 4, 2024

zekun000 Sep 26, 2024

igor-aptos Oct 4, 2024

zekun000 Sep 26, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

igor-aptos Oct 4, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

lightmark Sep 30, 2024

[move][stdlib] Add efficient BigOrderedMap implementation #14753

Are you sure you want to change the base?

[move][stdlib] Add efficient BigOrderedMap implementation #14753

Conversation

igor-aptos commented Sep 25, 2024

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Sep 25, 2024 • edited Loading

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

msmouse commented Sep 26, 2024

lightmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trunk-io bot commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading