refactor: serialization simplifications#28203
Merged
Merged
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
jonatack
reviewed
Aug 2, 2023
jonatack
left a comment
Member
There was a problem hiding this comment.
This looks like a very nice cleanup (and in a widely included and large file).
Have started running a node on this branch.
Initial approach ACK 513f4659b0a2ac4e2454ad2425b0fec8d7f64f04
maflcko
approved these changes
Aug 3, 2023
maflcko
left a comment
Member
There was a problem hiding this comment.
very nice, ACK 513f4659b0a2ac4e2454ad2425b0fec8d7f64f04 🙊
Show signature
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: very nice, ACK 513f4659b0a2ac4e2454ad2425b0fec8d7f64f04 🙊
GEzCyPu57ooJeHDaJ2Nuwgozpyvft2GzjmZzH1LsuCmxm4NLtc+ONqj570HEbSuzwFAnZ/pZOvN9NSTjIWsxDw==
Member
There was a problem hiding this comment.
review note in 10105d97f412cc54ba4031c56538f7e0c611251f: A simple is_same_v works, because I presume T can never include const in a prevector, similar to how std::vector<const int> does not exist.
…eMany() Instead of recursively calling `SerializeMany` and peeling off one argument at a time, use a fold expression. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
…izeMany() Instead of recursively calling `UnserializeMany` and peeling off one argument at a time, use a fold expression. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
513f465 to
f054bd0
Compare
Contributor
Author
|
Rebased to f054bd0 to clang-format the changes, and adds back the |
Member
|
only change is to add a missing Show signatureSignature: |
Member
|
ACK f054bd0 |
Member
|
utACK f054bd0 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 9, 2023
f054bd0 refactor: use "if constexpr" in std::vector's Unserialize() (Martin Leitner-Ankerl) 088caa6 refactor: use "if constexpr" in std::vector's Serialize() (Martin Leitner-Ankerl) 0fafaca refactor: use "if constexpr" in prevector's Unserialize() (Martin Leitner-Ankerl) c8839ec refactor: use "if constexpr" in prevector's Serialize() (Martin Leitner-Ankerl) 1403d18 refactor: use fold expressions instead of recursive calls in UnserializeMany() (Martin Leitner-Ankerl) bd08a00 refactor: use fold expressions instead of recursive calls in SerializeMany() (Martin Leitner-Ankerl) Pull request description: This simplifies the serialization code a bit and should also make it a bit faster. * use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations. * use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization. ACKs for top commit: MarcoFalke: only change is to add a missing `&`. lgtm, re-ACK f054bd0 📦 jonatack: ACK f054bd0 sipa: utACK f054bd0 john-moffett: ACK f054bd0 Tree-SHA512: 0417bf2d6be486c581732297945449211fc3481bac82964e27628b38ef55a47dfa58d730148aeaf1b19fa8eb1076489cc646ceebb178162a9afa59034601501d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This simplifies the serialization code a bit and should also make it a bit faster.
use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
use
if constexprinstead of unnecessarily creating a temporary object only to call the right overload. This is used forstd::vectorandprevectorserialization.