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 should cache "final ledger state" #1301

Closed
edsko opened this issue Nov 27, 2019 · 2 comments · Fixed by #1599
Closed

Mempool should cache "final ledger state" #1301

edsko opened this issue Nov 27, 2019 · 2 comments · Fixed by #1599
Assignees
Labels
benchmarking consensus issues related to ouroboros-consensus enhancement New feature or request mempool optimisation Performance optimisation
Milestone

Comments

@edsko
Copy link
Contributor

edsko commented Nov 27, 2019

The mempool should store a reference to the ledger state after all transactions have been validated. Then when new transactions get added, and the ledger state at the tip of the chain has not changed, we can skip transaction evaluation altogether (initVR, afterKnownValid) and just reuse the previously stored ledger state. This could greatly improve the performance of the mempool, with relatively low memory overhead (I think).

@edsko edsko added consensus issues related to ouroboros-consensus mempool enhancement New feature or request labels Nov 27, 2019
@dcoutts dcoutts added optimisation Performance optimisation benchmarking labels Nov 27, 2019
@dcoutts
Copy link
Contributor

dcoutts commented Nov 27, 2019

Nice idea. Not critical yet. Lets wait 'til we have a good benchmark and/or profile of high throughput tx submission so we can measure the effect of the change (which should mostly be reduced CPU use for the case of a relay getting lots of txs from lots of peers at once so that it does not benefit from batching).

@dcoutts
Copy link
Contributor

dcoutts commented Feb 7, 2020

Apparently the latest benchmarks indicate that this is now a measurable effect, with the time to add a single tx being linearly related to the size of the mempool. This doesn't mean we have to prioritise this now.

@mrBliss mrBliss self-assigned this Feb 10, 2020
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1564) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
@mrBliss mrBliss added this to the S6 2020-02-13 milestone Feb 10, 2020
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1564) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1564) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
@mrBliss mrBliss linked a pull request Feb 10, 2020 that will close this issue
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
iohk-bors bot added a commit that referenced this issue Feb 10, 2020
1599: Mempool: better revalidation and caching of the ledger state r=mrBliss a=mrBliss

Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This   function will be much easier to test. We provide an implementation of `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the implementations by relying on the background thread for synchronising the Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool   to it (#1301). Previously, we had to reapply all transactions whenever we added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is thunk-free.

* Correct revalidation in (#1564) after explicitly removing transactions from the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.


Also includes a rewrite of the Mempool tests using the `Mock.Utxo`. Using the `Mock.Utxo` instead of the overly simplistic `TestTx` we were using allows for more complicated dependencies between transactions. This better resembles the real transactions, at the cost of some complexity. With the former transaction type, we weren't even able to trigger #1565.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
mrBliss added a commit that referenced this issue Feb 10, 2020
Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This
  function will be much easier to test. We provide an implementation of
  `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the
  implementations by relying on the background thread for synchronising the
  Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool
  to it (#1301). Previously, we had to reapply all transactions whenever we
  added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is
  thunk-free.

* Correct revalidation in (#1565) after explicitly removing transactions from
  the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.
iohk-bors bot added a commit that referenced this issue Feb 10, 2020
1599: Mempool: better revalidation and caching of the ledger state r=mrBliss a=mrBliss

Fixes #1565 and #1301.

* Replace `Mempool.addTxs` with `Mempool.tryAddTxs`, which doesn't block. This   function will be much easier to test. We provide an implementation of `Mempool.addTxs` in terms of `Mempool.tryAddTxs`. We simplify the implementations by relying on the background thread for synchronising the Mempool with an updated ledger state.

* Cache the `TickedLedgerState` after applying all transactions in the Mempool   to it (#1301). Previously, we had to reapply all transactions whenever we added a new transaction. The new approach is much faster.

* As we're keeping the `TickedLedgerState` in memory, make sure it is thunk-free.

* Correct revalidation in (#1564) after explicitly removing transactions from the Mempool, see `revalidateTxsFor`.

* Cleanup of conversion functions in `TxSeq`.

* O(1) `MempoolSize` calculation for a `TxSeq` using its fingertree measure.


Also includes a rewrite of the Mempool tests using the `Mock.Utxo`. Using the `Mock.Utxo` instead of the overly simplistic `TestTx` we were using allows for more complicated dependencies between transactions. This better resembles the real transactions, at the cost of some complexity. With the former transaction type, we weren't even able to trigger #1565.

Co-authored-by: Thomas Winant <thomas@well-typed.com>
@iohk-bors iohk-bors bot closed this as completed in #1599 Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarking consensus issues related to ouroboros-consensus enhancement New feature or request mempool optimisation Performance optimisation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants