Signed-off-by: Anthony Fieroni bvbfan@abv.bg
Enabling moving will prevent unwanted copies which can downside performance. Using std::array will help moving semantics to be done, otherwise COutPoint will not be fully movable but it will make exclusive copy on its hashes. base_blob has advantage from std::array by its iterators, base_uint is protected derived from std::array just like C array approach. Overall this patch tends to make improvements in speed and memory overhead.
This is currently failing 7/10 of the Travis tests.
Overall this patch tends to make improvements in speed and memory overhead.
Can you please post your speed and memory benchmarking results.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This is currently failing 7/10 of the Travis tests.
Can you check what the problem is, it looks like some cache issue?
Can you please post your speed and memory benchmarking results.
What results you expects, running bench or perf with / without patch ?
None of these classes have dynamically allocated memory. A move for them should be identical to a copy. How do you expect that there could be any performance gains?
It shouldn't be copy, about me, as i read http://timepp.github.io/doc/cpp14/array.cons.html
It should use less memory, you have right on that integers have practically no difference on copy vs move, but optimization is still relevant to me, since it's base classes which impact on derived one as well.
This PR contains a lot of std::move:s on trivially-copyable types (uint256, uint160 and COutPoint) which don't have any effect.
Same goes for std::move:s on const reference arguments.
I agree, i use to make it by unique_ptr but in many places it's used like
memcpy(&hash, data, sizeof(hash))
I found some places but not all, some tests segfaults.
It isn't obvious to me how this PR makes a material improvement either to the runtime behaviour or the long term maintenance of the codebase.
PR aims to optimize hashes, uints and outpoints by moving. I resolve remaining problems of expecting continuous bytes, in ComputeMerkleRoot double hashing is make by SHA256D64 but in new implementation bytes are not continuous anymore, so i replace it by
for (std::size_t i = 0, j = 0; i < hashes.size(); i += 2) {
hashes[j++] = Hash(hashes[i].begin(), hashes[i].end(),
hashes[i+1].begin(), hashes[i+1].end());
}
All tests passes but can it be problematic vs SHA256D64?
Rebase to master
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
Sorry for the flood but some tests fails without a reason, it looks like.
NAK. I'm just not seeing a material benefit justifying the expenditure of review efforts.
Closing for now. This doesn't compile on appveyor and there seems to be disagreement about whether to do this at all. Probably not worth to put in further effort.
@MarcoFalke, you can see at Travis it compile and most tests pass (some weird) not. I'm not familiar with appveyor but it should be something trivial. Edit: I understand that patch became huge, but that was side effect of using memcpy + sizeof + & on object. Also it tries to fix that as well.