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

Move smart vector/table to framework #6823

Merged
merged 4 commits into from
Mar 2, 2023
Merged

Conversation

lightmark
Copy link
Contributor

@lightmark lightmark commented Feb 28, 2023

Description

  1. add a per-bucket limit to avoid DOS
  2. remove unusable inline function as [simple_map] remove inline functions #6817
  3. change big_vector constructor to be friend only, the only friend is smart vector
  4. move big_vector, smart_vector, smart_table to aptos_std
  5. add a file in e2e-move-test with basic gas benchmarking.

After the gas change #6816, it becomes:

huge_smart_vector_create_gas |  5084900 | 0.254 | 0.763 | 1.525
huge_smart_vector_update_gas |     2100 | 0.000 | 0.000 | 0.001
huge_smart_vector_read_gas |      400 | 0.000 | 0.000 | 0.000
huge_vector_create_gas |  4080900 | 0.204 | 0.612 | 1.224
huge_vector_update_gas |  3995700 | 0.200 | 0.599 | 1.199
huge_vector_read_gas |     2600 | 0.000 | 0.000 | 0.001
huge_smart_table_create_gas |  2043500 | 0.102 | 0.307 | 0.613
huge_smart_table_update_gas |      700 | 0.000 | 0.000 | 0.000
huge_smart_table_read_gas |      300 | 0.000 | 0.000 | 0.000
huge_table_create_gas | 50594900 | 2.530 | 7.589 | 15.178
huge_table_update_gas |    50800 | 0.003 | 0.008 | 0.015
huge_table_read_gas  |      300 | 0.000 | 0.000 | 0.000

Test Plan

unit test

Copy link
Contributor

@movekevin movekevin left a comment

Choose a reason for hiding this comment

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

Did you run the gas benchmarking with the 100x reduction in Alden's PR?

@lightmark
Copy link
Contributor Author

Did you run the gas benchmarking with the 100x reduction in Alden's PR?

It's the old one. I will update.

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from 55e9685 to 9020778 Compare February 28, 2023 21:49
@lightmark
Copy link
Contributor Author

now with the new gas schedule, they outperform vector/table for large datasets in all aspects except for the creation of huge vector. I think it is both reasonable and acceptable according to current storage fee pricing. cc @msmouse

@movekevin
Copy link
Contributor

movekevin commented Feb 28, 2023

ation of huge vector. I think it is both reasonable and acceptable according to current storage fee pricing. cc @msmouse

Why is creation of huge smart vector more expensive than creating a huge vector?

And how huge is huge? Is it more than 1 KB of data total?

@lightmark
Copy link
Contributor Author

ation of huge vector. I think it is both reasonable and acceptable according to current storage fee pricing. cc @msmouse

Why is creation of huge smart vector more expensive than creating a huge vector?

And how huge is huge? Is it more than 1 KB of data total?

Because it involves some extra logic to find the right bucket, etc. It also 100% fully utilize 1kb free quota. by huge I mean way more than 1kb, here 10000 * size_of(u64) I suppose 80kb.

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from 9020778 to 9469007 Compare March 1, 2023 04:26
@clay-aptos clay-aptos added documentation Improvements or additions to documentation release-notes This tag indicates the associated change should be documented in the Aptos Release Notes. labels Mar 1, 2023
Copy link
Contributor

@clay-aptos clay-aptos left a comment

Choose a reason for hiding this comment

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

Hi @lightmark , the docs here LGTM with some tiny comments I left inline. Perhaps the Move team would be alright with me editing the source files directly sometime (rather than comment on the Markdown output).

I am also asking for your, @wrwg and the team's opinion on including these wonderful additions in the relevant release notes. They seem significant, so I have created and added a release-notes tag to see if we can capture and highlight such changes.

See this issue comment for more details:
#6373 (comment)

Thanks!

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from 9469007 to 19f9ecf Compare March 1, 2023 22:31
@lightmark
Copy link
Contributor Author

@clay-aptos Hey I think it is a really good idea to include it in the release notes as we wanna promoting the usage of the new gas-saving data structures.

@lightmark lightmark enabled auto-merge (squash) March 1, 2023 22:38
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from 19f9ecf to eda1ad0 Compare March 2, 2023 00:00
Copy link
Contributor

@bowenyang007 bowenyang007 left a comment

Choose a reason for hiding this comment

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

i trust you

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from eda1ad0 to 495a3d0 Compare March 2, 2023 00:11
@msmouse
Copy link
Contributor

msmouse commented Mar 2, 2023

After the gas change #6816, it becomes

Where is the "before" numbers? 😁

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lightmark lightmark force-pushed the move_smart_table_to_framework branch from 495a3d0 to f969295 Compare March 2, 2023 01:00
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f96929550d2667d2f8eea7abf6e6e058bb347200

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f96929550d2667d2f8eea7abf6e6e058bb347200 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7994 TPS, 4820 ms latency, 7000 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f96929550d2667d2f8eea7abf6e6e058bb347200
compatibility::simple-validator-upgrade::single-validator-upgrade : 4577 TPS, 8686 ms latency, 12200 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f96929550d2667d2f8eea7abf6e6e058bb347200
compatibility::simple-validator-upgrade::half-validator-upgrade : 4780 TPS, 8216 ms latency, 11200 ms p99 latency,no expired txns
4. upgrading second batch to new version: f96929550d2667d2f8eea7abf6e6e058bb347200
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7128 TPS, 5718 ms latency, 10100 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f96929550d2667d2f8eea7abf6e6e058bb347200 passed
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

✅ Forge suite land_blocking success on f96929550d2667d2f8eea7abf6e6e058bb347200

performance benchmark with full nodes : 6107 TPS, 6474 ms latency, 18500 ms p99 latency,(!) expired 560 out of 2608360 txns
Test Ok

@lightmark lightmark merged commit 88bd6c9 into main Mar 2, 2023
@lightmark lightmark deleted the move_smart_table_to_framework branch March 2, 2023 01:44
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> f96929550d2667d2f8eea7abf6e6e058bb347200

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> f96929550d2667d2f8eea7abf6e6e058bb347200 (PR)
Upgrade the nodes to version: f96929550d2667d2f8eea7abf6e6e058bb347200
framework_upgrade::framework-upgrade::full-framework-upgrade : 6734 TPS, 5719 ms latency, 9700 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> f96929550d2667d2f8eea7abf6e6e058bb347200 passed
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation release-notes This tag indicates the associated change should be documented in the Aptos Release Notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants