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

[mempool] reduce default capacity per user #4588

Closed
wants to merge 9 commits into from

Conversation

bchocho
Copy link
Contributor

@bchocho bchocho commented Sep 27, 2022

Description

Reduce the default capacity per user: 100 -> 20. This should make it less likely that some users take over mempool capacity.

Test Plan

Ran land_blocking and load_vs_perf_benchmark with good results. Will let CICD run the continuous tests.


This change is Reviewable

@bchocho bchocho added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 27, 2022
@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.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@bchocho bchocho force-pushed the brian/mempool-capacity-per-user branch from 1ec7fba to 9ddfa81 Compare September 28, 2022 00:21
@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.

@bchocho
Copy link
Contributor Author

bchocho commented Sep 28, 2022

The compat failures seem like an unrelated change in main. The land blocking failures are legit. Will have to investigate.

@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.

@bchocho bchocho force-pushed the brian/mempool-capacity-per-user branch from 372cb26 to 9573804 Compare September 28, 2022 15:09
@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.

@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.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@igor-aptos igor-aptos left a comment

Choose a reason for hiding this comment

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

Revert main.rs before landing

@github-actions

This comment has been minimized.

@bchocho bchocho force-pushed the brian/mempool-capacity-per-user branch from 9456038 to 2af5876 Compare September 30, 2022 16:58
@bchocho
Copy link
Contributor Author

bchocho commented Sep 30, 2022

This is what I got from load_vs_perf before it timed out (due to setting too short of a duration)

wanted/s     | submitted/s  | committed/s  | expired/s    | rejected/s   | chain txn/s  | latency      | p50 lat      | p90 lat      | p99 lat      | actual dur  
7500         | 7502         | 7060         | 442          | 0            | 6645         | 52126        | 60100        | 69500        | 79300        | 720         
8000         | 8003         | 6700         | 1303         | 0            | 6486         | 62638        | 64300        | 69400        | 79100        | 720         
9000         | 8999         | 6256         | 2742         | 0            | 6281         | 65866        | 64500        | 70300        | 80100        | 720         
10000        | 10004        | 6260         | 3743         | 0            | 6201         | 64635        | 63700        | 68000        | 73300        | 720         
12000        | 11993        | 6187         | 5805         | 0            | 6170         | 64522        | 63300        | 68500        | 77100        | 720         

I also noticed account minting takes longer (7m vs 1m previously) likely because of the parallelism reduction.

@bchocho bchocho marked this pull request as ready for review September 30, 2022 17:02
@sitalkedia
Copy link
Contributor

Are the failures in Forge tests unrelated to this change? Can we get a successful run?

@bchocho
Copy link
Contributor Author

bchocho commented Sep 30, 2022

I had set land_blocking to load_vs_perf_benchmark and didn’t set the duration long enough so it timed out. Github should run one more land_blocking before land :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on 843b204dce971d98449b82624f4f684c7a18b991 ==> 2af5876cd74b95bd0db4182bc3eb118f7a26f962

Compatibility test results for 843b204dce971d98449b82624f4f684c7a18b991 ==> 2af5876cd74b95bd0db4182bc3eb118f7a26f962 (PR)
1. Check liveness of validators at old version: 843b204dce971d98449b82624f4f684c7a18b991
compatibility::simple-validator-upgrade::liveness-check : 7748 TPS, 4887 ms latency, 8800 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 2af5876cd74b95bd0db4182bc3eb118f7a26f962
compatibility::simple-validator-upgrade::single-validator-upgrade : 5211 TPS, 7396 ms latency, 10000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 2af5876cd74b95bd0db4182bc3eb118f7a26f962
compatibility::simple-validator-upgrade::half-validator-upgrade : 4735 TPS, 8056 ms latency, 12800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 2af5876cd74b95bd0db4182bc3eb118f7a26f962
compatibility::simple-validator-upgrade::rest-validator-upgrade : 7850 TPS, 4757 ms latency, 8100 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for 843b204dce971d98449b82624f4f684c7a18b991 ==> 2af5876cd74b95bd0db4182bc3eb118f7a26f962 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 2af5876cd74b95bd0db4182bc3eb118f7a26f962

performance benchmark with full nodes : 7579 TPS, 4850 ms latency, 7200 ms p99 latency,(!) expired 20074 out of 3256500 txns
Test Ok

@bchocho bchocho closed this Oct 5, 2022
@davidiw davidiw deleted the brian/mempool-capacity-per-user branch December 4, 2022 17:30
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