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

fatxpool: do not use individual transaction listeners #7316

Merged
merged 21 commits into from
Feb 4, 2025

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented Jan 23, 2025

Description

During 2s block investigation it turned out that ForkAwareTxPool::register_listeners call takes significant amount of time.

register_listeners: at HashAndNumber { number: 12, hash: 0xe9a1...0b1d2 } took 200.041933ms
register_listeners: at HashAndNumber { number: 13, hash: 0x5eb8...a87c6 } took 264.487414ms
register_listeners: at HashAndNumber { number: 14, hash: 0x30cb...2e6ec } took 340.525566ms
register_listeners: at HashAndNumber { number: 15, hash: 0x0450...4f05c } took 405.686659ms
register_listeners: at HashAndNumber { number: 16, hash: 0xfa6f...16c20 } took 477.977836ms
register_listeners: at HashAndNumber { number: 17, hash: 0x5474...5d0c1 } took 483.046029ms
register_listeners: at HashAndNumber { number: 18, hash: 0x3ca5...37b78 } took 482.715468ms
register_listeners: at HashAndNumber { number: 19, hash: 0xbfcc...df254 } took 484.206999ms
register_listeners: at HashAndNumber { number: 20, hash: 0xd748...7f027 } took 414.635236ms
register_listeners: at HashAndNumber { number: 21, hash: 0x2baa...f66b5 } took 418.015897ms
register_listeners: at HashAndNumber { number: 22, hash: 0x5f1d...282b5 } took 423.342397ms
register_listeners: at HashAndNumber { number: 23, hash: 0x7a18...f2d03 } took 472.742939ms
register_listeners: at HashAndNumber { number: 24, hash: 0xc381...3fd07 } took 489.625557ms

This PR implements the idea outlined in #7071. Instead of having a separate listener for every transaction in each view, we now use a single stream of aggregated events per view, with each stream providing events for all transactions in that view. Each event is represented as a tuple: (transaction-hash, transaction-status). This significantly reduce the time required for maintain.

Review Notes

  • single aggregated stream, provided by the individual view delivers events in form of (transaction-hash, transaction-status),
  • MultiViewListener now has a task. This task is responsible for:
    • polling the stream map (which consists of individual view's aggregated streams) and the controller_receiver which provides side-channel commands (like AddView or FinalizeTransaction) sent from the transaction pool.
    • dispatching individual transaction statuses and control commands into the external (created via API, e.g. over RPC) listeners of individual transactions,
  • external listener is responsible for status handling logic (e.g. deduplication of events, or ignoring some of them) and triggering statuses to external world (this was not changed).
  • level of debug messages was adjusted (per-tx messages shall be trace),

Closes #7071

@michalkucharczyk michalkucharczyk marked this pull request as draft January 23, 2025 16:15
@michalkucharczyk michalkucharczyk added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Jan 23, 2025
@michalkucharczyk michalkucharczyk marked this pull request as ready for review January 29, 2025 11:45
michalkucharczyk and others added 2 commits January 30, 2025 13:33
…view_listener.rs

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

Left some stylistic comments and some questions/opinions. LGTM but I will approve after clarifying the comments (if we don't need other changes than the ones I suggested).

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13075004797
Failed job name: test-linux-stable

@michalkucharczyk michalkucharczyk added this pull request to the merge queue Feb 4, 2025
Merged via the queue into master with commit aa42deb Feb 4, 2025
205 of 207 checks passed
@michalkucharczyk michalkucharczyk deleted the mku-txpool-register-listeners branch February 4, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fatxpool: optimize per-transaction listeners
3 participants