[move] Implement generic comparison method in move - move part (cmp::compare) #15245

igor-aptos · 2024-11-08T23:29:46Z

Description

Separated out move part of the cmp::compare native call (first part is in the #14714), as it requires enums, and can only land after enums are enabled.

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 · 2024-11-08T23:29:49Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-targeted-unit-tests	11m	🟥
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩
rust-cargo-deny	7m	🟩 🟩 🟥 🟩 🟩
rust-move-tests	6m	🟥
rust-move-tests	6m	🟥
rust-move-tests	4m	🟥
rust-lints	4m	🟩
rust-lints	4m	🟩
rust-move-tests	3m	🟥
general-lints	2m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
rust-check-merge-base	2m	🟩
file_change_determinator	55s	🟩 🟩 🟩 🟩 🟩
rust-move-tests	33s	🟥
permission-check	18s	🟩 🟩 🟩 🟩 🟩

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

Job	Duration	vs 7d avg	Delta
check-dynamic-deps	4m	2m

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

georgemitenkov · 2024-11-13T23:40:26Z

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

+    ///   - enum's are compared first by their variant, and if equal - they are compared as structs are.
+    native public(friend) fun compare<T>(first: &T, second: &T): Ordering;
+
+    public fun is_eq(self: &Ordering): bool {


why not just eq, lt?

primarily to align with Rust naming - https://doc.rust-lang.org/std/cmp/enum.Ordering.html

but secondly, eq soudns much more like a binary operator (i.e. a.eq(b)), vs is_eq sounds like unary - compare(a, b).is_eq()

georgemitenkov · 2024-11-13T23:54:08Z

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

+    struct SomeStruct has drop {
+        field_1: u64,
+        field_2: u64,
+    }


nit: newline missing here

georgemitenkov · 2024-11-13T23:54:47Z

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

+    native public(friend) fun compare<T>(first: &T, second: &T): Ordering;
+
+    public fun is_eq(self: &Ordering): bool {
+        self is Ordering::Equal


can we have is_ne/ne? as well?

igor-aptos · 2024-11-19T20:43:14Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-01-11T00:44:28Z

✅ Forge suite `realistic_env_max_load` success on `b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5`

two traffics test: inner traffic : committed: 14983.38 txn/s, latency: 2652.82 ms, (p50: 2700 ms, p70: 2700, p90: 2800 ms, p99: 3000 ms), latency samples: 5697000
two traffics test : committed: 99.97 txn/s, latency: 1345.78 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 1600 ms), latency samples: 1720
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.565, avg: 1.531", "ConsensusProposalToOrdered: max: 0.288, avg: 0.285", "ConsensusOrderedToCommit: max: 0.300, avg: 0.293", "ConsensusProposalToCommit: max: 0.588, avg: 0.578"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.53s no progress at version 517 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.55s no progress at version 2326746 (avg 0.55s) [limit 16].
Test Ok

github-actions · 2025-01-11T00:45:31Z

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

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 16550.56 txn/s, latency: 2096.75 ms, (p50: 1700 ms, p70: 1800, p90: 3200 ms, p99: 12300 ms), latency samples: 548780
2. Upgrading first Validator to new version: b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7784.01 txn/s, latency: 3877.67 ms, (p50: 4400 ms, p70: 4700, p90: 4800 ms, p99: 4900 ms), latency samples: 142820
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7589.57 txn/s, latency: 4422.56 ms, (p50: 4800 ms, p70: 4900, p90: 4900 ms, p99: 5000 ms), latency samples: 253380
3. Upgrading rest of first batch to new version: b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7884.46 txn/s, latency: 3766.07 ms, (p50: 4400 ms, p70: 4600, p90: 4700 ms, p99: 4800 ms), latency samples: 146380
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7686.13 txn/s, latency: 4304.59 ms, (p50: 4600 ms, p70: 4800, p90: 4800 ms, p99: 4900 ms), latency samples: 261100
4. upgrading second batch to new version: b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11391.39 txn/s, latency: 2597.38 ms, (p50: 2800 ms, p70: 2900, p90: 3700 ms, p99: 3700 ms), latency samples: 199240
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5609.38 txn/s, submitted: 5609.54 txn/s, expired: 0.17 txn/s, latency: 2883.15 ms, (p50: 2800 ms, p70: 3300, p90: 3500 ms, p99: 3700 ms), latency samples: 373529
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5 passed
Test Ok

igor-aptos requested review from vgao1996, wrwg and ziaptos November 8, 2024 23:29

igor-aptos requested review from msmouse, lightmark, grao1991, davidiw and movekevin as code owners November 8, 2024 23:29

lightmark approved these changes Nov 8, 2024

View reviewed changes

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from 686d2de to a85b7e7 Compare November 9, 2024 00:18

igor-aptos force-pushed the igor/native_compare_move_part branch from 3e9fb56 to 52eebf4 Compare November 9, 2024 00:18

igor-aptos mentioned this pull request Nov 9, 2024

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

Merged

8 tasks

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from a85b7e7 to 2efd3d0 Compare November 12, 2024 18:04

igor-aptos force-pushed the igor/native_compare_move_part branch from 52eebf4 to 6d8744d Compare November 12, 2024 18:05

georgemitenkov approved these changes Nov 13, 2024

View reviewed changes

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from 2efd3d0 to 81dbdb6 Compare November 15, 2024 00:00

igor-aptos force-pushed the igor/native_compare_move_part branch from 6d8744d to 3c7a17d Compare November 15, 2024 00:00

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from 81dbdb6 to 83e27b8 Compare November 15, 2024 20:59

igor-aptos force-pushed the igor/native_compare_move_part branch from 3c7a17d to 7c84b5f Compare November 15, 2024 20:59

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from 83e27b8 to f72afc4 Compare November 19, 2024 20:42

igor-aptos force-pushed the igor/native_compare_move_part branch from 7c84b5f to 0ea0a79 Compare November 19, 2024 20:43

igor-aptos mentioned this pull request Nov 19, 2024

[move-stdlib] Use vector::move_range inside vector, and evaluate performance / calibrate gas #14862

Merged

8 tasks

igor-aptos force-pushed the igor/enable_move_2_for_framework branch from f72afc4 to 07bad73 Compare November 23, 2024 02:48

igor-aptos force-pushed the igor/native_compare_move_part branch from 0ea0a79 to 0ed1006 Compare November 23, 2024 02:48

igor-aptos requested review from gelash, zekun000, sasha8, danielxiangzl, junkil-park and 0xmaayan as code owners January 7, 2025 20:58

igor-aptos changed the base branch from igor/enable_move_2_for_framework to igor_copy/use-v2-as-default-in-rust-unit-tests January 7, 2025 20:58

igor-aptos mentioned this pull request Jan 7, 2025

[Bugfix][Compiler-V2] Fix public(package) causing unit test errors (#15627) #15684

Closed

22 tasks

igor-aptos force-pushed the igor_copy/use-v2-as-default-in-rust-unit-tests branch from c5bf1b0 to 7a63894 Compare January 10, 2025 06:40

igor-aptos force-pushed the igor/native_compare_move_part branch from 5e9556a to 7c1b9c7 Compare January 10, 2025 06:40

igor-aptos force-pushed the igor_copy/use-v2-as-default-in-rust-unit-tests branch from 7a63894 to eb4b7a4 Compare January 10, 2025 07:30

igor-aptos force-pushed the igor/native_compare_move_part branch from 7c1b9c7 to d6f7b1d Compare January 10, 2025 07:31

igor-aptos force-pushed the igor_copy/use-v2-as-default-in-rust-unit-tests branch from eb4b7a4 to f8b9b81 Compare January 10, 2025 08:34

igor-aptos force-pushed the igor/native_compare_move_part branch 2 times, most recently from ee38172 to 20d08b0 Compare January 10, 2025 22:40

igor-aptos changed the base branch from igor_copy/use-v2-as-default-in-rust-unit-tests to main January 10, 2025 22:40

igor-aptos force-pushed the igor/native_compare_move_part branch from 20d08b0 to 7093dad Compare January 10, 2025 23:08

igor-aptos enabled auto-merge (rebase) January 10, 2025 23:09

This comment has been minimized.

Sign in to view

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

b6298b6

igor-aptos force-pushed the igor/native_compare_move_part branch from 7093dad to b6298b6 Compare January 11, 2025 00:16

This comment has been minimized.

Sign in to view

igor-aptos merged commit f9334ca into main Jan 11, 2025
43 of 46 checks passed

igor-aptos deleted the igor/native_compare_move_part branch January 11, 2025 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move] Implement generic comparison method in move - move part (cmp::compare) #15245

[move] Implement generic comparison method in move - move part (cmp::compare) #15245

igor-aptos commented Nov 8, 2024

trunk-io bot commented Nov 8, 2024 •

edited

Loading

georgemitenkov Nov 13, 2024

igor-aptos Nov 15, 2024

georgemitenkov Nov 13, 2024

georgemitenkov Nov 13, 2024

igor-aptos commented Nov 19, 2024 •

edited

Loading

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 Jan 11, 2025

github-actions bot commented Jan 11, 2025

[move] Implement generic comparison method in move - move part (cmp::compare) #15245

[move] Implement generic comparison method in move - move part (cmp::compare) #15245

Conversation

igor-aptos commented Nov 8, 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 Nov 8, 2024 • edited Loading

georgemitenkov Nov 13, 2024

Choose a reason for hiding this comment

igor-aptos Nov 15, 2024

Choose a reason for hiding this comment

georgemitenkov Nov 13, 2024

Choose a reason for hiding this comment

georgemitenkov Nov 13, 2024

Choose a reason for hiding this comment

igor-aptos commented Nov 19, 2024 • edited Loading

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 Jan 11, 2025

✅ Forge suite realistic_env_max_load success on b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5

github-actions bot commented Jan 11, 2025

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5

trunk-io bot commented Nov 8, 2024 •

edited

Loading

igor-aptos commented Nov 19, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `b6298b6a1e154f26e5121c6ce89ad4d56aefeaf5`

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