Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native implementation to optimize object derived address computation #12768

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

igor-aptos
Copy link
Contributor

Description

Computing address for primary_fungible_asset store is expensive, and repeated multiple times. (especially if we start using fungible assets for gas charging)

This optimizes that computation by moving it to rust, and by caching it within a transaction (so computation in prologue gets saved for epilogue)

Type of Change

  • Performance improvement

Which Components or Systems Does This Change Impact?

  • Aptos Framework

How Has This Been Tested?

added unit test
performance on Coin => FA migration

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

Copy link

trunk-io bot commented Apr 2, 2024

⏱️ 1h 46m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 49m 🟥🟥
rust-move-unit-coverage 24m 🟩🟥
rust-lints 11m 🟩🟩
run-tests-main-branch 10m 🟩🟩
general-lints 4m 🟩🟩
check-dynamic-deps 4m 🟩🟩
semgrep/ci 1m 🟩🟩
file_change_determinator 36s 🟩🟩
file_change_determinator 31s 🟩🟩
file_change_determinator 30s 🟩🟩
permission-check 10s 🟩🟩
permission-check 10s 🟩🟩
rust-move-tests 8s 🟩🟩
permission-check 8s 🟩🟩
permission-check 7s 🟩🟩

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

Job Duration vs 7d avg Delta
run-tests-main-branch 6m 4m +30%
rust-lints 6m 7m -20%
rust-move-tests 3s 3m -98%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 92.15686% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 62.3%. Comparing base (d7867f7) to head (3236e99).
Report is 1 commits behind head on main.

Files Patch % Lines
...s-gas-schedule/src/gas_schedule/aptos_framework.rs 0.0% 3 Missing ⚠️
aptos-move/aptos-vm/src/move_vm_ext/vm.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12768   +/-   ##
=======================================
  Coverage    62.3%    62.3%           
=======================================
  Files         828      828           
  Lines      185791   185829   +38     
=======================================
+ Hits       115821   115855   +34     
- Misses      69970    69974    +4     

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

Copy link
Contributor

@vgao1996 vgao1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any perf data on this?

@@ -194,12 +194,18 @@ module aptos_framework::object {
from_bcs::to_address(hash::sha3_256(bytes))
}

public native fun create_user_derived_object_address_impl(source: address, derive_from: address): address;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the native private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh mistake

@igor-aptos
Copy link
Contributor Author

if gas is paid from primary fungible store, prologue+epilogue gets 35% faster

@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from d6a53a8 to 9194612 Compare April 2, 2024 20:50
@igor-aptos igor-aptos requested a review from vgao1996 April 3, 2024 06:18
@igor-aptos igor-aptos removed the request for review from mstraka100 April 23, 2024 17:01
Copy link
Contributor

@lightmark lightmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from 9194612 to 75dd92c Compare April 23, 2024 21:24
@igor-aptos igor-aptos enabled auto-merge (squash) April 24, 2024 00:05

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from 75dd92c to 049f350 Compare April 24, 2024 20:25

This comment has been minimized.

This comment has been minimized.

@igor-aptos 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 Apr 24, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from 049f350 to b43e3ea Compare April 25, 2024 02:26

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from b43e3ea to 01cc490 Compare April 25, 2024 05:03
@igor-aptos igor-aptos force-pushed the igor/native_object_derive_address branch from 01cc490 to 3236e99 Compare April 25, 2024 05:35

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.10.1 ==> 3236e997ce1c964172a73b75fe90dc6b3a597a1c

Compatibility test results for aptos-node-v1.10.1 ==> 3236e997ce1c964172a73b75fe90dc6b3a597a1c (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6663 txn/s, latency: 4989 ms, (p50: 4800 ms, p90: 8400 ms, p99: 10200 ms), latency samples: 233220
2. Upgrading first Validator to new version: 3236e997ce1c964172a73b75fe90dc6b3a597a1c
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1869 txn/s, latency: 15580 ms, (p50: 19300 ms, p90: 22200 ms, p99: 22600 ms), latency samples: 91620
3. Upgrading rest of first batch to new version: 3236e997ce1c964172a73b75fe90dc6b3a597a1c
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1721 txn/s, latency: 16123 ms, (p50: 19100 ms, p90: 21400 ms, p99: 22300 ms), latency samples: 89500
4. upgrading second batch to new version: 3236e997ce1c964172a73b75fe90dc6b3a597a1c
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3447 txn/s, latency: 9080 ms, (p50: 9900 ms, p90: 12300 ms, p99: 12600 ms), latency samples: 137900
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 3236e997ce1c964172a73b75fe90dc6b3a597a1c passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 3236e997ce1c964172a73b75fe90dc6b3a597a1c

two traffics test: inner traffic : committed: 7960 txn/s, latency: 4915 ms, (p50: 4800 ms, p90: 5700 ms, p99: 10200 ms), latency samples: 3446960
two traffics test : committed: 100 txn/s, latency: 2028 ms, (p50: 2000 ms, p90: 2200 ms, p99: 5400 ms), latency samples: 1660
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.208, avg: 0.202", "QsPosToProposal: max: 0.282, avg: 0.217", "ConsensusProposalToOrdered: max: 0.478, avg: 0.396", "ConsensusOrderedToCommit: max: 0.407, avg: 0.382", "ConsensusProposalToCommit: max: 0.793, avg: 0.778"]
Max round gap was 1 [limit 4] at version 26495. Max no progress secs was 4.782718 [limit 15] at version 1731171.
Test Ok

@igor-aptos igor-aptos merged commit 93cf759 into main Apr 25, 2024
53 checks passed
@igor-aptos igor-aptos deleted the igor/native_object_derive_address branch April 25, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants