[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984

igor-aptos · 2024-10-16T21:09:11Z

Description

It is sometimes useful to know if type has constants serialized size, and for example perform some optimization based on it.

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?

Move/Aptos Virtual Machine

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-16T21:09:14Z

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

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	15m	🟩
rust-move-tests	9m	🟥
rust-move-tests	9m	🟥
rust-move-unit-coverage	8m	⬜
rust-move-tests	8m	⬜
rust-cargo-deny	5m	🟩 🟩 🟩
check-dynamic-deps	3m	🟩 🟩 🟩
general-lints	1m	🟩 🟩 🟩
semgrep/ci	54s	🟩 🟩 🟩
file_change_determinator	40s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩
permission-check	7s	🟩 🟩 🟩

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

codecov · 2024-10-16T21:34:13Z

Codecov Report

Attention: Patch coverage is 1.81818% with 54 lines in your changes missing coverage. Please review.

Project coverage is 57.5%. Comparing base (a5af4dc) to head (a420b4c).

Files with missing lines	Patch %	Lines
third_party/move/move-vm/types/src/value_serde.rs	0.0%	34 Missing ⚠️
...ptos-move/framework/move-stdlib/src/natives/bcs.rs	4.7%	20 Missing ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           igor/use_native_vector_move_range   #14984     +/-   ##
====================================================================
- Coverage                               60.1%    57.5%   -2.7%     
====================================================================
  Files                                    858      858             
  Lines                                 211455   211510     +55     
====================================================================
- Hits                                  127237   121651   -5586     
- Misses                                 84218    89859   +5641

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

msmouse · 2024-10-16T21:54:38Z

third_party/move/move-vm/types/src/value_serde.rs

+            Ok(fields
+                .iter()
+                .map(|field| constant_serialized_size(&field.layout))
+                .collect::<Result<Option<Vec<_>>, _>>()


Can this potentially allocate too much? Shall we bother to write a loop or try_for_each()?

this is nice, but I agree - safer to not worry about it :)

github-actions · 2024-11-12T07:17:45Z

✅ Forge suite `realistic_env_max_load` success on `c268f4a86b05ae687941cba677591dd03cbb1629`

two traffics test: inner traffic : committed: 14230.25 txn/s, latency: 2815.82 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 12400 ms), latency samples: 5410640
two traffics test : committed: 99.98 txn/s, latency: 1683.97 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 10600 ms), latency samples: 1680
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.040, avg: 1.582", "ConsensusProposalToOrdered: max: 0.320, avg: 0.290", "ConsensusOrderedToCommit: max: 0.356, avg: 0.345", "ConsensusProposalToCommit: max: 0.647, avg: 0.635"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.99s no progress at version 28107 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.46s no progress at version 5836169 (avg 8.39s) [limit 15].
Test Ok

github-actions · 2024-11-12T07:21:06Z

✅ Forge suite `compat` success on `fcd2dedf6ca61a23f4979e944c629dcdbdae5dca` ==> `c268f4a86b05ae687941cba677591dd03cbb1629`

Compatibility test results for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> c268f4a86b05ae687941cba677591dd03cbb1629 (PR)
1. Check liveness of validators at old version: fcd2dedf6ca61a23f4979e944c629dcdbdae5dca
compatibility::simple-validator-upgrade::liveness-check : committed: 14838.84 txn/s, latency: 2307.60 ms, (p50: 1800 ms, p70: 1900, p90: 3300 ms, p99: 12100 ms), latency samples: 573600
2. Upgrading first Validator to new version: c268f4a86b05ae687941cba677591dd03cbb1629
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7662.72 txn/s, latency: 3767.01 ms, (p50: 4200 ms, p70: 4400, p90: 4500 ms, p99: 4500 ms), latency samples: 143040
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6926.15 txn/s, latency: 4613.84 ms, (p50: 4600 ms, p70: 4600, p90: 6900 ms, p99: 7100 ms), latency samples: 234340
3. Upgrading rest of first batch to new version: c268f4a86b05ae687941cba677591dd03cbb1629
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7394.74 txn/s, latency: 3649.27 ms, (p50: 4000 ms, p70: 4500, p90: 5000 ms, p99: 5100 ms), latency samples: 132300
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7592.18 txn/s, latency: 4200.39 ms, (p50: 4400 ms, p70: 4400, p90: 6200 ms, p99: 6500 ms), latency samples: 253100
4. upgrading second batch to new version: c268f4a86b05ae687941cba677591dd03cbb1629
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12343.49 txn/s, latency: 2254.45 ms, (p50: 2400 ms, p70: 2500, p90: 2600 ms, p99: 2800 ms), latency samples: 214260
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11687.58 txn/s, latency: 2676.50 ms, (p50: 2600 ms, p70: 2700, p90: 4500 ms, p99: 5500 ms), latency samples: 377940
5. check swarm health
Compatibility test for fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> c268f4a86b05ae687941cba677591dd03cbb1629 passed
Test Ok

runtian-zhou · 2024-11-12T18:16:35Z

third_party/move/move-vm/types/src/value_serde.rs

+        | MoveTypeLayout::U256
+        | MoveTypeLayout::U64
+        | MoveTypeLayout::Address
+        | MoveTypeLayout::Signer => 1,


Should we return None for signer as it shouldn't have been serialized?

this is for gas charging. below we do say signer doesn't have constant size.

runtian-zhou · 2024-11-12T18:16:38Z

third_party/move/move-vm/types/src/value_serde.rs

+        | MoveTypeLayout::U128
+        | MoveTypeLayout::U256
+        | MoveTypeLayout::U64
+        | MoveTypeLayout::Address


I suppose the size should be different?

this is type_visit_count for constant_serialized_size - i.e. amount needed to charge gas.

constant_serialized_size below is the actual computation

igor-aptos requested review from vgao1996 and georgemitenkov October 16, 2024 21:09

igor-aptos requested review from davidiw, movekevin and wrwg as code owners October 16, 2024 21:09

igor-aptos force-pushed the igor/use_native_vector_move_range branch from 3bcedb1 to a5af4dc Compare October 16, 2024 21:17

igor-aptos requested review from msmouse, lightmark, grao1991 and zekun000 as code owners October 16, 2024 21:17

igor-aptos force-pushed the igor/constant_serialized_size branch from 8a929c9 to a420b4c Compare October 16, 2024 21:17

msmouse reviewed Oct 16, 2024

View reviewed changes

igor-aptos force-pushed the igor/use_native_vector_move_range branch from a5af4dc to 9fcf365 Compare October 21, 2024 23:10

igor-aptos force-pushed the igor/constant_serialized_size branch from a420b4c to 4c0b5bc Compare October 21, 2024 23:10

igor-aptos requested a review from msmouse October 21, 2024 23:27

igor-aptos force-pushed the igor/constant_serialized_size branch from 4c0b5bc to 58152b4 Compare November 8, 2024 23:21

igor-aptos requested review from a team, JoshLind, gregnazario, banool, bchocho, sasha8, gelash, ibalajiarun, runtian-zhou, danielxiangzl and areshand as code owners November 8, 2024 23:21

igor-aptos removed request for davidiw, areshand, gregnazario, bchocho, alinush, ibalajiarun, JoshLind, heliuchuan, sasha8, banool, junkil-park, runtian-zhou, gelash, 0xmaayan, danielxiangzl and movekevin November 8, 2024 23:25

igor-aptos force-pushed the igor/native_compare branch from 12807a4 to 1c15e62 Compare November 9, 2024 00:17

igor-aptos force-pushed the igor/constant_serialized_size branch from 58152b4 to c268f4a Compare November 9, 2024 00:17

igor-aptos added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Nov 12, 2024

This comment has been minimized.

Sign in to view

igor-aptos force-pushed the igor/native_compare branch from 1c15e62 to 37384ce Compare November 12, 2024 18:04

igor-aptos force-pushed the igor/constant_serialized_size branch from c268f4a to 0c7a167 Compare November 12, 2024 18:04

runtian-zhou reviewed Nov 12, 2024

View reviewed changes

[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64>

4b040df

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

igor-aptos force-pushed the igor/constant_serialized_size branch from 0c7a167 to 4b040df Compare November 12, 2024 18:20

runtian-zhou approved these changes Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984

[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984

igor-aptos commented Oct 16, 2024

trunk-io bot commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024

msmouse Oct 16, 2024

igor-aptos Oct 16, 2024

igor-aptos Oct 21, 2024

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 12, 2024

github-actions bot commented Nov 12, 2024

runtian-zhou Nov 12, 2024

igor-aptos Nov 12, 2024

runtian-zhou Nov 12, 2024

igor-aptos Nov 12, 2024

[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984

Are you sure you want to change the base?

[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984

Conversation

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

codecov bot commented Oct 16, 2024

Codecov Report

msmouse Oct 16, 2024

Choose a reason for hiding this comment

igor-aptos Oct 16, 2024

Choose a reason for hiding this comment

igor-aptos Oct 21, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 12, 2024

✅ Forge suite realistic_env_max_load success on c268f4a86b05ae687941cba677591dd03cbb1629

github-actions bot commented Nov 12, 2024

✅ Forge suite compat success on fcd2dedf6ca61a23f4979e944c629dcdbdae5dca ==> c268f4a86b05ae687941cba677591dd03cbb1629

runtian-zhou Nov 12, 2024

Choose a reason for hiding this comment

igor-aptos Nov 12, 2024

Choose a reason for hiding this comment

runtian-zhou Nov 12, 2024

Choose a reason for hiding this comment

igor-aptos Nov 12, 2024

Choose a reason for hiding this comment

trunk-io bot commented Oct 16, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `c268f4a86b05ae687941cba677591dd03cbb1629`

✅ Forge suite `compat` success on `fcd2dedf6ca61a23f4979e944c629dcdbdae5dca` ==> `c268f4a86b05ae687941cba677591dd03cbb1629`