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.

Sign up for GitHub

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

Jump to bottom

Add upsert and destroy also to vector #6860

Merged
merged 14 commits into from
Mar 15, 2023
Merged

Add upsert and destroy also to vector #6860

merged 14 commits into from
Mar 15, 2023

Conversation

gerben-stavenga
Copy link
Contributor

Description

Adding upsert to simple map. It's tricky and can only be done with inline functions and lambdas.

Also added destroy to simple map and vector

Test Plan

Unit tests should suffice here

@gerben-stavenga gerben-stavenga requested a review from davidiw as a code owner March 2, 2023 12:26
@gerben-stavenga gerben-stavenga linked an issue Mar 2, 2023 that may be closed by this pull request
[Request] Add a upsert function in SimpleMap #6642
Closed
@ghost
Copy link

ghost commented Mar 2, 2023

You know what? At current gas pricing policy, I think best gas optimized way was create a trash bin, Then throw the vector to the trash bin forever, After that, Wait for a Aptos Governance Proposal to delete the trash bin.

@movekevin
Copy link
Contributor

You know what? At current gas pricing policy, I think best gas optimized way was create a trash bin, Then throw the vector to the trash bin forever, After that, Wait for a Aptos Governance Proposal to delete the trash bin.

We have a smart vector coming that is much more gas efficient: #6823. There's some benchmarking there

movekevin
movekevin reviewed Mar 2, 2023
View reviewed changes
aptos-move/framework/aptos-stdlib/sources/simple_map.move Outdated
@@ -77,6 +77,38 @@ module aptos_std::simple_map {
vector::push_back(&mut map.data, Element { key, value });
}

public inline fun upsert<Key: store, Value: store>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the tradeoff between this vs having a non-inline function that returns the old values (as Option and Option)?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think key should be drop + store and value should also be store + drop. REturn an Option that is not droppable seems useless in practice. At least, this is what Table looks like now.
  2. This function is so long that seems not worth an inline function.

movekevin
movekevin reviewed Mar 2, 2023
View reviewed changes
aptos-move/framework/aptos-stdlib/sources/simple_map.move Outdated

public inline fun destroy<Key: store, Value: store>(
map: SimpleMap<Key, Value>,
d: |Key, Value|
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about developer experience passing along a drop function? Is it weird at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think both functions may just want drop kvs. for undroppable kvs, user should handle them carefully with their own logic since a lambda doesn't help much in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for destroy it's fine. I wrote two upserts one with drop lambda, one returning an optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is a lambda not helping?

@movekevin movekevin requested a review from lightmark March 2, 2023 17:59
lightmark
lightmark reviewed Mar 2, 2023
View reviewed changes
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.

I highly recommend to add type constraints with drop and remove lambdas...

aptos-move/framework/aptos-stdlib/sources/simple_map.move Outdated
@@ -77,6 +77,38 @@ module aptos_std::simple_map {
vector::push_back(&mut map.data, Element { key, value });
}

public inline fun upsert<Key: store, Value: store>(
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think key should be drop + store and value should also be store + drop. REturn an Option that is not droppable seems useless in practice. At least, this is what Table looks like now.
  2. This function is so long that seems not worth an inline function.

aptos-move/framework/aptos-stdlib/sources/simple_map.move Outdated

public inline fun destroy<Key: store, Value: store>(
map: SimpleMap<Key, Value>,
d: |Key, Value|
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think both functions may just want drop kvs. for undroppable kvs, user should handle them carefully with their own logic since a lambda doesn't help much in that case.

lightmark
lightmark approved these changes Mar 14, 2023
View reviewed changes
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.

Awesome~

@gerben-stavenga gerben-stavenga enabled auto-merge (squash) March 14, 2023 21:34
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 901936f3ea4765bbddc2cf28371ba182e28c3b65

performance benchmark with full nodes : 5735 TPS, 6923 ms latency, 11700 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> 901936f3ea4765bbddc2cf28371ba182e28c3b65

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

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 901936f3ea4765bbddc2cf28371ba182e28c3b65

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 901936f3ea4765bbddc2cf28371ba182e28c3b65 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 8162 TPS, 4686 ms latency, 6500 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 901936f3ea4765bbddc2cf28371ba182e28c3b65
compatibility::simple-validator-upgrade::single-validator-upgrade : 4886 TPS, 7960 ms latency, 10100 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 901936f3ea4765bbddc2cf28371ba182e28c3b65
compatibility::simple-validator-upgrade::half-validator-upgrade : 4713 TPS, 8397 ms latency, 10500 ms p99 latency,no expired txns
4. upgrading second batch to new version: 901936f3ea4765bbddc2cf28371ba182e28c3b65
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7212 TPS, 5569 ms latency, 8400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 901936f3ea4765bbddc2cf28371ba182e28c3b65 passed
Test Ok

@gerben-stavenga gerben-stavenga merged commit 0eb577a into main Mar 15, 2023
@gerben-stavenga gerben-stavenga deleted the upsert branch March 15, 2023 00:26
@thepomeranian thepomeranian mentioned this pull request Apr 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lightmark lightmark lightmark approved these changes

@movekevin movekevin movekevin approved these changes

@wrwg wrwg Awaiting requested review from wrwg wrwg is a code owner

@davidiw davidiw Awaiting requested review from davidiw davidiw is a code owner

@clay-aptos clay-aptos Awaiting requested review from clay-aptos

@saharct saharct Awaiting requested review from saharct

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Request] Add a upsert function in SimpleMap
3 participants
@gerben-stavenga @movekevin @lightmark