perf: introduce ToBytes in bls.h and use it in all spots trivially possible, convert vecBytes to std::array#6515
Conversation
|
Removed to simplify merge commit message |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/dkgsession.cpp (1)
1024-1026: Consistency forquorumSigmanipulation.
The approach parallels the previous signature manipulation, ensuringToBytesandSetBytesremain consistent for simulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6310321 and d350cbc37ed75995024494cc328f766a427c9907.
📒 Files selected for processing (4)
src/bls/bls.h(14 hunks)src/governance/object.cpp(1 hunks)src/governance/vote.cpp(1 hunks)src/llmq/dkgsession.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/governance/object.cpp
🔇 Additional comments (17)
src/governance/vote.cpp (1)
185-185: Use of SetBytes is consistent and appropriate.
This change keeps the code aligned with the new SetBytes approach for handling BLS data.
src/bls/bls.h (15)
62-62: Consistent invocation of SetBytes.
Using SetBytes in the constructor keeps object creation unified with the broader rename from SetByteVector.
Line range hint 106-119: Ensure proper reset on invalid input.
The new SetBytes method correctly resets the wrapper on short or zero-filled input, preventing malformed states.
139-147: Addition of ToBytes methods is a welcome change.
Returning a fixed-size std::array helps eliminate unnecessary dynamic allocations, improving performance.
167-167: Maintain uniform usage of SetBytes.
Replacing direct references to old methods ensures that all code paths use SetBytes consistently.
179-179: Directly writing array bytes is clear.
Using AsBytes with the result of ToBytes is straightforward for serialization.
193-193: De-serialize using SetBytes.
Switching from SetByteVector to SetBytes for reading from streams keeps the routine consistent with the rest of the refactoring.
198-198: Fallback scheme usage is correct.
Attempting the opposite scheme if the first attempt fails is well-handled here, continuing to rely on SetBytes.
218-218: Efficient verification using memcmp.
Comparing the byte arrays via memcmp ensures a direct check of the internal representation for malleability.
234-234: Hex conversion uses ToBytes effectively.
Extracting bytes for hex-string output further demonstrates the benefit of a fixed-size array.
261-264: Fixed-size array in CBLSIdImplicit.
Returning a std::array<uint8_t, 32> for serialization is consistent with the rest of the new design.
397-397: Refactor to fixed-size vecBytes.
Changing from std::vector<uint8_t> to std::array<uint8_t, BLSObject::SerSize> reduces dynamic allocations in CBLSLazyWrapper.
408-408: Default initialization for vecBytes.
Initializing the array with {0} ensures deterministic behavior for an uninitialized buffer.
450-450: Rebuilding vecBytes from ToBytes.
Writing the new array in Serialize is correct and locks in a consistent internal representation.
498-498: Correct use of obj.SetBytes after retrieval.
Deserializing data from vecBytes continues the consistent usage pattern of the new API.
535-535: ToBytes usage in GetHash.
Generating the hash directly from a fixed-size array clarifies the logic and prevents memory reallocation overhead.
src/llmq/dkgsession.cpp (1)
1019-1021: Properly modifying the in-flight signature bytes.
This simulated "lie" operation showcases the flexible usage of ToBytes/SetBytes, but verify that offset [5] is always valid.
|
it conflicts with my PR #6508 which already had one review iteration by Udjin. |
|
This pull request has conflicts, please rebase. |
…ssible, convert vecBytes to std::array this results in an improved reindex speed as previously roughly 2% of total cycles were just in allocating the vector.
PastaPastaPasta
force-pushed
the
refac-bls-stackify
branch
from
d350cbc to
bbf97d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bls/bls.h (1)
Line range hint
1-591: Excellent performance optimization through stack allocation.The conversion from
std::vectortostd::arrayfor serialization is a well-executed performance improvement that:
- Eliminates heap allocations
- Maintains thread safety
- Preserves backward compatibility
- Introduces no breaking changes
The changes align perfectly with the PR's objective of improving serialization performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d350cbc37ed75995024494cc328f766a427c9907 and 8510351.
📒 Files selected for processing (4)
src/bls/bls.h(15 hunks)src/governance/object.cpp(1 hunks)src/governance/vote.cpp(1 hunks)src/llmq/dkgsession.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/governance/vote.cpp
- src/governance/object.cpp
- src/llmq/dkgsession.cpp
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build (linux64_sqlite, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_nowallet, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_fuzz, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_cxx20, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (arm-linux, arm-linux-gnueabihf, arm-linux)
🔇 Additional comments (5)
src/bls/bls.h (5)
130-133: LGTM! Performance improvement through stack allocation.The new
ToBytesmethod efficiently returns a fixed-size array, eliminating heap allocations during serialization.
243-243: LGTM! Efficient serialization using underlying data.Good use of [[nodiscard]] attribute and direct member access for zero-copy serialization.
441-444: LGTM! Thread-safe serialization update.The serialization code correctly handles the transition to array-based storage while maintaining thread safety through mutex protection.
271-271: LGTM! Consistent usage across derived classes.The derived classes correctly use the new
SetBytesmethod while maintaining backward compatibility through the legacy scheme parameter.Also applies to: 343-343
Line range hint
388-400: Consider adding size validation in constructor.While changing
vecBytestostd::arrayimproves performance, consider adding a static_assert to validateBLSObject::SerSizematches the expected sizes defined in the constants.
| inline void Serialize(Stream& s, const bool specificLegacyScheme) const | ||
| { | ||
| s.write(AsBytes(Span{ToByteVector(specificLegacyScheme).data(), SerSize})); | ||
| s.write(AsBytes(Span{ToBytes(specificLegacyScheme).data(), SerSize})); |
There was a problem hiding this comment.
here's UB: ToBytes returns temporary object and Span doesn't own it.
| s.write(AsBytes(Span{ToBytes(specificLegacyScheme).data(), SerSize})); | |
| const auto bytes{ToBytes(specificLegacyScheme)}; | |
| s.write(AsBytes(Span{bytes.data(), SerSize})); |
| inline bool CheckMalleable(Span<uint8_t> vecBytes, const bool specificLegacyScheme) const | ||
| { | ||
| if (memcmp(vecBytes.data(), ToByteVector(specificLegacyScheme).data(), SerSize)) { | ||
| if (memcmp(vecBytes.data(), ToBytes(specificLegacyScheme).data(), SerSize)) { |
There was a problem hiding this comment.
here's UB. after call data() temporary object is erased.
| if (memcmp(vecBytes.data(), ToBytes(specificLegacyScheme).data(), SerSize)) { | |
| const auto bytes{ToBytes(specificLegacyScheme)}; | |
| if (memcmp(vecBytes.data(), bytes.data(), SerSize)) { |
| CBLSLazyWrapper() : | ||
| vecBytes(BLSObject::SerSize, 0), | ||
| bufLegacyScheme(bls::bls_legacy_scheme.load()) | ||
| vecBytes{0}, |
There was a problem hiding this comment.
nit: Should be changed to avoid confusion
| vecBytes{0}, | |
| vecBytes{}, |
|
|
||
| std::array<uint8_t, SerSize> ToBytes(const bool specificLegacyScheme) const | ||
| { | ||
| return impl.SerializeToArray(specificLegacyScheme); |
There was a problem hiding this comment.
| return impl.SerializeToArray(specificLegacyScheme); | |
| if (!fValid) { | |
| return std::array<uint8_t, SerSize>{}; | |
| } | |
| return impl.SerializeToArray(specificLegacyScheme); |
it works for me with this fix 5b4b333
4 participants
Issue being fixed or feature implemented
we made changes to bls library library to allow std::array based serialization, which resulted in ~10% speedups in serialization in library benchmarks. I noticed today:
pre-this change:
post this change:
this shows that roughly 2.5% of total cycles during reindex are spent on vector new in our bls.h object.
What was done?
Convert to array to avoid dynamic allocation slowdown
How Has This Been Tested?
reindexing atm, looks good so far, and performance is improved as expected
Breaking Changes
None
Checklist: