Accurately account for mempool index memory #18086

sipa · 2020-02-07T02:21:54Z

This introduces a C++ allocator class (memusage::AccountingAllocator) which enables containers that accurately account for all their memory allocations in a tracker variable.

This is then used to replace the heuristics in the mempool to guess memory usage.

sipa · 2020-02-07T02:23:43Z

@sdaftuar You may be interested in this for #18044 - it should be easy to use this to determine how much an extra index adds.

DrahtBot · 2020-02-07T03:07:56Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove CTxMempool::mapLinks data structure member #19478 (Remove CTxMempool::mapLinks data structure member by JeremyRubin)
refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
Change UpdateForDescendants to use Epochs #18191 (Change UpdateForDescendants to use Epochs by JeremyRubin)
Use wtxid for transaction relay #18044 (Use wtxid for transaction relay by sdaftuar)
txmempool: split epoch logic into class #18017 (txmempool: split epoch logic into class by ajtowns)
Add fee_est tool for debugging fee estimation code #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.

JeremyRubin · 2020-02-07T03:26:27Z

@sipa can you take a look at JeremyRubin#7?

It looks like when we get rid of mapTxLinks we'd then want to use an accounting allocator on the new child sets and get some DynamicMemoryUsage added to the cached inner usage?

At some point I also want to better break down the cachedInnerUsage tracking into buckets as it makes it easier to trace bugs.

sipa · 2020-02-07T03:45:14Z

I'm happy to rebase this on top of a removal of mapLinks, if that'd go in first. It looks pretty like I'd just need to remove mapLinks from this as well.

I don't know if breaking down cachedInnerUsage is all that meaningful, as the Check() function can verify its correctness in aggregate.

There are plenty more options of shuffling things with AccountingAllocators... I don't have any particular preference apart from minimizing code changes.

JeremyRubin · 2020-02-07T03:53:01Z

Don't you need to make TxLinks sets use accounting allocators in this patch too?

sipa · 2020-02-07T04:04:40Z

@JeremyRubin Ah, that seems reasonable. My priority was the things that use a boost multiindex as guessing memory usage there is very hard, but no reason why it can't be extended to also the sets there.

JeremyRubin · 2020-02-07T04:45:05Z

Do you know if we were over or underestimating before?

sipa · 2020-02-07T04:47:46Z

In mapTx we were overestimating slightly for realistic mempool size, but underestimating for near-empty ones.

JeremyRubin · 2020-02-07T05:16:09Z

Ok the near empty case doesn't really matter... w.r.t. the realistic size one, here's a question just to be detailed/pedantic with what this change does:

Given that we were previously overestimating, if I set my mempool to 100MB then I actually was getting (let's say) 99 MB, if the "slightly" was by 1%. If after this change, I am now actually using a little bit more memory is this enough to be concerned? E.g., are we going to see a bunch of raspberry pi users complaining of OOMs?

sipa · 2020-02-07T05:33:13Z

Our memory accounting has never been more accurate than in the orders of 10s of MBs. A tiny change like this shouldn't matter.

And probably on the long term, it helps: it's likely the case that in some scenarios we were off more than others (e.g. many tiny transactions vs a few big ones) - better estimations means that experimentally-determined max memory limits need less margin.

JeremyRubin · 2020-02-07T05:57:34Z

The mempool tests happen to be very fragile -- I've noticed that sometimes when I made the mempool more memory efficient (e.g., in removing mapLinks) there are a bunch of failures because the tests depend on evictions happening because it's out of space.

I'm not positive that's why your tests are failing, but it could be related.

sipa · 2020-02-07T06:02:29Z

Yes, they're failing because the mempool limits now work slightly differently. The test makes rather silly assumptions on the menory usage of transaction entries.

laanwj · 2020-03-27T15:33:42Z

Concept ACK, I think this is an elegant way to track accurate usage.

hebasto · 2020-05-03T05:40:22Z

Concept ACK.

src/memusage.h

hebasto · 2020-05-14T17:46:11Z

In mapTx we were overestimating slightly for realistic mempool size, but underestimating for near-empty ones.

As unit tests are based on "near-empty" containers the difference of estimations seems broke them.

Testing ed1c5b1 with mempool_tests/MempoolSizeLimitTest: the difference in CTxMemPool::DynamicMemoryUsage() is ~700 bytes.

hebasto · 2020-07-10T06:50:24Z

src/memusage.h

+    explicit AccountingAllocator(size_t& allocated) noexcept : m_allocated(&allocated) {}
+
+    //! A copy-constructed container will be non-accounting.
+    static AccountingAllocator select_on_container_copy_construction() { return AccountingAllocator(); }


fc0ba65
From the docs it follows that:

Suggested change

static AccountingAllocator select_on_container_copy_construction() { return AccountingAllocator(); }

AccountingAllocator select_on_container_copy_construction() const { return AccountingAllocator(); }

…eutral

Includes thread safety annotations added by Hennadii Stepanov.

DrahtBot · 2020-07-22T20:16:29Z

🐙 This pull request conflicts with the target branch and needs rebase.

jonatack

Concept ACK, light pre-rebase ACK.

Feel free to ignore: commit message eace374 s/inherent/inherit/; the 2 test commits both do essentially the same thing in under 4 lines, could be squashed.

adamjonas · 2020-08-14T13:31:26Z

It looks like this will need a rebase anyway, but I'm having issues compiling eace374, which looks consistent with the CI.

Error

./memusage.h:92:64: error: cannot initialize a parameter of type 'std::__1::allocator<std::__1::__hash_node<int, void *> >::pointer' (aka 'std::__1::__hash_node<int, void *> *') with an lvalue of type 'int *'
    template<typename P> void destroy(P* ptr) { m_base.destroy(ptr); }
                                                               ^~~
/Library/Developer/CommandLineTools/usr/include/c++/v1/memory:1742:18: note: in instantiation of function template specialization 'memusage::AccountingAllocator<std::__1::__hash_node<int, void *> >::destroy<int>' requested here
            {__a.destroy(__p);}
                 ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/memory:1595:14: note: in instantiation of function template specialization 'std::__1::allocator_traits<memusage::AccountingAllocator<std::__1::__hash_node<int, void *> > >::__destroy<int>' requested here
            {__destroy(__has_destroy<allocator_type, _Tp*>(), __a, __p);}
             ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/__hash_table:845:29: note: in instantiation of function template specialization 'std::__1::allocator_traits<memusage::AccountingAllocator<std::__1::__hash_node<int, void *> > >::destroy<int>' requested here
            __alloc_traits::destroy(__na_, _NodeTypes::__get_ptr(__p->__value_));
                            ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/memory:2648:7: note: in instantiation of member function 'std::__1::__hash_node_destructor<memusage::AccountingAllocator<std::__1::__hash_node<int, void *> > >::operator()' requested here
      __ptr_.second()(__tmp);
      ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/memory:2602:19: note: in instantiation of member function 'std::__1::unique_ptr<std::__1::__hash_node<int, void *>, std::__1::__hash_node_destructor<memusage::AccountingAllocator<std::__1::__hash_node<int, void *> > > >::reset' requested here
  ~unique_ptr() { reset(); }
                  ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/__hash_table:2034:29: note: in instantiation of member function 'std::__1::unique_ptr<std::__1::__hash_node<int, void *>, std::__1::__hash_node_destructor<memusage::AccountingAllocator<std::__1::__hash_node<int, void *> > > >::~unique_ptr' requested here
        __node_holder __h = __construct_node_hash(__hash, _VSTD::forward<_Args>(__args)...);
                            ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/__hash_table:1146:16: note: in instantiation of function template specialization 'std::__1::__hash_table<int, std::__1::hash<int>, std::__1::equal_to<int>, memusage::AccountingAllocator<int> >::__emplace_unique_key_args<int, const int &>' requested here
        return __emplace_unique_key_args(_NodeTypes::__get_key(__x), __x);
               ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/unordered_set:864:18: note: in instantiation of member function 'std::__1::__hash_table<int, std::__1::hash<int>, std::__1::equal_to<int>, memusage::AccountingAllocator<int> >::__insert_unique' requested here
        __table_.__insert_unique(*__first);
                 ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/unordered_set:831:5: note: in instantiation of function template specialization 'std::__1::unordered_set<int, std::__1::hash<int>, std::__1::equal_to<int>, memusage::AccountingAllocator<int> >::insert<const int *>' requested here
    insert(__il.begin(), __il.end());
    ^
test/allocator_tests.cpp:245:24: note: in instantiation of member function 'std::__1::unordered_set<int, std::__1::hash<int>, std::__1::equal_to<int>, memusage::AccountingAllocator<int> >::unordered_set' requested here
        container_type container1({}, 2, std::hash<int>(), std::equal_to<int>(), memusage::AccountingAllocator<int>(dummy)); // container1 is accounted for in 'dummy'
                       ^
/Library/Developer/CommandLineTools/usr/include/c++/v1/memory:1880:52: note: passing argument to parameter '__p' here
    _LIBCPP_INLINE_VISIBILITY void destroy(pointer __p) {__p->~_Tp();}
                                                   ^
1 error generated.

hebasto · 2020-10-13T18:20:33Z

@sipa @adamjonas
Mind looking into https://github.com/hebasto/bitcoin/commits/201013-build18086 ?

That branch is rebased and:

dropped "Make AccountingAllocator not inherent from std::allocator" commit
integrated my suggestion
improved tests: s/BOOST_CHECK/BOOST_CHECK_EQUAL/ (was very useful for me during debugging)
added a commit with the test for std::map as it is the container we actually use with the AccountingAllocator (among std::vector and boost::multi_index_container)

CI CentOS build fails due to the old GCC 4.8 which, I think, just lacks support of the std::allocator_traits<Alloc>::select_on_container_copy_construction. It even lacks map( const Allocator& alloc ) constructor.

sipa · 2020-10-13T19:30:08Z

I'll get back to this after the 0.21 branching.

DrahtBot · 2021-12-15T11:22:09Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2022-03-21T13:07:35Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake · 2022-03-22T10:56:46Z

I'll get back to this after the 0.21 branching.

I'm going to close this, and mark "Up for Grabs". @martinus you might be interested here given #22702?

hebasto · 2022-11-30T22:51:19Z

I'll get back to this after the 0.21 branching.

I'm going to close this, and mark "Up for Grabs".

#26614.

fanquake added Mempool Resource usage labels Feb 7, 2020

fanquake requested a review from JeremyRubin February 7, 2020 02:23

sipa force-pushed the 201910_accounting_allocator branch from 1c4deb5 to 6b79662 Compare February 7, 2020 02:50

sipa force-pushed the 201910_accounting_allocator branch from 6b79662 to 9e1de7a Compare February 7, 2020 03:28

sipa force-pushed the 201910_accounting_allocator branch from 9e1de7a to ea454c2 Compare February 11, 2020 01:28

DrahtBot mentioned this pull request Feb 22, 2020

Change UpdateForDescendants to use Epochs #18191

Closed

DrahtBot added the Needs rebase label Mar 5, 2020

hebasto reviewed May 14, 2020

View reviewed changes

src/memusage.h Outdated Show resolved Hide resolved

hebasto reviewed Jul 10, 2020

View reviewed changes

hebasto and others added 12 commits July 20, 2020 22:11

test: make mempool_tests/MempoolSizeLimitTest allocation-neutral

0c03c61

test: make validation_flush_tests/getcoinscachesizestate allocation-n…

f48ba0b

…eutral

Add AccountingAllocator to help with memusage tracking

8bad146

Make AccountingAllocator not inherent from std::allocator

eace374

Accurate mapTx counting

073e60c

Account mapDeltas accurately

277d2ef

Account mapLinks accurately

844bf4f

Includes thread safety annotations added by Hennadii Stepanov.

Account vTxHashes accurately

8ef608a

Accurately account for queuedTx in DisconnectedBlockTransactions

ce9ad71

Add allocator support to indirectmap

e088ce8

Account for mapNextTx accurately

fc2aed7

Account mapLinks.{children,parents} accurately

6ad6e54

sipa force-pushed the 201910_accounting_allocator branch from 67f1516 to 6ad6e54 Compare July 21, 2020 05:18

DrahtBot added the Needs rebase label Jul 22, 2020

jonatack reviewed Jul 24, 2020

View reviewed changes

fanquake added the Up for grabs label Mar 22, 2022

fanquake closed this Mar 22, 2022

hebasto mentioned this pull request Nov 30, 2022

Accurately account for mempool index memory #26614

Closed

maflcko removed Up for grabs Needs rebase labels Dec 1, 2022

bitcoin locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accurately account for mempool index memory #18086

Accurately account for mempool index memory #18086

sipa commented Feb 7, 2020 •

edited

Loading

sipa commented Feb 7, 2020 •

edited

Loading

DrahtBot commented Feb 7, 2020 •

edited

Loading

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

laanwj commented Mar 27, 2020

hebasto commented May 3, 2020

hebasto commented May 14, 2020

hebasto Jul 10, 2020

DrahtBot commented Jul 22, 2020

jonatack left a comment

adamjonas commented Aug 14, 2020 •

edited

Loading

hebasto commented Oct 13, 2020 •

edited

Loading

sipa commented Oct 13, 2020

DrahtBot commented Dec 15, 2021

DrahtBot commented Mar 21, 2022

fanquake commented Mar 22, 2022

hebasto commented Nov 30, 2022

	static AccountingAllocator select_on_container_copy_construction() { return AccountingAllocator(); }
	AccountingAllocator select_on_container_copy_construction() const { return AccountingAllocator(); }

Accurately account for mempool index memory #18086

Accurately account for mempool index memory #18086

Conversation

sipa commented Feb 7, 2020 • edited Loading

sipa commented Feb 7, 2020 • edited Loading

DrahtBot commented Feb 7, 2020 • edited Loading

Conflicts

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

JeremyRubin commented Feb 7, 2020

sipa commented Feb 7, 2020

laanwj commented Mar 27, 2020

hebasto commented May 3, 2020

hebasto commented May 14, 2020

hebasto Jul 10, 2020

Choose a reason for hiding this comment

DrahtBot commented Jul 22, 2020

jonatack left a comment

Choose a reason for hiding this comment

adamjonas commented Aug 14, 2020 • edited Loading

hebasto commented Oct 13, 2020 • edited Loading

sipa commented Oct 13, 2020

DrahtBot commented Dec 15, 2021

DrahtBot commented Mar 21, 2022

fanquake commented Mar 22, 2022

hebasto commented Nov 30, 2022

sipa commented Feb 7, 2020 •

edited

Loading

sipa commented Feb 7, 2020 •

edited

Loading

DrahtBot commented Feb 7, 2020 •

edited

Loading

adamjonas commented Aug 14, 2020 •

edited

Loading

hebasto commented Oct 13, 2020 •

edited

Loading