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: optimize per-transaction listeners #7071

Closed
Tracked by #5472
michalkucharczyk opened this issue Jan 7, 2025 · 0 comments · Fixed by #7316
Closed
Tracked by #5472

fatxpool: optimize per-transaction listeners #7071

michalkucharczyk opened this issue Jan 7, 2025 · 0 comments · Fixed by #7316
Assignees
Labels
T0-node This PR/Issue is related to the topic “node”.

Comments

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Jan 7, 2025

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

As the current implementation was delivered in the sprit of "make it work, make it fast", the time for optimization has come. Here is the idea.

Instead of having a single listener for every transaction in every view, we need to use the single stream of aggregated events for every transaction in the single view.

Some implementation details / hints:

  • view provides the stream of tuples: (tx_hash, status), new stream needs to be added here, and should follow logic of dropped_by_limits_sink.
  • new task for multi_view_listener needs to be added. It will select: aggregated streams map, add/remove view msgs, per-tx msgs: invalidate,finalize,...,
  • external listener is a stream (could be unfold). Internally it selects: add/remove view, view event (block_hash,status), per-tx msgs: invalidated, drpoped, finalzied, broadcated) and implements required logic (which currently is implemneted in ExternalWatcherContext,
  • create_external_watcher adds a new channel (rx,tx) between an unfolded stream (external listener instance) and mvl task. New enum for messages is needed.

Brain dump pic attached for the record.
mvl-opimize-braindump

@michalkucharczyk michalkucharczyk self-assigned this Jan 7, 2025
@michalkucharczyk michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Jan 8, 2025
@michalkucharczyk michalkucharczyk moved this from Todo to In Progress in fork-aware-txpool Jan 22, 2025
github-merge-queue bot pushed a commit that referenced this issue Feb 4, 2025
#### Description
During 2s block investigation it turned out that
[ForkAwareTxPool::register_listeners](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/transaction-pool/src/fork_aware_txpool/fork_aware_txpool.rs#L1036)
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](https://github.com/paritytech/polkadot-sdk/blob/2b18e080cfcd6b56ee638c729f891154e566e52e/substrate/client/transaction-pool/src/fork_aware_txpool/multi_view_listener.rs#L68-L95)
(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

---------

Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in fork-aware-txpool Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant