[framework] Adding utilities to OrderedMap / BigOrderedMap #15810

igor-aptos · 2025-01-24T21:13:48Z

making traversal APIs complete and public
adding ordering-based utilities, without iterator functionality

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

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 · 2025-01-24T21:13:51Z

⏱️ 10m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
check-dynamic-deps	4m	🟩 🟩
rust-cargo-deny	4m	🟩 🟩
general-lints	1m	🟩 🟩
semgrep/ci	1m	🟩 🟩
file_change_determinator	20s	🟩 🟩
permission-check	4s	🟩 🟩
permission-check	4s	🟩 🟩

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

igor-aptos · 2025-01-24T21:20:10Z

aptos-move/framework/aptos-framework/sources/datastructures/big_ordered_map.move

+        // Use new_with_type_size_hints or new_with_config if your types have variable sizes.
        assert!(
            bcs::constant_serialized_size<K>().is_some() && bcs::constant_serialized_size<V>().is_some(),
            error::invalid_argument(EINVALID_CONFIG_PARAMETER)


not sure if it is better to allow new() for all types - and then if first values inserted are not representative - by a factor of 100x+ - not being able to insert bigger values later.
or as implemented here - be conservative - and ask people to intentionally provide or not provide size hints.

igor-aptos · 2025-01-24T21:20:46Z

aptos-move/framework/aptos-framework/sources/datastructures/big_ordered_map.move

+
+    /// Returns the number of elements in the BigOrderedMap.
+    /// This is an expensive function, as it goes through all the leaves to compute it.
+    public fun compute_length<K: store, V: store>(self: &BigOrderedMap<K, V>): u64 {


intentionally not calling length() because it is expensive to call. Is that reasonable?

runtian-zhou · 2025-01-31T21:13:28Z

aptos-move/framework/aptos-framework/sources/datastructures/ordered_map.move

@@ -349,7 +397,7 @@ module aptos_std::ordered_map {
    }

    /// Returns whether the iterator is a begin iterator.
-    public fun iter_is_begin<K, V>(self: &IteratorPtr, map: &OrderedMap<K, V>): bool {
+    public(friend) fun iter_is_begin<K, V>(self: &IteratorPtr, map: &OrderedMap<K, V>): bool {


Is this intentional?

yes! I've patched this in 1.26 already. iter* should all be public friend, until references are available in move

runtian-zhou

Otherwise LGTM

making traversal APIs complete and public adding ordering-based utilities, without iterator functionality

github-actions · 2025-02-03T23:14:25Z

✅ Forge suite `compat` success on `60f7ca8827f5d64a148c3b163dc4126b0879279b` ==> `53c9d4e84587f8f794124e104cf24aa082dbea11`

Compatibility test results for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11 (PR)
1. Check liveness of validators at old version: 60f7ca8827f5d64a148c3b163dc4126b0879279b
compatibility::simple-validator-upgrade::liveness-check : committed: 6371.69 txn/s, submitted: 6371.86 txn/s, expired: 0.17 txn/s, latency: 2490.89 ms, (p50: 2500 ms, p70: 2700, p90: 2900 ms, p99: 3200 ms), latency samples: 410589
2. Upgrading first Validator to new version: 53c9d4e84587f8f794124e104cf24aa082dbea11
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3342.89 txn/s, latency: 8833.59 ms, (p50: 9400 ms, p70: 10600, p90: 11300 ms, p99: 11500 ms), latency samples: 74280
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3335.73 txn/s, latency: 10343.01 ms, (p50: 11300 ms, p70: 11600, p90: 11600 ms, p99: 11900 ms), latency samples: 127560
3. Upgrading rest of first batch to new version: 53c9d4e84587f8f794124e104cf24aa082dbea11
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3344.56 txn/s, latency: 8828.83 ms, (p50: 9600 ms, p70: 10800, p90: 11600 ms, p99: 11700 ms), latency samples: 73540
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3352.53 txn/s, latency: 10275.95 ms, (p50: 11300 ms, p70: 11600, p90: 11900 ms, p99: 11900 ms), latency samples: 127500
4. upgrading second batch to new version: 53c9d4e84587f8f794124e104cf24aa082dbea11
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 6337.80 txn/s, latency: 4811.84 ms, (p50: 5600 ms, p70: 6000, p90: 6100 ms, p99: 6200 ms), latency samples: 117940
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6223.03 txn/s, latency: 5542.85 ms, (p50: 6000 ms, p70: 6000, p90: 6100 ms, p99: 6300 ms), latency samples: 217040
5. check swarm health
Compatibility test for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11 passed
Test Ok

github-actions · 2025-02-03T23:15:09Z

✅ Forge suite `framework_upgrade` success on `60f7ca8827f5d64a148c3b163dc4126b0879279b` ==> `53c9d4e84587f8f794124e104cf24aa082dbea11`

Compatibility test results for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11 (PR)
Upgrade the nodes to version: 53c9d4e84587f8f794124e104cf24aa082dbea11
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1620.51 txn/s, submitted: 1625.80 txn/s, failed submission: 5.29 txn/s, expired: 5.29 txn/s, latency: 1779.38 ms, (p50: 1800 ms, p70: 2100, p90: 2500 ms, p99: 3700 ms), latency samples: 147020
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1552.42 txn/s, submitted: 1556.63 txn/s, failed submission: 4.21 txn/s, expired: 4.21 txn/s, latency: 1859.87 ms, (p50: 1600 ms, p70: 2000, p90: 3100 ms, p99: 4600 ms), latency samples: 140240
5. check swarm health
Compatibility test for 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11 passed
Upgrade the remaining nodes to version: 53c9d4e84587f8f794124e104cf24aa082dbea11
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1750.71 txn/s, submitted: 1754.74 txn/s, failed submission: 4.03 txn/s, expired: 4.03 txn/s, latency: 1751.43 ms, (p50: 1500 ms, p70: 1900, p90: 2800 ms, p99: 4500 ms), latency samples: 156360
Test Ok

github-actions · 2025-02-03T23:29:19Z

✅ Forge suite `realistic_env_max_load` success on `53c9d4e84587f8f794124e104cf24aa082dbea11`

two traffics test: inner traffic : committed: 13106.47 txn/s, submitted: 13108.05 txn/s, expired: 1.58 txn/s, latency: 3020.74 ms, (p50: 3000 ms, p70: 3000, p90: 3300 ms, p99: 3900 ms), latency samples: 4983360
two traffics test : committed: 99.96 txn/s, latency: 2134.95 ms, (p50: 2100 ms, p70: 2500, p90: 2800 ms, p99: 3700 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.316, avg: 0.961", "ConsensusProposalToOrdered: max: 0.326, avg: 0.316", "ConsensusOrderedToCommit: max: 0.962, avg: 0.639", "ConsensusProposalToCommit: max: 1.277, avg: 0.955"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.15s no progress at version 45190 (avg 0.22s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.78s no progress at version 2437325 (avg 0.78s) [limit 16].
Test Ok

igor-aptos requested review from lightmark, runtian-zhou and wrwg January 24, 2025 21:13

igor-aptos requested review from davidiw and movekevin as code owners January 24, 2025 21:13

igor-aptos commented Jan 24, 2025

View reviewed changes

igor-aptos force-pushed the igor/adding_utilities_to_maps branch from b12b063 to 9dbe637 Compare January 27, 2025 16:58

runtian-zhou reviewed Jan 31, 2025

View reviewed changes

runtian-zhou approved these changes Jan 31, 2025

View reviewed changes

igor-aptos force-pushed the igor/adding_utilities_to_maps branch from 9dbe637 to 8b276ea Compare January 31, 2025 21:57

wrwg approved these changes Feb 1, 2025

View reviewed changes

igor-aptos force-pushed the igor/adding_utilities_to_maps branch from 8b276ea to 9372017 Compare February 3, 2025 06:38

[framework] Adding utilities to OrderedMap / BigOrderedMap

53c9d4e

making traversal APIs complete and public adding ordering-based utilities, without iterator functionality

igor-aptos force-pushed the igor/adding_utilities_to_maps branch from 9372017 to 53c9d4e Compare February 3, 2025 21:59

igor-aptos enabled auto-merge (squash) February 3, 2025 22:46

This comment has been minimized.

Sign in to view

igor-aptos merged commit 4eaf379 into main Feb 3, 2025
88 checks passed

igor-aptos deleted the igor/adding_utilities_to_maps branch February 3, 2025 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[framework] Adding utilities to OrderedMap / BigOrderedMap #15810

[framework] Adding utilities to OrderedMap / BigOrderedMap #15810

igor-aptos commented Jan 24, 2025

trunk-io bot commented Jan 24, 2025 •

edited

Loading

igor-aptos Jan 24, 2025

igor-aptos Jan 24, 2025 •

edited

Loading

runtian-zhou Jan 31, 2025

igor-aptos Jan 31, 2025

runtian-zhou left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 3, 2025

github-actions bot commented Feb 3, 2025

This comment has been minimized.

github-actions bot commented Feb 3, 2025

[framework] Adding utilities to OrderedMap / BigOrderedMap #15810

[framework] Adding utilities to OrderedMap / BigOrderedMap #15810

Conversation

igor-aptos commented Jan 24, 2025

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 Jan 24, 2025 • edited Loading

igor-aptos Jan 24, 2025

Choose a reason for hiding this comment

igor-aptos Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

runtian-zhou Jan 31, 2025

Choose a reason for hiding this comment

igor-aptos Jan 31, 2025

Choose a reason for hiding this comment

runtian-zhou left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 3, 2025

✅ Forge suite compat success on 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11

github-actions bot commented Feb 3, 2025

✅ Forge suite framework_upgrade success on 60f7ca8827f5d64a148c3b163dc4126b0879279b ==> 53c9d4e84587f8f794124e104cf24aa082dbea11

This comment has been minimized.

github-actions bot commented Feb 3, 2025

✅ Forge suite realistic_env_max_load success on 53c9d4e84587f8f794124e104cf24aa082dbea11

trunk-io bot commented Jan 24, 2025 •

edited

Loading

igor-aptos Jan 24, 2025 •

edited

Loading

✅ Forge suite `compat` success on `60f7ca8827f5d64a148c3b163dc4126b0879279b` ==> `53c9d4e84587f8f794124e104cf24aa082dbea11`

✅ Forge suite `framework_upgrade` success on `60f7ca8827f5d64a148c3b163dc4126b0879279b` ==> `53c9d4e84587f8f794124e104cf24aa082dbea11`

✅ Forge suite `realistic_env_max_load` success on `53c9d4e84587f8f794124e104cf24aa082dbea11`