This function was doing millions of unnecessary heap allocations during IBD.
I’m start to catalog unnecessary heap allocations as a pet project of mine: as-zero-as-possible-alloc IBD. This is one small step.
before:
after:
~should I type alias this?~ I type aliased it
This is a part of the Zero Allocations Project #18849 (ZAP1). This code came up as a place where many allocations occur.
src/test/fuzz/script.cpp
https://github.com/bitcoin/bitcoin/blob/844d2070a2c0106bb7a54be5cad7d4da4d9cd55e/src/test/fuzz/script.cpp#L39-L40
9@@ -10,8 +10,9 @@
10 #include <script/script.h>
11 #include <serialize.h>
12 #include <span.h>
13+#include <prevector.h>
14
15-bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
16+bool CompressScript(const CScript& script, prevector<33, unsigned char> &out);
Any reason not to do this for decompression?
nope, I could do that as well.
15-bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
16+/**
17+ * This saves us from making many heap allocations when serializaing
18+ * and deserializaing compressed scripts
19+ */
20+typedef prevector<33, unsigned char> CCompressedScript;
nit: How about using CompressedScript = prevector<33, unsigned char>;
“Do not prefix class names with C.”
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
78+ auto size = GetSpecialScriptSize(nSize);
79+ // if size > sizeof(CompressedScript) this would incur a
80+ // heap allocation in the CompressedScript prevector.
81+ // bump the CompressedScript prevector size if this ever
82+ // happens.
83+ assert(size <= sizeof(CompressedScript));
This line is a little frightening to me.
Is this something we know at compile time and can static_assert? Otherwise we should just resize and take the performance hit here.
Performance degradation is much better than crash fail :)
GetSpecialScriptSize was ever changed, but I just tried chaging the 32 value to 34 and the assert wasn’t triggered. so now I’m wondering if there should be better test coverage here.
15-bool CompressScript(const CScript& script, std::vector<unsigned char> &out);
16+/**
17+ * This saves us from making many heap allocations when serializaing
18+ * and deserializaing compressed scripts
19+ */
20+using CompressedScript = prevector<33, unsigned char>;
Can you explain where 33 comes from?
oh yeah good idea
Use a prevector for stack allocation instead of heap allocation during
script compression and decompression. These functions were doing
millions of unnecessary heap allocations during IBD.
We introduce a CompressedScript type alias for this prevector. It is
size 33 as that is the maximum size of a compressed script.
Fix the DecompressScript header to match the variable name from
compressor.cpp
Signed-off-by: William Casarin <jb55@jb55.com>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
doing some tests I’m seeing a very tiny improvement in tmpfs/memdisk reindex up to height 200k:
but could just be luck :shrug: would be interesting to test this to even higher heights.
0n Δn=6 Δn+1 ms
11 -3135 -581 219706 zeroalloc-mem-200k
22 -2554 -981 220287 zeroalloc-mem-200k
33 -1573 -100 221268 zeroalloc-mem-200k
44 -1473 -88 221368 master-mem-200k
55 -1385 -1385 221456 master-mem-200k
66 222841 master-mem-200k
I also traced the number of calls to CompressScript: around 57248988 calls at height 200k
I used
./src/bitcoind -datadir=/run/bitcoin -reindex -connect=127.0.0.2 -assumevalid=0 -printtoconsole=1