FindByte across block-sized buffer
#34046
The benchmark was updated to start from different file positions and iterate with a stride of ~32kb to exercise a few “random” places.
Found while reviewing #34044
Note: this PR started off as making FindByte more realistic, but based on #34044 (comment) I decided to remove it, and after #34046 (comment) it was reverted to its original state
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34046.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
🚧 At least one of the CI tasks failed.
Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/20113425463/job/57716719394
LLM reason (✨ experimental): bench_sanity_check test segfault caused CI failure.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
42+ assert(bf.GetPos() == found_index);
43+ }
44 });
45-
46 assert(file.fclose() == 0);
47+ fs::remove(path);
Code Review ACK 031ee0b22c4ee4f04f66cb8f0be534b404dbe73c
The new benchmark is more realistic
25- uint8_t data[file_size] = {0};
26- data[file_size - 1] = 1;
27- file << data;
28+ std::vector data(file_size, std::byte{0});
29+ data[found_index] = target;
30+ file.write_buffer(data);
I don’t understand this either. There is no obfuscation, so the two are the same either way?
I’d prefer to just keep the << in high level code, unless there is a reason not to.
To use span-serialization, you can just type std::span{bla}, see also git grep '<< std::span{'.
I don’t think it makes sense to use write_buffer for places where span-serialization is wanted. Otherwise, this leads to inconsistent code, such as:
0file << int_val;
1file.write_buffer(vec_val);
2file << int_other_val;
Also, this mutating the buffer (possibly) makes it doubly confusing.
this mutating the buffer
There was no obfuscation enabled, so no mutation was happening.
don’t think it makes sense to use write_buffer for places where span-serialization is wanted
I dislike magical operators and needless serialization when we want to directly write content, but I’m also fine with file << std::span{data} - pushed.
The current benchmark doesn't represent a realistic scenario:
* it only checks 200 bytes, while in reality we have 8 MB: https://github.com/bitcoin/bitcoin/blob/2c44c41984e0a170cf38d595635a9e861854573c/src/validation.cpp#L5009
* it only checks the very last position (which is basically the missing case), while in reality it's a circular buffer, so the magic bytes can be anywhere
* plain array was needlessly serialized instead of written directly
The benchmark was updated to start from different file positions and iterate with a stride of ~32kb to exercise a few "random" places.
review ACK 93941dc74be8817463e024e5aa11ed0638fdc9f3 🐪
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 93941dc74be8817463e024e5aa11ed0638fdc9f3 🐪
3Yt2ihRdgRtgrO4buTQEVh9tmEonYfAwnXmc8GMkuzgWbdXqsilW8jJP/6QMesry75/BUwnPrr7ognDzINKfsAw==
reindex got slower, while even the updated benchmark showed a speedup, since current blocks always contain the header in the first position.
Concept NACK.
The existence of a benchmark does not imply it’s worth efforts for improving the code further. It’s still useful to see it doesn’t regress.
It’s not that we don’t care about FindByte’s performance. It would probably be unacceptable to make it 100x slower. But as-is, it’s good enough.