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.
Could you please add a benchmark to ./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);
Just to throw out the idea, mmap wouldn't pay off right?
I don't think that's a win here, as the entire block is read consecutively - could even be slower as it'd have to create and destroy the mapping. Also it's not portable.
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) {
Shouldn't this compare against the serialization flags of the block on disk? Currently you are assuming that all blocks are serialized as witness blocks on disk, but this is not true for all "early" blocks.
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.
Leaving out the less common edge case (non-witness peers) seems fine for now.
Unsure if we care, but we could also check whether SegWit was active for the requested block, and if not, we can serve without deserialization even when witnesses are not requested.
Concept ACK. Would be nice to see how much the additional savings are on top of #13098.
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);
Probably want to check the size is sane before we do this.
Good point. What constant would be appropriate here? Edit: I'll go with MAX_SIZE from serialize.h.
Another thing I wondered here: what is the C++11 proper way to allocate a vector (or a RAII memory area) without zeroing it? I think that's unnecessary here.
That was a bad idea: even though we handle errors while reading, as this data is sent directly over P2P, zeroing is defense-in-depth against heartbleed-style issues here
utACK except for the below:
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());
Should probably remove this debug logging
Yes, definitely. I added it while WIP so that people testing this can be sure that the code actually triggers and they're testing the right thing.
Will measure some round trips tomorrow.
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;
I guess this deserialization logic should be within the try {}.
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");
Nit: braces around then-branch if on a separate line.
utACK after squash.
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:
With patch:
real 63m51.273s
Without patch:
real 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) {
nit, space after catch } 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?
To avoid heartbleed-type leaks as this data goes directly over the network.
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.
@jonasschnelli That's really strange. As reported, I did see some actual speed-ups where using this. Maybe someone else can try some measurements, or we should just close, I don't know.
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 think we should definitively look into why it is slower to sync, since that indicates a problem (potentially in our code) exists elsewhere.
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;
with patch:
real 0m55.928s
real 0m55.986s
real 0m55.913s
real 0m55.844s
real 0m55.790s
without patch (using the commit before):
real 2m47.673s
real 2m46.329s
real 2m46.634s
real 2m46.413s
real 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
@jonasschnelli Did you have a chance to look into why your result was unexpected?
@Marcofalke: no. I haven't but I'm willing to do as soon as someone could confirm my results (SSD test).
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.
@jonasschnelli I coulnd't find the branch you were using. Mind to share, otherwise I can't reproduce?
Same experiment as #13151 (comment) on i.MX6Q ARM board w/ USB2 spinning disk:
with patch:
real 10m18.368s
real 11m14.600s
real 10m12.006s
real 10m21.668s
real 10m11.070s
without patch (using the commit before):
real 27m30.574s
real 26m27.591s
real 25m38.311s
real 25m36.661s
real 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.
squashed, no other changes -- 9893e712e9e04e8b9478e36e0b5d843899540bd2 → 0bf431870e45d8e20c4671e51a782ebf97b75fac
I guess my setup was either faulty or there is a performance loss with that particular setup (>1000MB/s IO r&w on very fast CPUs). However, this PR is a clear and significant win!
@jonasschnelli I can't explain why, but you might want to try to drop the files cached in memory with sync && sudo sh -c 'echo 3 > /proc/sys/vm/drop_caches'. For me this was speeding up the sync on an ssd.