util: Faster std::byte (pre)vector (un)serialize #29114

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2312-fast-byte-vec-ser- changing 2 files +8 −10
  1. maflcko commented at 3:30 pm on December 19, 2023: member

    Currently, large vectors of std::byte are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them.

    On my system this gives a 10x speedup for ./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial, when std::byte are used:

     0diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp
     1index 2524e215e4..76b16bc34e 100644
     2--- a/src/bench/prevector.cpp
     3+++ b/src/bench/prevector.cpp
     4@@ -17,7 +17,7 @@ struct nontrivial_t {
     5 static_assert(!std::is_trivially_default_constructible<nontrivial_t>::value,
     6               "expected nontrivial_t to not be trivially constructible");
     7 
     8-typedef unsigned char trivial_t;
     9+typedef std::byte trivial_t;
    10 static_assert(std::is_trivially_default_constructible<trivial_t>::value,
    11               "expected trivial_t to be trivially constructible");
    12 
    

    However, the optimization does not cover signed char. Fix that as well.

  2. DrahtBot commented at 3:30 pm on December 19, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK sipa, TheCharlatan, achow101
    Concept ACK jamesob, martinus

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Utils/log/libs on Dec 19, 2023
  4. jamesob commented at 4:28 pm on December 19, 2023: member
    Concept ACK. I better blow the dust off of bitcoinperf…
  5. maflcko commented at 4:39 pm on December 19, 2023: member

    Benchmarks are also done by https://corecheck.dev/bitcoin/bitcoin/pulls/29114 :)

    Though, to test this, one would have to manually apply the diff either way, so local running is probably the easiest.

    (I don’t think std::vector<std::byte> serialization is used anywhere, where performance matters or is measured)

  6. Faster std::byte (pre)vector (un)serialize facaa14785
  7. Allow int8_t optimized vector serialization
    int8_t serialization is allowed, but not the optimized vector
    serialization. Fix that.
    fab41697a5
  8. maflcko force-pushed on Dec 22, 2023
  9. maflcko marked this as ready for review on Dec 22, 2023
  10. maflcko commented at 9:01 am on December 22, 2023: member
    Rebased and taken out of draft after the dependency #29056 was merged
  11. maflcko commented at 10:10 am on December 22, 2023: member
  12. martinus commented at 11:59 am on December 22, 2023: contributor
    Oh no! You are using C++20 so I finally need to learn its features… Code review ACK and ran tests & benchmarks, the performance difference for std::byte before and after is a factor 14 for me (143.39 ns/op down to 10.03 ns/op).
  13. maflcko commented at 1:43 pm on December 22, 2023: member

    For the code here, there shouldn’t be anything new to learn. It it just a different syntax, for something that can be written with enable_if as well. See https://www.foonathan.net/2016/09/cpp14-concepts/#conclusion

    Emulation of the requires clause is possible using almost the same syntax with std::enable_if.

  14. sipa commented at 3:25 pm on December 22, 2023: member
    utACK fab41697a5448ef2861f65795bd63a4ccdda6a40
  15. DrahtBot requested review from jamesob on Dec 22, 2023
  16. DrahtBot added the label CI failed on Jan 15, 2024
  17. maflcko commented at 10:00 am on January 16, 2024: member
    CI failure can be ignored
  18. in src/serialize.h:858 in facaa14785 outdated
    852@@ -856,10 +853,9 @@ void Unserialize(Stream& is, prevector<N, T>& v)
    853 template <typename Stream, typename T, typename A>
    854 void Serialize(Stream& os, const std::vector<T, A>& v)
    855 {
    856-    if constexpr (std::is_same_v<T, unsigned char>) {
    857+    if constexpr (BasicByte<T>) { // Use optimized version for unformatted basic bytes
    858         WriteCompactSize(os, v.size());
    859-        if (!v.empty())
    860-            os.write(MakeByteSpan(v));
    861+        if (!v.empty()) os.write(MakeByteSpan(v));
    


    TheCharlatan commented at 3:53 pm on February 7, 2024:
    Nit: No need for these formatting-only changes?

    maflcko commented at 10:33 am on February 8, 2024:
    Ah, will undo if I retouch.
  19. TheCharlatan approved
  20. TheCharlatan commented at 4:36 pm on February 7, 2024: contributor

    ACK fab41697a5448ef2861f65795bd63a4ccdda6a40

    It’s a nice cleanup, but I don’t observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of fab41697a5448ef2861f65795bd63a4ccdda6a40.

  21. maflcko commented at 10:35 am on February 8, 2024: member

    It’s a nice cleanup, but I don’t observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of fab4169.

    Can you clarify if there is no speedup, or just a smaller one? Also, what it the speed for uint8_t on master vs std::byte on master vs std::byte after this pull?

  22. achow101 commented at 6:08 pm on February 8, 2024: member
    ACK fab41697a5448ef2861f65795bd63a4ccdda6a40
  23. achow101 merged this on Feb 8, 2024
  24. achow101 closed this on Feb 8, 2024

  25. maflcko deleted the branch on Feb 8, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me