[move] Implement generic comparison method in move cmp::compare #14714

igor-aptos · 2024-09-20T23:18:46Z

Description

Implement generic comparison operator for any move type.
This enables writing generic methods that require comparison - for example binary_search on a vector.

Since move doesn't have signed integers, comparison return value is not intuitive (increased by 1 from usual -1, 0, 1), and so is wrapped into Ordering struct - so users don't need to think about it (with move 2, calling those will be pretty intuitive)

Type of Change

New feature

Which Components or Systems Does This Change Impact?

Move/Aptos Virtual Machine

How Has This Been Tested?

Provided unit tests

Key Areas to Review

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-20T23:18:49Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-unit-coverage	15m	🟩
rust-move-unit-coverage	15m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-tests	10m	🟥
rust-move-tests	10m	🟥
rust-move-tests	10m	🟥
rust-move-tests	9m	🟥
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩
general-lints	6m	🟩 🟩 🟩 🟩
rust-move-unit-coverage	3m	🟥
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	54s	🟩 🟩 🟩 🟩
file_change_determinator	34s	🟩 🟩 🟩
permission-check	16s	🟩 🟩 🟩 🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
general-lints	25s	2m

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

codecov · 2024-09-20T23:35:03Z

Codecov Report

Attention: Patch coverage is 4.57516% with 146 lines in your changes missing coverage. Please review.

Please upload report for BASE (igor/enable_move_2_for_framework@7a83ca1). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...party/move/move-vm/types/src/values/values_impl.rs	0.0%	117 Missing ⚠️
...ptos-move/framework/move-stdlib/src/natives/cmp.rs	22.5%	24 Missing ⚠️
aptos-move/aptos-native-interface/src/context.rs	0.0%	5 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                         @@
##             igor/enable_move_2_for_framework   #14714   +/-   ##
===================================================================
  Coverage                                    ?    57.4%           
===================================================================
  Files                                       ?      859           
  Lines                                       ?   211663           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?   121527           
  Misses                                      ?    90136           
  Partials                                    ?        0

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

runtian-zhou · 2024-09-23T15:28:33Z

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

+    /// 2   iff first value is larger than the second
+    native fun compare_impl<T>(first: &T, second: &T): u8;
+
+    struct Ordering has copy, drop {


Can we use enum for this now? cc @wrwg

would enum be better here? Would enums be more inefficient here?

how would the native function return an enum that is defined in the module here?

runtian-zhou · 2024-09-23T15:30:53Z

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

+    }
+
+    // #[test]
+    // fun test_compare_enums() {


Why is enum ignored?

enum's are not yet enabled in the framework.

ziaptos · 2024-09-23T22:35:25Z

third_party/move/move-vm/types/src/values/values_impl.rs

+                }
+
+                let len_cmp = l.len().cmp(&r.len());
+                if len_cmp.is_ne() {


Just l.len().cmp(&r.len())

ziaptos · 2024-09-24T00:17:17Z

third_party/move/move-vm/types/src/values/values_impl.rs

+            (VecBool(r1), VecBool(r2)) => r1.borrow()[self.idx].cmp(&r2.borrow()[other.idx]),
+            (VecAddress(r1), VecAddress(r2)) => r1.borrow()[self.idx].cmp(&r2.borrow()[other.idx]),
+
+            // Equality between a generic and a specialized container.


Not equality

msmouse · 2024-09-25T20:57:12Z

third_party/move/move-vm/types/src/values/values_impl.rs

+            | (IndexedRef(_), _)
+            | (DelayedFieldID { .. }, _) => {
+                return Err(PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR)
+                    .with_message(format!("cannot compare values: {:?}, {:?}", self, other)))


Shall we use Display instead of Debug in the message (hopefully that means more stable error message)?

this is copied from equals. I am not sure what is more stable from MoveVM prespective, but seems like this should be the same as in equals.

and changing equals would require feature flag? if we wanted to change eeverything to {} , we should do that separately, I think

okay.. putting Debug text to messages we dare not to change generally makes me very sad but you don't make it worse I guess

@vgao1996 for thoughts

The error message of VMError is (more or less unfortunately, making it hard to show to users) not stored on chain, so I do not think it is very important to be stable.

msmouse · 2024-09-25T20:58:22Z

third_party/move/move-vm/types/src/values/values_impl.rs

+            | (VecAddress(_), _) => {
+                return Err(
+                    PartialVMError::new(StatusCode::INTERNAL_TYPE_ERROR).with_message(format!(
+                        "cannot compare container values: {:?}, {:?}",


lightmark · 2024-09-28T09:44:11Z

aptos-move/aptos-gas-schedule/src/gas_schedule/move_stdlib.rs

@@ -36,5 +39,8 @@ crate::gas_schedule::macros::define_gas_parameters!(
        [bcs_serialized_size_base: InternalGas, { RELEASE_V1_18.. => "bcs.serialized_size.base" }, 735],
        [bcs_serialized_size_per_byte_serialized: InternalGasPerByte, { RELEASE_V1_18.. => "bcs.serialized_size.per_byte_serialized" }, 36],
        [bcs_serialized_size_failure: InternalGas, { RELEASE_V1_18.. => "bcs.serialized_size.failure" }, 3676],
+
+        [cmp_compare_base: InternalGas, { RELEASE_V1_21.. => "cmp.base" }, 367],


how do we get 367?

copied from equality costs

lightmark · 2024-09-28T09:44:37Z

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

+    const EQUAL: u8 = 1;
+    const LESS_THAN: u8 = 0;
+    const GREATER_THAN: u8 = 2;


why not enum?

lightmark · 2024-09-28T09:45:49Z

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

+    struct Ordering has copy, drop {
+        value: u8,
+    }


less_than, equal, greater_than can be ordering and return value. Why do we choose u8 instead?

igor-aptos · 2024-10-03T22:25:10Z

I cannot use move 2 features (i.e. enums), until framework is compiled with it.

I can modify it in additional commit in the stack, and decide when landing.

runtian-zhou · 2024-11-12T18:12:01Z

third_party/move/move-vm/types/src/values/values_impl.rs

+    fn compare(&self, other: &Self) -> PartialVMResult<Ordering> {
+        use Container::*;
+
+        let res = match (


Was wondering if the logic would be easier if you do a read_ref on both ends and compare?

not sure, why equals doesn't do that? maybe perf?

runtian-zhou · 2024-11-12T18:13:07Z

third_party/move/move-vm/types/src/values/values_impl.rs

+            (Bool(l), Bool(r)) => l.cmp(r),
+            (Address(l), Address(r)) => l.cmp(r),
+
+            (Container(l), Container(r)) => l.compare(r)?,


Can a Container be compared with IndexedRef etc?

no, as equals doesn't have those either.

runtian-zhou · 2024-11-12T18:13:57Z

The value logic is indeed very redundent. Let's try to blow that logic as soon as possible... @ziaptos

igor-aptos · 2024-11-12T18:15:37Z

@runtian-zhou do you mean 100% code coverage with move unit tests, or something else?

For the complexity of code - code (i.e. all the matches) have been copied 1-1 from equals, so they should cover all the cases exactly.

signature of this move function uses enums, so I cannot land the move file with private function, until move 2 is enabled for the framework. So I've separated them out - but will confirm all tests in #15245 pass, before I land this one.

ziaptos

I've reviewed this before, but didn't stamp

github-actions · 2024-11-13T21:40:47Z

✅ Forge suite `realistic_env_max_load` success on `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`

two traffics test: inner traffic : committed: 14643.66 txn/s, latency: 2715.71 ms, (p50: 2700 ms, p70: 2700, p90: 2700 ms, p99: 3000 ms), latency samples: 5567900
two traffics test : committed: 100.06 txn/s, latency: 1483.54 ms, (p50: 1400 ms, p70: 1400, p90: 1600 ms, p99: 10100 ms), latency samples: 1740
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.977, avg: 1.525", "ConsensusProposalToOrdered: max: 0.315, avg: 0.290", "ConsensusOrderedToCommit: max: 0.356, avg: 0.347", "ConsensusProposalToCommit: max: 0.644, avg: 0.637"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.84s no progress at version 2926701 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.60s no progress at version 2926699 (avg 8.60s) [limit 15].
Test Ok

github-actions · 2024-11-13T21:42:16Z

✅ Forge suite `framework_upgrade` success on `ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70` ==> `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`

Compatibility test results for ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931 (PR)
Upgrade the nodes to version: 73c4d4d881346b17d2dcb3c50ae6b6fb42164931
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1323.62 txn/s, submitted: 1327.13 txn/s, failed submission: 3.51 txn/s, expired: 3.51 txn/s, latency: 2183.10 ms, (p50: 1800 ms, p70: 2100, p90: 3600 ms, p99: 6000 ms), latency samples: 120580
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1281.09 txn/s, submitted: 1283.77 txn/s, failed submission: 2.67 txn/s, expired: 2.67 txn/s, latency: 2330.38 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 5100 ms), latency samples: 115100
5. check swarm health
Compatibility test for ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931 passed
Upgrade the remaining nodes to version: 73c4d4d881346b17d2dcb3c50ae6b6fb42164931
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1453.67 txn/s, submitted: 1457.30 txn/s, failed submission: 3.63 txn/s, expired: 3.63 txn/s, latency: 2134.85 ms, (p50: 1800 ms, p70: 2100, p90: 3300 ms, p99: 4900 ms), latency samples: 128200
Test Ok

github-actions · 2024-11-13T21:42:44Z

✅ Forge suite `compat` success on `ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70` ==> `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`

Compatibility test results for ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931 (PR)
1. Check liveness of validators at old version: ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70
compatibility::simple-validator-upgrade::liveness-check : committed: 16477.36 txn/s, latency: 2078.92 ms, (p50: 1900 ms, p70: 2000, p90: 2200 ms, p99: 5400 ms), latency samples: 526320
2. Upgrading first Validator to new version: 73c4d4d881346b17d2dcb3c50ae6b6fb42164931
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7204.00 txn/s, latency: 3762.68 ms, (p50: 4000 ms, p70: 4700, p90: 5300 ms, p99: 5400 ms), latency samples: 129160
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7651.58 txn/s, latency: 4186.30 ms, (p50: 4300 ms, p70: 4400, p90: 6200 ms, p99: 6400 ms), latency samples: 252500
3. Upgrading rest of first batch to new version: 73c4d4d881346b17d2dcb3c50ae6b6fb42164931
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7764.18 txn/s, latency: 3630.67 ms, (p50: 4100 ms, p70: 4300, p90: 4400 ms, p99: 4500 ms), latency samples: 142120
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7785.42 txn/s, latency: 4109.84 ms, (p50: 4300 ms, p70: 4500, p90: 5500 ms, p99: 5800 ms), latency samples: 256480
4. upgrading second batch to new version: 73c4d4d881346b17d2dcb3c50ae6b6fb42164931
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 13262.18 txn/s, latency: 2066.40 ms, (p50: 2200 ms, p70: 2400, p90: 2600 ms, p99: 2700 ms), latency samples: 228840
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11454.86 txn/s, latency: 2715.16 ms, (p50: 2200 ms, p70: 2600, p90: 5500 ms, p99: 7600 ms), latency samples: 371080
5. check swarm health
Compatibility test for ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931 passed
Test Ok

igor-aptos requested review from runtian-zhou, vgao1996, wrwg and ziaptos September 20, 2024 23:18

igor-aptos requested review from davidiw and movekevin as code owners September 20, 2024 23:18

runtian-zhou reviewed Sep 23, 2024

View reviewed changes

ziaptos reviewed Sep 24, 2024

View reviewed changes

msmouse reviewed Sep 25, 2024

View reviewed changes

lightmark reviewed Sep 28, 2024

View reviewed changes

igor-aptos force-pushed the igor/native_compare branch from a2a930b to b024117 Compare October 3, 2024 22:14

igor-aptos requested review from zekun000 and georgemitenkov as code owners October 3, 2024 22:14

igor-aptos changed the base branch from main to igor/use_native_vector_move_range October 3, 2024 22:15

igor-aptos force-pushed the igor/use_native_vector_move_range branch from 0df908c to 0fab436 Compare October 4, 2024 17:00

igor-aptos force-pushed the igor/native_compare branch from b024117 to b508022 Compare October 4, 2024 17:01

igor-aptos force-pushed the igor/use_native_vector_move_range branch from 0fab436 to b1a2e70 Compare October 4, 2024 22:36

igor-aptos force-pushed the igor/native_compare branch from b508022 to 18ba714 Compare October 4, 2024 22:36

igor-aptos force-pushed the igor/use_native_vector_move_range branch from b1a2e70 to 6b493cd Compare October 8, 2024 19:00

igor-aptos force-pushed the igor/native_compare branch from 18ba714 to b0d9226 Compare October 8, 2024 19:00

igor-aptos force-pushed the igor/use_native_vector_move_range branch from 6b493cd to 6046016 Compare October 9, 2024 20:31

igor-aptos requested a review from grao1991 as a code owner October 9, 2024 20:31

igor-aptos force-pushed the igor/native_compare branch 2 times, most recently from cef8611 to df51e9c Compare October 9, 2024 22:50

igor-aptos changed the base branch from igor/use_native_vector_move_range to igor/enable_move_2_for_framework October 9, 2024 23:10

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from 5394fb5 to 927268c Compare October 10, 2024 00:08

runtian-zhou reviewed Nov 12, 2024

View reviewed changes

runtian-zhou approved these changes Nov 12, 2024

View reviewed changes

igor-aptos force-pushed the igor/native_vector_move_range branch from 9e0fd82 to db496b3 Compare November 12, 2024 18:20

igor-aptos force-pushed the igor/native_compare branch from 37384ce to 6bd7875 Compare November 12, 2024 18:20

ziaptos approved these changes Nov 13, 2024

View reviewed changes

igor-aptos force-pushed the igor/native_vector_move_range branch from db496b3 to 43449cb Compare November 13, 2024 19:04

igor-aptos force-pushed the igor/native_compare branch 2 times, most recently from 7f9c9c7 to c721045 Compare November 13, 2024 19:33

igor-aptos changed the base branch from igor/native_vector_move_range to main November 13, 2024 19:33

igor-aptos enabled auto-merge (squash) November 13, 2024 19:37

igor-aptos force-pushed the igor/native_compare branch from c721045 to 3144f47 Compare November 13, 2024 19:50

This comment has been minimized.

Sign in to view

[move] Implement generic comparison method in move - rust part

73c4d4d

igor-aptos force-pushed the igor/native_compare branch from 3144f47 to 73c4d4d Compare November 13, 2024 21:13

This comment has been minimized.

Sign in to view

igor-aptos merged commit 77679db into main Nov 13, 2024
48 checks passed

igor-aptos deleted the igor/native_compare branch November 13, 2024 21:46

[move] Implement generic comparison method in move cmp::compare #14714

[move] Implement generic comparison method in move cmp::compare #14714

Conversation

igor-aptos commented Sep 20, 2024

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

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

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

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

msmouse Sep 25, 2024 • edited Loading

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

lightmark Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

igor-aptos commented Oct 3, 2024

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

runtian-zhou commented Nov 12, 2024

igor-aptos commented Nov 12, 2024

ziaptos 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 13, 2024

✅ Forge suite realistic_env_max_load success on 73c4d4d881346b17d2dcb3c50ae6b6fb42164931

github-actions bot commented Nov 13, 2024

✅ Forge suite framework_upgrade success on ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931

github-actions bot commented Nov 13, 2024

✅ Forge suite compat success on ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70 ==> 73c4d4d881346b17d2dcb3c50ae6b6fb42164931

trunk-io bot commented Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

msmouse Sep 25, 2024 •

edited

Loading

lightmark Sep 28, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`

✅ Forge suite `framework_upgrade` success on `ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70` ==> `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`

✅ Forge suite `compat` success on `ea6e45f0eee4b6da2ebf93b9b89d269d334fcf70` ==> `73c4d4d881346b17d2dcb3c50ae6b6fb42164931`