[move-stdlib] Introduce efficient OrderedMap implementation #14872

igor-aptos · 2024-10-04T18:01:26Z

Description

Implement efficient OrderedMap implementation in move-stdlib, to replace/deprecate SimpleMap. It is an in-memory datastructure - i.e. doesn't store anything in separate resources.

Implementation is a SortedVectorMap, but given the efficiency improvements of vector operations with memcpy, it's performance characteristics are extremely good.

Running different sizes tests (these are full results), we get milliseconds needed for 100 inserts and 100 removals on maps of different sizes:

num elements  SimpleMap       OrderedMap::SortedVectorMap
10            6.4             6.8
100           25.4            10.3
1000          673.9           12.8
10000         too long        19.5

Basically achieving log(n) performance, with very simple implementation.

How Has This Been Tested?

provided unit tests

Key Areas to Review

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-10-04T18:01:30Z

⏱️ 4h 33m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 41m	⬜ 🟥 🟥 🟥 🟥
execution-performance / test-target-determinator	17m	🟩 🟩 🟩 🟩 🟩
rust-move-unit-coverage	16m	🟩
rust-move-unit-coverage	14m	🟩
test-target-determinator	14m	🟩 🟩 🟩 🟩
rust-move-unit-coverage	14m	🟩
rust-cargo-deny	14m	🟩 🟩 🟩 🟩 🟩 (+3 more)
rust-move-unit-coverage	11m	🟩
rust-move-unit-coverage	10m	🟩
check-dynamic-deps	10m	🟩 🟩 🟩 🟩 🟩 (+5 more)
rust-move-unit-coverage	9m	🟩
rust-move-tests	4m	🟥
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+5 more)
rust-move-tests	4m	🟥
rust-move-tests	4m	🟥

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
execution-performance / single-node-performance	37m	16m
test-target-determinator	3m	4m

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

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

codecov · 2024-10-04T18:12:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@                 Coverage Diff                  @@
##           igor/native_compare   #14872   +/-   ##
====================================================
  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/ordered_map.move

lightmark · 2024-10-04T18:34:39Z

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

+            return;
+        };
+
+        // TODO: can be implemented more efficiently, as we know both maps are already sorted.


https://leetcode.com/problems/merge-two-sorted-lists/

yeah, but issue is here we have vectors, not lists, and so how to do it in-place, without moving too many elements.

Easiest is to can either create a new vector, populate, and then swap (I'll probably just do that)

if "vector" here had capacity, I could start from the end, without temporary storage, but I don't have a way to create empty values here (i.e. have this work without value being drop+copy)

lightmark · 2024-10-04T18:41:01Z

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

+            f(iter.iter_borrow_key(self), iter.iter_borrow(self));
+            iter = iter.iter_next(self);
+        }
+        // vector::for_each_ref(


what's wrong here?

entry is not a public type, so when this get's inlined in the caller, it cannot do compile.

basically inline functions need to be implemented only using public API.

I can remove this snippet as well

lightmark · 2024-10-04T18:42:04Z

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

+
+    // return index containing the key, or insert position.
+    // I.e. index of first element that has key larger or equal to the passed `key` argument.
+    fun binary_search<K, V>(key: &K, entries: &vector<Entry<K, V>>, start: u64, end: u64): u64 {


Can we make a standard Entry and implement binary search only once?

I am writing binary search only once.

If you mean for binary search to be reusable, so other's don't need to write it, maybe that should be an inline function in vector

Moving Entry to a standard separate file, makes all field accesses to require getter and setter functions, not sure that's best

lightmark

amazing work! Thanks @igor-aptos . You are the hero!

github-actions · 2025-01-11T07:47:40Z

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `b60e33676dc0df7baf8ee2a84919684772e50e03`

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> b60e33676dc0df7baf8ee2a84919684772e50e03 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 16065.83 txn/s, latency: 2151.52 ms, (p50: 2000 ms, p70: 2100, p90: 3000 ms, p99: 6700 ms), latency samples: 546360
2. Upgrading first Validator to new version: b60e33676dc0df7baf8ee2a84919684772e50e03
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7748.23 txn/s, latency: 3846.59 ms, (p50: 4400 ms, p70: 4700, p90: 4800 ms, p99: 4900 ms), latency samples: 143900
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7485.44 txn/s, latency: 4405.20 ms, (p50: 4800 ms, p70: 4900, p90: 4900 ms, p99: 5000 ms), latency samples: 257060
3. Upgrading rest of first batch to new version: b60e33676dc0df7baf8ee2a84919684772e50e03
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6082.19 txn/s, latency: 4990.95 ms, (p50: 5800 ms, p70: 6200, p90: 6300 ms, p99: 6400 ms), latency samples: 115360
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5688.22 txn/s, latency: 5921.68 ms, (p50: 6500 ms, p70: 6600, p90: 6700 ms, p99: 6800 ms), latency samples: 196460
4. upgrading second batch to new version: b60e33676dc0df7baf8ee2a84919684772e50e03
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11672.33 txn/s, latency: 2494.77 ms, (p50: 2800 ms, p70: 2900, p90: 3200 ms, p99: 3400 ms), latency samples: 204560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11702.27 txn/s, latency: 2797.56 ms, (p50: 2900 ms, p70: 3000, p90: 3100 ms, p99: 3300 ms), latency samples: 383040
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> b60e33676dc0df7baf8ee2a84919684772e50e03 passed
Test Ok

github-actions · 2025-01-11T07:49:13Z

✅ Forge suite `realistic_env_max_load` success on `b60e33676dc0df7baf8ee2a84919684772e50e03`

two traffics test: inner traffic : committed: 16336.40 txn/s, latency: 2432.18 ms, (p50: 2400 ms, p70: 2400, p90: 2700 ms, p99: 3000 ms), latency samples: 6211420
two traffics test : committed: 99.97 txn/s, latency: 1326.14 ms, (p50: 1200 ms, p70: 1300, p90: 1500 ms, p99: 2700 ms), latency samples: 1820
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.345, avg: 1.246", "ConsensusProposalToOrdered: max: 0.266, avg: 0.263", "ConsensusOrderedToCommit: max: 0.288, avg: 0.279", "ConsensusProposalToCommit: max: 0.553, avg: 0.541"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.49s no progress at version 481 (avg 0.17s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.47s no progress at version 2584029 (avg 0.47s) [limit 16].
Test Ok

igor-aptos requested review from davidiw, lightmark, msmouse and wrwg October 4, 2024 18:01

igor-aptos requested a review from movekevin as a code owner October 4, 2024 18:01

igor-aptos requested a review from grao1991 October 4, 2024 18:01