Skip to content

Add pool based memory resource#25325

Merged
achow101 merged 5 commits intobitcoin:masterfrom
martinus:2022-06-very-not-scary-NodePoolResource
Apr 20, 2023
Merged

Add pool based memory resource#25325
achow101 merged 5 commits intobitcoin:masterfrom
martinus:2022-06-very-not-scary-NodePoolResource

Conversation

@martinus
Copy link
Contributor

@martinus martinus commented Jun 10, 2022

A memory resource similar to std::pmr::unsynchronized_pool_resource, but optimized for node-based containers. The goal is to be able to cache more coins with the same memory usage, and allocate/deallocate faster.

This is a reimplementation of #22702. The goal was to implement it in a way that is simpler to review & test

  • There is now a generic PoolResource for allocating/deallocating memory. This has practically the same API as std::pmr::memory_resource. (Unfortunately I cannot use std::pmr because libc++ simply doesn't implement that API).

  • Thanks to sipa there is now a fuzzer for PoolResource! On a fast machine I ran it for ~770 million executions without finding any issue.

  • The estimation of the correct node size is now gone, PoolResource now has multiple pools and just needs to be created large enough to have space for the unordered_map nodes.

I ran benchmarks with #22702, mergebase, and this PR. Frequency locked Intel i7-8700, clang++ 13.0.1 to reindex up to block 690000.

bitcoind -dbcache=5000 -assumevalid=00000000000000000002a23d6df20eecec15b21d32c75833cce28f113de888b7 -reindex-chainstate -printtoconsole=0 -stopatheight=690000

The performance is practically identical with #22702, just 0.4% slower. It's ~21% faster than master:

Progress in Million Transactions over Time(2)

Size of Cache in MiB over Time
Note that on cache drops mergebase's memory doesnt go so far down because it does not free the CCoinsMap bucket array.

Size of Cache in Million tx over Time(1)

@martinus martinus marked this pull request as draft June 10, 2022 08:08
@martinus martinus force-pushed the 2022-06-very-not-scary-NodePoolResource branch 2 times, most recently from 6de57f7 to d3c437f Compare June 10, 2022 17:23
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 10, 2022

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK john-moffett, jonatack, LarryRuane, achow101
Stale ACK sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@martinus martinus force-pushed the 2022-06-very-not-scary-NodePoolResource branch 3 times, most recently from 850fe38 to ba38016 Compare June 11, 2022 05:03
@martinus martinus force-pushed the 2022-06-very-not-scary-NodePoolResource branch 5 times, most recently from 6932541 to b56c223 Compare June 11, 2022 14:37
@martinus martinus force-pushed the 2022-06-very-not-scary-NodePoolResource branch 4 times, most recently from 98bb268 to d196da1 Compare June 12, 2022 19:32
@martinus martinus changed the title [WIP] Add pool based memory resource Add pool based memory resource Jun 13, 2022
@martinus martinus marked this pull request as ready for review June 13, 2022 06:19
@martinus martinus force-pushed the 2022-06-very-not-scary-NodePoolResource branch from d196da1 to 9205b60 Compare June 19, 2022 05:13
@martinus
Copy link
Contributor Author

rebased to 9205b60 with minor fixes in pool.h so it is usable in boost::unordered_map

@DrahtBot DrahtBot requested review from LarryRuane, jonatack and sipa and removed request for LarryRuane March 24, 2023 15:48
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice review work, @LarryRuane.

re-ACK 9f947fc

@DrahtBot DrahtBot requested review from LarryRuane and removed request for LarryRuane March 24, 2023 17:05
@LarryRuane
Copy link
Contributor

ACK 9f947fc

@DrahtBot DrahtBot removed the request for review from LarryRuane March 24, 2023 17:40
@martinus
Copy link
Contributor Author

@sipa could you have another look after my update from d87cb99 -> 9f947fc?

@achow101
Copy link
Member

re-ACK 9f947fc

@DrahtBot DrahtBot removed the request for review from achow101 April 20, 2023 20:11
@sipa
Copy link
Member

sipa commented Apr 20, 2023

Posthumous utACK 9f947fc

@martinus
Copy link
Contributor Author

Wohoo 🎉 Thanks everyone for making this happen!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.