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

Tx pool rewrite with multi_index_map #3993

Merged
merged 28 commits into from
Aug 17, 2023

Conversation

chenyukang
Copy link
Collaborator

@chenyukang chenyukang commented May 25, 2023

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

This PR unifies the pending and gap pool with multi_index_map, with these two benefits:

  1. remove duplicated code logic between different pools.
  2. we add the same sorting fields for all entries in tx_pool for future improvement
  3. easy to implement future requirements such as replace-by-fee strategy.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks, linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

This PR may introduce performance regression for tx_pool, another change is we now do double-spend checking when a transaction is added to the pool.

I will do more testing for this change.

Release note

Title Only: Include only the PR title in the release note.
Note: Add a note under the PR title in the release note.

@chenyukang chenyukang requested a review from a team as a code owner May 25, 2023 04:13
@chenyukang chenyukang requested review from zhangsoledad and removed request for a team May 25, 2023 04:13
@chenyukang chenyukang force-pushed the tx_pool_refactor branch 5 times, most recently from 7f75581 to c78eef0 Compare May 29, 2023 05:46
@chenyukang chenyukang force-pushed the tx_pool_refactor branch 9 times, most recently from 2487a0f to 47606a7 Compare June 7, 2023 02:32
tx-pool/Cargo.toml Show resolved Hide resolved
tx-pool/src/pool.rs Outdated Show resolved Hide resolved
tx-pool/src/pool.rs Outdated Show resolved Hide resolved
tx-pool/src/component/pool_map.rs Show resolved Hide resolved
tx-pool/src/process.rs Outdated Show resolved Hide resolved
tx-pool/src/process.rs Show resolved Hide resolved
tx-pool/src/service.rs Show resolved Hide resolved
tx-pool/src/service.rs Outdated Show resolved Hide resolved
@eval-exec eval-exec added t:enhancement Type: Feature, refactoring. s:pr-created Status: PR is ready for review labels Jun 7, 2023
tx-pool/src/process.rs Outdated Show resolved Hide resolved
tx-pool/src/process.rs Outdated Show resolved Hide resolved
@chenyukang chenyukang force-pushed the tx_pool_refactor branch 2 times, most recently from 26cfe98 to eabc345 Compare June 8, 2023 14:08
@chenyukang
Copy link
Collaborator Author

@wyjin

I have finished some benchmark testing for comparing develop and this branch, here are some data, FYI:

develop branch, code: https://github.com/chenyukang/ckb/tree/develop-bench

test component::tests::bench::test_container_bench_add        ... bench:   2,144,760 ns/iter (+/- 3,794,550)
test component::tests::bench::test_container_bench_add_remove ... bench:   9,826,341 ns/iter (+/- 336,570)
test component::tests::bench::test_container_bench_sort       ... bench:   4,752,366 ns/iter (+/- 418,446)

current branch bench, code: https://github.com/chenyukang/ckb/tree/tx_pool_refactor_bench

test component::tests::bench::test_container_bench_add        ... bench:   1,956,728 ns/iter (+/- 4,352,650)
test component::tests::bench::test_container_bench_add_remove ... bench:  10,976,779 ns/iter (+/- 174,750)
test component::tests::bench::test_container_bench_sort       ... bench:   3,896,260 ns/iter (+/- 102,732)

It seems sort has been improved.

@wyjin
Copy link

wyjin commented Jun 12, 2023

@wyjin

I have finished some benchmark testing for comparing develop and this branch, here are some data, FYI:

develop branch, code: https://github.com/chenyukang/ckb/tree/develop-bench

test component::tests::bench::test_container_bench_add        ... bench:   2,144,760 ns/iter (+/- 3,794,550)
test component::tests::bench::test_container_bench_add_remove ... bench:   9,826,341 ns/iter (+/- 336,570)
test component::tests::bench::test_container_bench_sort       ... bench:   4,752,366 ns/iter (+/- 418,446)

current branch bench, code: https://github.com/chenyukang/ckb/tree/tx_pool_refactor_bench

test component::tests::bench::test_container_bench_add        ... bench:   1,956,728 ns/iter (+/- 4,352,650)
test component::tests::bench::test_container_bench_add_remove ... bench:  10,976,779 ns/iter (+/- 174,750)
test component::tests::bench::test_container_bench_sort       ... bench:   3,896,260 ns/iter (+/- 102,732)

It seems sort has been improved.

Is this good enough? remove appears to be slower because size of the pool never exceeds 3. I'd expect to see a major improvement in remove when the pool is bigger and the number of duplications in non-unique fields is also reasonably big.

The develop branch uses the upstream multi_index_map crate. I am curious/worried about how the performance compares to not using multi index map at all. (Is using multi index map causing performance regression?) Seems multi index is used in the ckb2023 branch so probably this test is already done?

@chenyukang
Copy link
Collaborator Author

Is this good enough?

Yes, upstream multi_index_map is used in develop branch, but only for proposed pool. So I want to have a benchmark compare develop with this branch.

From the above benchmark data, we can say the performance is on the same level with trivial difference. I guess this difference won't have too much effect on the whole CKB, since there are rpc operation involved in.

I have concern on the memory, today I write some testing code to verify it. I constantly add parent-child transactions to the pool(this is the worst case, since we need to keep the links).

The memory footprint is:

2w 65.7 MB
3w 86.1 MB
4w 131.5 MB
5w 158.2 MB
6w 201.4 MB

But we have max_ancestors_count to limit the depth.

Anyway, let's wait for the result from acceptance, I talked with them today for the testing.

@zhangsoledad zhangsoledad added this pull request to the merge queue Aug 17, 2023
@zhangsoledad zhangsoledad removed this pull request from the merge queue due to a manual request Aug 17, 2023
@zhangsoledad zhangsoledad added this pull request to the merge queue Aug 17, 2023
Merged via the queue into nervosnetwork:develop with commit 959ba02 Aug 17, 2023
@doitian doitian mentioned this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:pr-created Status: PR is ready for review t:enhancement Type: Feature, refactoring.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants