We recently (in #28906) had OOM problems due to incorrect memory usage estimation. When PoolResource is used the estimation can be brittle because when implementing memusage estimation for a container it is not obvious when the pool can be used.
As suggested here #28906 (comment), to prevent these problems in future we can simply let the PoolResource do all the accounting. Then the memory usage estimation is always accurate, even when the pool’s memory chunks cannot be used.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
0@@ -0,0 +1,30 @@
1+// Copyright (c) 2015-2023 The Bitcoin Core developers
Per iwyu in https://cirrus-ci.com/task/6429107576111104 (“run ci”):
add #include <malloc_usage.h> to src/test/disconnected_transactions.cpp, src/test/pool_tests.cpp, and src/txmempool.cpp
grep the output also for memusage.h
nit, start year, and I think we’re doing this now to avoid future need to update
0// Copyright (c) 2023-present The Bitcoin Core developers
277@@ -266,6 +278,11 @@ class PoolResource final
278 {
279 return m_chunk_size_bytes;
280 }
281+
282+ [[nodiscard]] size_t DynamicMemoryUsage() const
PoolMemoryUsage) to distinguish it while grepping from the DynamicMemoryUsage elsewhere in the codebase?
Concept ACK
A couple of minor comments/questions on first look, feel free to defer until you need to update for more substantive feedback.
11+namespace memusage {
12+
13+/** Compute the total memory used by allocating alloc bytes. */
14+static inline size_t MallocUsage(size_t alloc)
15+{
16+ // Measured on libc6 2.19 on Linux.
That way it becomes usable in other code like allocators/pool
That way all containers that use the pool have accurate memory tracking.
Add test to show memory is accurately tracked, even when nodes can't be allocated by the pool.
The more accurate memory estimation interferes shows that our memory estimation for Windows
is off, as the real allocated memory is much higher. This adapts the memusage_test test so it still
works with the more correct estimation.
196@@ -197,6 +197,7 @@ BITCOIN_CORE_H = \
197 key_io.h \
198 logging.h \
199 logging/timer.h \
200+ malloc_usage.h \
