-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
improve MallocUsage() accuracy #28531
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
The results of the old and new versions of this function should differ only slightly; it would be bad if the new one gave very different results, because node operators might find too much memory being consumed, or (not as bad) not enough. To manually test this, I ran |
Changing to draft because CI on a couple of the platforms failed the new test, so the physical memory calculation will need to be different on those platforms, I try to figure that out. |
I can reproduce the same results with glibc 2.31 and 2.38, in 32bit and 64bit. I don't think it needs to be perfect on all platforms, I think your new calculation is fine. I played a bit with a reproducer, here is mine: https://godbolt.org/z/s971sbnhG I refactored your |
🤔 There hasn't been much activity lately and the CI seems to be failing. If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in. |
b469486
to
04f3fbe
Compare
Force-pushed to 04f3fbe - rebase only. (CI is still expected to fail, I'm working on that now.) |
cee653f
to
86b71ec
Compare
6bfdad9
to
58ebf90
Compare
@martinus
On earlier runs, some other tests have failed, but I'm not sure if those are spurious failures. I'm not sure what to do now, any thoughts or suggestions are welcome! |
@LarryRuane There is no reason to assume that the memory allocation overhead on those platforms, with substantially different C libraries, would match the formula we inferred on x86_64 and arm Linux. If our goal is actually accurate memory usage calculations on all systems, we will need to derive a formula for each supported system separately. |
58ebf90
to
e0fa518
Compare
@sipa - I don't think the test failures could be due to differences in memory allocation overhead across platforms. The actual memory allocation overhead isn't being tested at all. To do that would probably require a test that uses I don't mind giving up and closing this PR, but as @martinus said in an earlier comment, this calculation doesn't need to be perfect, just better. I think this test failure must be due to differences in [UPDATE: the following discussion about I thought I had it figured out; I verified with the debugger that as the test adds the 10k entries to the two maps, it has to repeatedly grow the bucket array. The allocation sizes (in bytes) for the bucket array that I saw here on Ubuntu were: 104, 232, 472, 1026, 2056, 4328, 8872, 18856, 40696, 82184.... The tradeoff one makes when using the pool resource allocator is that memory allocations that are freed but then those same sizes not allocated again, those allocations are, in effect, leaked (until the resource is destroyed). The pool resource works great when freeing and allocating the same sizes (rounded up to the alignment, usually 8 bytes) repeatedly. This is, of course, the case for the individual map entries. So the latest force-push simply calls It's difficult when CI fails on platforms that I don't have access to. Do others have a solution to that? Can I install macOS on Ubuntu or Windows 10 (I have both) as a VM? Maybe I can set up my Windows laptop to build and run the debugger, but that seems like a lot of work. |
@LarryRuane The problem here is that the The inconcsistency now comes when using So, with your new MallocUsage the numbers are most likely more correct, but this seems to trigger the incorrect underestimation for |
@martinus - thanks, that makes perfect sense! I hadn't thought of the possibility of the nodes having two pointers instead of one (double instead of single linked list). Seems like a bad design, but anyway. I don't think there would be much benefit to fixing |
e0fa518
to
c6cba06
Compare
c3d1f4b
to
eec7112
Compare
eec7112
to
ee3683d
Compare
Turned into draft for now, due to failing CI. If this is no longer a WIP, you can move it out of draft. |
ee3683d
to
9bc440d
Compare
9bc440d
to
aeac0f2
Compare
NOTE, this PR isn't ready to merge; at the very least it has some debug prints that need to be removed. CI was failing on some platforms (Windows and macOS), in This test calls the test utility function After With this PR, that transaction was not accepted on Windows and macOS. Some debug prints show that on those platforms, when After trying a few fixes to the test that didn't work, what finally worked (the current branch) is, to have This attempt didn't work the first time, however, because the mempools wouldn't reliably sync in @glozow and @theStack, can you let me know your opinion? You two have worked a lot in this area. |
@LarryRuane: Interesting problem (and very creative solution with the RBF tx :)). Fwiw, on one of my machines running OpenBSD 7.5, the same behaviour as you described occurs on commit 663e501: after One idea I had to solve this at the However, is this even necessary to be solved in |
aeac0f2
to
f758e73
Compare
Your patch is much better, @theStack, thanks! It's simpler and doesn't need that weird |
Co-authored-by: Pieter Wuille <[email protected]> Co-authored-by: Martin Leitner-Ankerl <[email protected]>
f758e73
to
1fcbebc
Compare
CI passed, thanks @theStack. I just force-pushed swapping of the two commits, the test commit has to be first for each commit to pass CI. |
The
MallocUsage()
function takes an allocation size as an argument and returns the amount of physical memory consumed, which is greater due to memory allocator overhead and alignment. It was first added in 2015 (first commit of #6102), but its accuracy has degraded as memory allocation libraries have evolved. It's used when it's important that large data structures, such as the coins cache and mempool, should use a predictable, configurable (limited) amount of physical memory (see the-dbcache
and-maxmempool
configuration options), as well as a few other places.sipa figured out a concise, efficient expression that this function can use, and that's what's implemented here.
Also add a unit test, which is more helpful than usual in this case since platforms, operating systems, and libraries vary significantly in this area.