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

Improve tx pool index for different sorting strategies #3971

Merged
merged 8 commits into from
May 10, 2023

Conversation

chenyukang
Copy link
Collaborator

What problem does this PR solve?

Issue Number: close #3942

Problem Summary:

What is changed and how it works?

What's Changed:

Related changes

  • Introduce multiple indexes in SortedTxMap for other sorting strategies.
  • Code refactor.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression

There is a performance penalty for Add, Remove entries in SortedTxMap:

before:

test component::tests::proposed::test_container_bench_add_remove ... bench:   5,718,670 ns/iter (+/- 174,237)
test component::tests::proposed::test_container_bench_sort       ... bench:   3,063,072 ns/iter (+/- 148,255)

after:

test component::tests::proposed::test_container_bench_add_remove ... bench:   6,640,891 ns/iter (+/- 163,363)
test component::tests::proposed::test_container_bench_sort       ... bench:   3,387,712 ns/iter (+/- 170,507)
  • Breaking backward compatibility
    No

Release note

None: Exclude this PR from the release note.

@chenyukang chenyukang requested a review from a team as a code owner May 6, 2023 08:02
@chenyukang chenyukang requested review from quake and removed request for a team May 6, 2023 08:02
@chenyukang chenyukang force-pushed the improve-tx-pool-index branch 2 times, most recently from 1ae9352 to 6d9b2ea Compare May 6, 2023 08:57
@chenyukang chenyukang force-pushed the improve-tx-pool-index branch 2 times, most recently from 37c6f61 to fb1e262 Compare May 6, 2023 15:12
@@ -90,7 +91,7 @@ impl TxEntry {
}

/// Returns a sorted_key
pub fn as_sorted_key(&self) -> AncestorsScoreSortKey {
pub fn as_score_key(&self) -> AncestorsScoreSortKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to change this method's comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think as_score_key maybe a better name, since we may have other sort keys.

self.sorted_index.insert(desc_entry.as_sorted_key());
}
}
self.update_descendants_index_key(&entry, EntryOp::Remove);
Copy link
Collaborator

@eval-exec eval-exec May 8, 2023

Choose a reason for hiding this comment

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

How about change EntryOp::Remove to EntryOp::Sub, since its purpose isn't "remove something" but "decrease weight".

@zhangsoledad zhangsoledad added this pull request to the merge queue May 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 10, 2023
@zhangsoledad zhangsoledad added this pull request to the merge queue May 10, 2023
Merged via the queue into nervosnetwork:develop with commit 2acaada May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve index of ckb tx-pool
3 participants