Remove 2nd mapTx lookup in CTxMemPool::removeForBlock #13189

1. 
   promag commented at 5:40 PM on May 8, 2018: member

   CTxMemPool::removeForBlock has 2 loops over the block transactions:

   • the first determines which block transactions are in the mempool (required for minerPolicyEstimator);
   • the second removes from the mempool transactions that conflict with the block transactions.

   With this PR the second loop is optimized because mapTx lookup is removed and CTxMemPool::RemoveStaged is called only once (since it supports batch removal).

2. 
   MarcoFalke commented at 6:10 PM on May 8, 2018: member

   Note that this conflicts with #12979 (Split validationinterface into paralell validation/mempool interfaces), which is why I'd suggest to rebase on top of that.

3. promag force-pushed on May 8, 2018
4. 
   promag commented at 9:41 PM on May 8, 2018: member

   Rebased on top of #12979 as per @MarcoFalke suggestion (currently it conflicts with master).

5. fanquake added the label Refactoring on May 8, 2018
6. fanquake added the label Mempool on May 8, 2018
7. MarcoFalke added the label Needs rebase on Jun 6, 2018
8. promag force-pushed on Jul 1, 2018
9. promag force-pushed on Jul 1, 2018
10. promag force-pushed on Jul 1, 2018
11. trivial: Add const to first argument of CTxMemPool::RemoveStaged 9266c29ae8
12. mempool: Remove 2nd mapTx lookup in CTxMemPool::removeForBlock e5af27e92c
13. promag force-pushed on Jul 1, 2018
14. 
    promag commented at 9:15 AM on July 1, 2018: member

    Rebased on master.

15. MarcoFalke removed the label Needs rebase on Jul 1, 2018
16. 
    MarcoFalke commented at 9:24 AM on July 1, 2018: member

    This seems non-trivial to review. Could you please add the motivation and advantages/disadvantages to the OP?

17. 
    promag commented at 10:39 AM on July 1, 2018: member

    @MarcoFalke is that enough?

    Maybe @morcos and @sdaftuar could take a look too?

18. fanquake requested review from morcos on Jul 1, 2018
19. fanquake removed review request from morcos on Jul 1, 2018
20. fanquake requested review from sdaftuar on Jul 1, 2018
21. fanquake requested review from morcos on Jul 1, 2018
22. 
    MarcoFalke commented at 9:19 AM on July 2, 2018: member

    Is there any meaningful advantage by removing the second lookup?

23. 
    promag commented at 10:57 AM on July 2, 2018: member

    @MarcoFalke this should only improve the performance.

24. 
    MarcoFalke commented at 11:07 AM on July 2, 2018: member

    should

    Would be nice to have a benchmark that shows this. If the performance is worse or identical with this patch, it might not be worth it.

25. 
    promag commented at 10:22 PM on July 8, 2018: member

    @MarcoFalke I'll try to measure it.

26. 
    DrahtBot commented at 8:56 PM on August 25, 2018: member

    <!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 55 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

27. DrahtBot closed this on Aug 25, 2018

    ---

28. DrahtBot reopened this on Aug 25, 2018

    ---

29. 
    DrahtBot commented at 6:56 PM on September 7, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #13949 (Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

30. 
    promag commented at 10:20 AM on March 24, 2019: member

    Not worth it, mapTx is not that large and lookups aren't that expensive and also removeForBlock it not that frequent.

31. promag closed this on Mar 24, 2019

    ---

32. promag deleted the branch on Mar 24, 2019
33. DrahtBot locked this on Dec 16, 2021

Contributors

promagMarcoFalkeDrahtBot

Review Requested
morcossdaftuar