In ProcessGetBlockData, send the block data directly from disk if type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the on-disk format matches the network format.
This is expected to increase performance because a deserialization and subsequent serialization roundtrip is avoided.
./src/bench/checkblock.cpp, so it is easier to see how much this improves?
Sure, though I’m not sure how to do that; none of the benches actually uses ReadBlockFromDisk, I would have to set up a fake block index or such.
(I also don’t think it will work on block413567 as-is because it has no magic/size header, and is not a file on disk, though it’s easy enough to write a temporary file of course). [When doing this from memory there’s effectively nothing to benchmark, too.]
1145+ HexStr(messageStart, messageStart + CMessageHeader::MESSAGE_START_SIZE));
1146+ }
1147+
1148+ try {
1149+ block.resize(nSize);
1150+ filein.read((char*)block.data(), nSize);
mmap wouldn’t pay off right?
Concept ACK.
Couldn’t we serve corrupted blocks?
1142@@ -1142,60 +1143,71 @@ void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1143 std::shared_ptr<const CBlock> pblock;
1144 if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) {
1145 pblock = a_recent_block;
1146+ } else if (inv.type == MSG_WITNESS_BLOCK) {
Yes I’m not convinced this logic is correct. It seems to work, though, even for the initial blocks.
Edit: What is the operation to convert from a non-witness block to witness block with no witnesses? I suppose this could still be done without a full round-trip?
@laanwj It’s always correct to give the raw blocks we store to peers that ask for witnesses (even if the block does not have a witness).
Converting extended format to basic format is a lot more complicated. You could have a special CTransaction which skips the witness fields instead of reading/deserializing them, but I don’t see how to do it without going through some form of serialization code.
@sipa Thanks.
Converting extended format to basic format is a lot more complicated. You could have a special CTransaction which skips the witness fields instead of reading/deserializing them, but I don’t see how to do it without going through some form of serialization code.
Right, that case should fall back to deserialization->serialization right now. I don’t think we can do much better there. Not sure it’s even worth optimizing, there won’t be many new clients being synced with pre-segwit versions.
Couldn’t we serve corrupted blocks?
Yes, that’s a possibility, though only if the underlying storage is corrupted. I’ve posited the idea to add a CRC32C to the on-disk blocks at some point (which is quick to verify, especially with specialized instructions, and should protect against accidental corruptions), but that’s quite an invasive change. It’s something that could be done later.
The only option to verify with the current information would be to do a Merkle tree check - which could be done without deserialization but it’s not pretty… (and a serious overhead SHA256-hashing)
The only option to verify with the current information would be to do a Merkle tree check
I don’t see that we currently do this, so it wouldn’t make anything worse here.
I think serving a corrupted block if our state is corrupted is fine, the peer will just disconnect us and go get the block from someone else, seems pretty harmless!
This is a much smaller change than I was expecting– in particular I forgot there was a size, light review ACK.
1144+ HexStr(msg_start_in, msg_start_in + CMessageHeader::MESSAGE_START_SIZE),
1145+ HexStr(messageStart, messageStart + CMessageHeader::MESSAGE_START_SIZE));
1146+ }
1147+
1148+ try {
1149+ block.resize(nSize);
1142@@ -1142,60 +1143,71 @@ void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1143 std::shared_ptr<const CBlock> pblock;
1144 if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) {
1145 pblock = a_recent_block;
1146+ } else if (inv.type == MSG_WITNESS_BLOCK) {
1147+ // Fast-path: in this case it is possible to serve the block directly from disk,
1148+ // as the network format matches the format on disk
1149+ LogPrintf("debug: Serving raw block directly from disk: %s\n", pindex->ToString());
utACK 4c790dff7481d1464a906ad6b17a3179a7da3431
This would probably also speedup an external indexing daemon via p2p (see experiment in https://github.com/jonasschnelli/bitcoincore-indexd [very WIP])
Here a flamegraph of serving the first 200k blocks via p2p localhost (though the real deser/ser starts probably at higher up in the chain).
1135+ return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
1136+ }
1137+
1138+ CMessageHeader::MessageStartChars msg_start_in;
1139+ unsigned int nSize;
1140+ filein >> msg_start_in >> nSize;
OK, thanks for review everyone, removed WIP tag and pushed commits with the following changes:
Will squash if no further issues.
This would probably also speedup an external indexing daemon via p2p (see experiment in https://github.com/jonasschnelli/bitcoincore-indexd [very WIP])
Yeah - I saw in #13098 that @MarcoFalke had also optimized the zmq full-block notification part. I’m not sure that is any critical path performance-wise (maybe for your indexer?) but if so I’ll leave that for a later PR.
Here a flamegraph of serving the first 200k blocks via p2p localhost (though the real deser/ser starts probably at higher up in the chain).
Did you forget to post the link?
1142@@ -1142,60 +1143,70 @@ void static ProcessGetBlockData(CNode* pfrom, const Consensus::Params& consensus
1143 std::shared_ptr<const CBlock> pblock;
1144 if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) {
1145 pblock = a_recent_block;
1146+ } else if (inv.type == MSG_WITNESS_BLOCK) {
1147+ // Fast-path: in this case it is possible to serve the block directly from disk,
1148+ // as the network format matches the format on disk
1149+ std::vector<uint8_t> block_data;
1150+ if (!ReadRawBlockFromDisk(block_data, pindex, chainparams.MessageStart()))
1151+ assert(!"cannot load block from disk");
Running with e0223ebf0c58f7beedea91df48e9586154cd4436 and just looking at the wall clock time for reading+optional deserialization shows for me on an ssd:
Edit: Note that this was done with full fake blocks and not real blocks from the network.
Thanks for benchmarking @MarcoFalke.
I used a patched version of @jonasschnelli’s bitcoincore-indexd to benchmark the time for fetching block 0..473600 through P2P, with no processing client-side. The result is:
0With patch:
1real 63m51.273s
2
3Without patch:
4real 70m28.956s
10% speedup. And in my case is the blocks are on a slow harddisk. I expect the gains to be more significant in case of a faster storage medium, or slower CPU.
1151+ blk_size, MAX_SIZE);
1152+ }
1153+
1154+ block.resize(blk_size); // Zeroing of memory is intentional here
1155+ filein.read((char*)block.data(), blk_size);
1156+ } catch(const std::exception& e) {
} catch (....
1149+ if (blk_size > MAX_SIZE) {
1150+ return error("%s: Block data is larger than maximum deserialization size for %s: %s versus %s", __func__, pos.ToString(),
1151+ blk_size, MAX_SIZE);
1152+ }
1153+
1154+ block.resize(blk_size); // Zeroing of memory is intentional here
Zeroing of memory is intentional
Why?
Did 10 rounds of requesting blocks in range 490'000 up to 500'000 on master and got. Setup:
https://github.com/laanwj/bitcoincore-indexd-g -O2):95211ms in avg. (all rounds where very similar and there was no need to exclude the first round)
exemption
-g O2):101051ms in avg.
I can’t figure out why this PR performs ~6% slower and I double checked the comparison by manually rolling back from the head of this PR to 598db389c33e5e90783ef1223df2eeab095ed622 and back to head. Also added the LogPrintfs back to ensure I’m using the “fast track” (removed again during benchmarking).
sidenode: found out that -debug=net (which was disabled during the rounds reported above) account for a 3% slowdown for the above test-scenario.
If someone wants to compare master against this PR built in the same environment:
PR: https://bitcoin.jonasschnelli.ch/build/600 master: https://bitcoin.jonasschnelli.ch/build/599
I updated my benchmark to also include the time it takes to Make (serialize) the net message:
I did the same experiment as @jonasschnelli, a modified bitcoincore-indexd that requests block 490000..500000 (https://github.com/laanwj/bitcoincore-indexd/tree/bench). Tried both cases 5 times;
0with patch:
1real 0m55.928s
2real 0m55.986s
3real 0m55.913s
4real 0m55.844s
5real 0m55.790s
6
7without patch (using the commit before):
8real 2m47.673s
9real 2m46.329s
10real 2m46.634s
11real 2m46.413s
12real 2m46.458s
~66% speedup. This is with a spinning rust disk not a SSD. This was done on a different computer than my previous test.
I think this is a clear benefit for spinning disk and probably also for ssd in non absurd localhost cases.
utACK
Sure, will do
On Mon, May 14, 2018, 11:41 Jonas Schnelli notifications@github.com wrote:
@MarcoFalke https://github.com/MarcoFalke: no. I haven’t but I’m willing to do as soon as someone could confirm my results (SSD test).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13151#issuecomment-388863028, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv9-r1eVW0U-ZpIGMl9OABHCU7k27ks5tyaWYgaJpZM4TvWjI .
For clarity: 598db means master@598db and 9893e means this pull request. I used @laanwj’s branch of bitcoincore-indexd.
Same experiment as #13151 (comment) on i.MX6Q ARM board w/ USB2 spinning disk:
0with patch:
1real 10m18.368s
2real 11m14.600s
3real 10m12.006s
4real 10m21.668s
5real 10m11.070s
6
7without patch (using the commit before):
8real 27m30.574s
9real 26m27.591s
10real 25m38.311s
11real 25m36.661s
12real 25m40.902s
Seems to help even with a slow CPU and slow I/O.
In `ProcessGetBlockData`, send the block data directly from disk if
type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the
on-disk format matches the network format.
This is expected to increase performance because a deserialization and
subsequent serialization roundtrip is avoided.
sync && sudo sh -c 'echo 3 > /proc/sys/vm/drop_caches'. For me this was speeding up the sync on an ssd.