[framework] smart_vector and big_vector #5690

lightmark · 2022-11-23T08:44:15Z

Description

This PR productionize big_vector and smart_vector to help move developer handle large sequencing containers on Aptos.
Basically they use Table as bucket to batch k elements in one table item.

Test Plan

cargo test

msmouse · 2022-11-28T18:37:36Z

without actual reviewing: should delete aptos-core/aptos-move/move-examples/data_structures/sources/big_vector.move

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

msmouse · 2022-11-29T01:24:31Z

(sorry, accidental unqualified approval)

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

areshand · 2022-11-29T01:41:12Z

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

+    /// `SmartVector` should not enforece that.
+    struct SmartVector<T> has store {
+        inline_inner: vector<T>,
+        table_inner: vector<BigVector<T>>,


why is this vector<BigVector> not BigVector?

vector here is actually an option w/o Drop ability requirement of T to save space of a table handle when the smart_vector is small.

can we have some documentation here? otherwise, it is quite hard to understand why having a vector here.

+1 on adding some quick comments to explain

This (or something similar) may help to clarify:

spec struct SmartVector { invariant vector::length(inline_inner) + vector::length(table_inner) == 1; }

movekevin

Just some relatively minor comments. This makes sense in general but I'll take a much closer look after design discussion.

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

movekevin · 2022-11-29T21:43:04Z

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

+        assert!(i < len, error::invalid_argument(EINDEX_OUT_OF_BOUNDS));
+        let val = swap_remove(v, i);
+        if (i + 1 < len - 1) {
+            range_reverse(v, i + 1, len - 1);


Why are we using range_reserve instead of just simply shifting every element to the left by one (e.g. swapping)?

That seems better. Yeah, let me change it.

movekevin · 2022-11-29T21:50:13Z

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

+    /// Return if an element equal to e exists in the vector v.
+    /// disclaimer: this function is costly. Use it at your own discretion.
+    public fun contains<T>(v: &BigVector<T>, val: &T): bool {
+        if (is_empty(v)) return false;


IMO this doesn't save much gas vs calling index_of as the main loop there would not run anyway. It can therefore be removed for simplicity.

movekevin · 2022-11-29T21:50:49Z

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

+        length(v) == 0
+    }
+
+    #[test_only]


Can we add more tests with more edge cases?

movekevin · 2022-11-29T21:51:48Z

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

+    /// `SmartVector` should not enforece that.
+    struct SmartVector<T> has store {
+        inline_inner: vector<T>,
+        table_inner: vector<BigVector<T>>,


+1 on adding some quick comments to explain

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

movekevin · 2022-11-29T22:01:08Z

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

+        };
+        destroy(v);
+    }
+}


Can we add more unit tests with edge cases?

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

msmouse · 2022-11-30T22:11:46Z

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

+        inline_inner: vector<T>,
+        table_inner: vector<BigVector<T>>,


first_bucket and extra_buckets sound more expressive, just my 2 cents.

I renamed them as inline_vec and big_vec

areshand · 2022-11-30T22:50:52Z

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

+                vector::push_back(&mut v.inline_inner, val);
+                return
+            };
+            let estimated_avg_size = max((size_of_val(&v.inline_inner) + val_size) / (inline_len + 1), 1);


can we generate this estimated_avg_size when creating this smart vector? this value should be the same for every push. doing this once in initialization can save gas

we can take the size of the first element as estimated_avg_size. But will be very inaccurate for T with variable size. TBH I am not sure which is better.

wrwg

This looks well suited for specification.

wrwg · 2022-12-01T05:54:51Z

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

+    /// `SmartVector` should not enforece that.
+    struct SmartVector<T> has store {
+        inline_inner: vector<T>,
+        table_inner: vector<BigVector<T>>,


This (or something similar) may help to clarify:

spec struct SmartVector { invariant vector::length(inline_inner) + vector::length(table_inner) == 1; }

lightmark · 2022-12-03T05:24:00Z

@msmouse looks good to you now?

lightmark · 2022-12-03T05:24:57Z

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

+    public(friend) fun empty<T: store>(bucket_size: u64): BigVector<T> {
+        assert!(bucket_size > 0, error::invalid_argument(EZERO_BUCKET_SIZE));
+        BigVector {
+            buckets: table_with_length::new(),
+            end_index: 0,
+            num_buckets: 0,
+            bucket_size,
+        }
+    }
+
+    /// Create a vector of length 1 containing the passed in element.
+    public(friend) fun singleton<T: store>(e: T, bucket_size: u64): BigVector<T> {


I made these two friend only to make this struct private for friend modules. Is this the right way to do that? @movekevin

Why do these need to be friend? I.e. why can't these be open for anyone to use?

lightmark · 2022-12-07T00:03:05Z

move two files to move-examples first for quick adoption.

msmouse · 2022-12-07T20:06:01Z

aptos-move/move-examples/data_structures/sources/big_vector.move

+    const EVECTOR_NOT_EMPTY: u64 = 2;
+    /// bucket_size cannot be 0
+    const EZERO_BUCKET_SIZE: u64 = 3;



Now these are values that'll change for an existing module, is that a problem?

Oh.. I guess we didn't publish these example modules on-chain under aptos_std?

It's fine for these to change even if this had been in aptos-stdlib as well. The error mapping would automatically get re-generated when the module is upgraded.

aptos-move/move-examples/data_structures/sources/big_vector.move

msmouse · 2022-12-07T21:49:48Z

aptos-move/move-examples/data_structures/sources/big_vector.move

    struct BigVector<T> has store {
        buckets: TableWithLength<u64, vector<T>>,
-        end_index: BigVectorIndex,
+        end_index: u64,
        num_buckets: u64,


this is redundant because buckets knows it's length?

No, buckets know its length but this index is not bucket index but element index.

I mean num_buckets @lightmark

aptos-move/move-examples/data_structures/sources/big_vector.move

aptos-move/move-examples/data_structures/sources/smart_vector.move

movekevin · 2022-12-08T21:08:28Z

aptos-move/move-examples/data_structures/sources/big_vector.move

+            vector::swap(table_with_length::borrow_mut(&mut v.buckets, i_bucket_index), i_vector_index, j_vector_index);
+            return
+        };
+        let bucket_i = table_with_length::remove(&mut v.buckets, i_bucket_index);


Can you add some comments to explain what this block of code is doing?

davidiw

please remove the framework docs, maybe just a simple cargo build on cached-framework or something. dismiss review when done.

lightmark · 2022-12-13T06:47:04Z

@davidiw done

thanks!

github-actions · 2022-12-17T02:02:26Z

✅ Forge suite `land_blocking` success on `fb2775e7addfde643fad83a048f5a688406a2eaa`

performance benchmark with full nodes : 6305 TPS, 6271 ms latency, 10800 ms p99 latency,(!) expired 940 out of 2693260 txns
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2022-12-17T02:07:22Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `fb2775e7addfde643fad83a048f5a688406a2eaa`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fb2775e7addfde643fad83a048f5a688406a2eaa (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7310 TPS, 5303 ms latency, 7500 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: fb2775e7addfde643fad83a048f5a688406a2eaa
compatibility::simple-validator-upgrade::single-validator-upgrade : 4574 TPS, 9095 ms latency, 11200 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: fb2775e7addfde643fad83a048f5a688406a2eaa
compatibility::simple-validator-upgrade::half-validator-upgrade : 4550 TPS, 8998 ms latency, 11900 ms p99 latency,no expired txns
4. upgrading second batch to new version: fb2775e7addfde643fad83a048f5a688406a2eaa
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6276 TPS, 6129 ms latency, 10500 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fb2775e7addfde643fad83a048f5a688406a2eaa passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

lightmark requested review from davidiw, movekevin and wrwg as code owners November 23, 2022 08:44

lightmark requested review from 0xchloe, areshand and bowenyang007 November 23, 2022 08:45

lightmark force-pushed the smart_vector branch 2 times, most recently from b21661a to 22aa836 Compare November 28, 2022 23:40

msmouse approved these changes Nov 29, 2022

View reviewed changes

msmouse requested review from msmouse and removed request for msmouse November 29, 2022 01:23

areshand reviewed Nov 29, 2022

View reviewed changes

aptos-move/framework/aptos-stdlib/sources/big_vector.move Outdated Show resolved Hide resolved

areshand reviewed Nov 29, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch from 22aa836 to 3093db5 Compare November 29, 2022 07:36

movekevin reviewed Nov 29, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch from 3093db5 to d2bd4a6 Compare November 30, 2022 04:48

msmouse reviewed Nov 30, 2022

View reviewed changes

areshand reviewed Nov 30, 2022

View reviewed changes

wrwg reviewed Dec 1, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch 2 times, most recently from f74d65e to f7320eb Compare December 3, 2022 02:59

lightmark commented Dec 3, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch from f7320eb to 3506067 Compare December 6, 2022 09:41

msmouse reviewed Dec 7, 2022

View reviewed changes

movekevin approved these changes Dec 8, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch from 3506067 to b61b520 Compare December 8, 2022 23:28

davidiw previously requested changes Dec 11, 2022

View reviewed changes

lightmark force-pushed the smart_vector branch from b61b520 to d887486 Compare December 13, 2022 06:46

lightmark force-pushed the smart_vector branch 2 times, most recently from 10577c9 to 81e076e Compare December 14, 2022 14:44

lightmark enabled auto-merge (squash) December 15, 2022 05:02

lightmark force-pushed the smart_vector branch from 81e076e to bc2db41 Compare December 15, 2022 12:34

[framework] smart_vector

fb2775e

lightmark force-pushed the smart_vector branch from bc2db41 to fb2775e Compare December 17, 2022 01:34

This comment has been minimized.

Sign in to view

lightmark merged commit 717a8c2 into main Dec 17, 2022

lightmark deleted the smart_vector branch December 17, 2022 02:07

Markuze mentioned this pull request Dec 26, 2022

markuze/aptos perf #5998

Closed

Markuze mentioned this pull request Jan 3, 2023

[Network] Adding AptosPerf Client #6054

Closed

lightmark mentioned this pull request Mar 8, 2023

[AIP-18][Discussion] Introducing SmartVector and SmartTable to aptos_std aptos-foundation/AIPs#82

Closed

[framework] smart_vector and big_vector #5690

[framework] smart_vector and big_vector #5690

Conversation

lightmark commented Nov 23, 2022

Description

Test Plan

msmouse commented Nov 28, 2022

msmouse commented Nov 29, 2022

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

movekevin 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

wrwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Dec 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Dec 7, 2022

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

msmouse Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

lightmark commented Dec 13, 2022

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 17, 2022

✅ Forge suite land_blocking success on fb2775e7addfde643fad83a048f5a688406a2eaa

github-actions bot commented Dec 17, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> fb2775e7addfde643fad83a048f5a688406a2eaa

msmouse Dec 13, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `fb2775e7addfde643fad83a048f5a688406a2eaa`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `fb2775e7addfde643fad83a048f5a688406a2eaa`