refactor: deduplicate integer serialization in RollingBloom benchmark #24088

pull phyBrackets wants to merge 1 commits into bitcoin:master from phyBrackets:patch-1 changing 1 files +4 −8
  1. phyBrackets commented at 6:34 PM on January 17, 2022: contributor

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

    refactor: deduplicate integer serialization in RollingBloom benchmark and i think we have to follow don't repeat yourself pattern here! I don't think that this much affect the memory and efficiency because it is still in constant time . But surely it reduces some readability of code which is not the big headache for sure.

  2. theStack commented at 6:50 PM on January 17, 2022: member

    Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling: https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L44-L48

    https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L77-L81

    As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

  3. phyBrackets renamed this:
    shouldn't we follow DRY principle here?
    refactor: deduplicate integer serialization in RollingBloom benchmark
    on Jan 17, 2022
  4. phyBrackets closed this on Jan 17, 2022
  5. phyBrackets reopened this on Jan 17, 2022
  6. phyBrackets commented at 7:09 PM on January 17, 2022: contributor

    Thanks for the suggestion. I update the following. @theStack

  7. DrahtBot added the label Refactoring on Jan 17, 2022
  8. in src/bench/rollingbloom.cpp:16 in d4526fc5a8 outdated 
      12 | @@ -13,16 +13,13 @@ static void RollingBloom(benchmark::Bench& bench)
  13 |      uint32_t count = 0;
  14 |      bench.run([&] {
  15 |          count++;
  16 | -        data[0] = count & 0xFF;
  17 | -        data[1] = (count >> 8) & 0xFF;
  18 | -        data[2] = (count >> 16) & 0xFF;
  19 | -        data[3] = (count >> 24) & 0xFF;
  20 | +        for(int i = 0; i <= 3; i++){
    sipa commented at 9:27 PM on January 17, 2022:

    This is just WriteLE32(data, count);.

  9. in src/bench/rollingbloom.cpp:20 in d4526fc5a8 outdated 
      24 | -
  25 | -        data[0] = (count >> 24) & 0xFF;
  26 | -        data[1] = (count >> 16) & 0xFF;
  27 | -        data[2] = (count >> 8) & 0xFF;
  28 | -        data[3] = count & 0xFF;
  29 | +        for(int i = 3; i >= 0; i--){
    sipa commented at 9:28 PM on January 17, 2022:

    This is just WriteBE32(data, count);.

    phyBrackets commented at 6:30 AM on January 18, 2022:

    so, should i change the following to just a function call by adding common.h header ?

    sipa commented at 7:35 PM on February 4, 2022:

    Yeah, better than duplicating the logic here.

  10. fanquake commented at 2:38 AM on January 18, 2022: member

    Once you've addressed the suggestions, make sure you squash your changes to a single commit, with a proper commit message.

  11. phyBrackets commented at 6:29 AM on January 18, 2022: contributor

    Once you've addressed the suggestions, make sure you squash your changes to a single commit, with a proper commit message. @fanquake will take care of that. Thanks.

  12. phyBrackets commented at 8:43 PM on January 18, 2022: contributor

    Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling:

    https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L44-L48

    https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L77-L81

    As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

    Isn't this bit inefficient because we are actually dropping the whole content of common.h ?

  13. phyBrackets commented at 8:29 PM on February 4, 2022: contributor

    Seems like you could also simply use the helpers WriteLE32 and WriteBE32 from the <crypto/common.h> header in this case to avoid manual bit-fiddling: https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L44-L48

    https://github.com/bitcoin/bitcoin/blob/d0bf9bb6a539f151ec92725d20a2b6c22cb095a5/src/crypto/common.h#L77-L81

    As PR and commit subject I'd suggest using something more concrete like "refactor: deduplicate integer serialization in RollingBloom benchmark", rather than a (rhetorical) question.

    Isn't this bit inefficient because we are actually dropping the whole content of common.h ?

    Hey @sipa what's your views on this before making a function call ?

  14. sipa commented at 8:37 PM on February 4, 2022: member

    I don't understand your question. Using ReadLE32/WriteLE32 is almost certainly more efficient than reimplementing it yourself (they're written in a way that the compiler can easily optimize out). Not that ~nanosecond level performance is relevant here...

  15. phyBrackets commented at 9:02 PM on February 4, 2022: contributor

    oh , okay ! Then it's fine I think.

  16. phyBrackets commented at 9:25 PM on February 4, 2022: contributor

    Sorry for the direct commit , need to do some more changes.

  17. phyBrackets force-pushed on Feb 5, 2022
  18. phyBrackets commented at 2:54 PM on February 12, 2022: contributor

    Hey, any update on this?

  19. MarcoFalke commented at 3:02 PM on February 12, 2022: member

    I haven't looked at the changes, but this can't be merged unless squashed.

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

  20. phyBrackets commented at 7:13 PM on February 12, 2022: contributor

    Cool, will do that soon. Thanks!

  21. phyBrackets commented at 2:22 PM on February 14, 2022: contributor

    Hey, I'm having some problem with squashing the commits. Should I make new PR with final changes? Will close this and refer to this PR! If you would allow?

  22. fanquake commented at 2:28 PM on February 14, 2022: member

    Should I make new PR with final changes?

    No. Please don't open new PRs for the same changes. You might need to take another look through: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits/.

  23. phyBrackets commented at 6:36 PM on February 17, 2022: contributor

    Hey @fanquake , I tried to rebase/squash , I have some problems with branches I think , there is branch conflict . I really don't want to open a new PR but it seems the only option , by the way if you would allow, I'll open that and set the reference to this PR , and will close this pr ?

  24. sipa commented at 6:43 PM on February 17, 2022: member

    @phyBrackets It's probably worthwhile learning how to resolve these issues anyway, if you want to continue contributing, whether you do that for this PR or a future one.

  25. in src/bench/rollingbloom.cpp:14 in d7825aa76d outdated 
       9 |  
  10 |  static void RollingBloom(benchmark::Bench& bench)
  11 |  {
  12 |      CRollingBloomFilter filter(120000, 0.000001);
  13 |      std::vector<unsigned char> data(32);
  14 | +    unsigned char* dataPtr = &data[0];
    MarcoFalke commented at 6:54 PM on February 17, 2022: 
        unsigned char* dataPtr = data.data();

    Shouldn't this use the C++11 member function?

    phyBrackets commented at 5:29 PM on February 18, 2022:

    Yeah, resolve this with the new commit.

  26. phyBrackets commented at 5:31 PM on February 18, 2022: contributor

    @phyBrackets It's probably worthwhile learning how to resolve these issues anyway, if you want to continue contributing, whether you do that for this PR or a future one.

    Hey @sipa , I'm really trying and searching around how to resolve this issue but I'm not able to , actually when I' trying to run git rebase -i HEAD~7 , this only shows the first 4 commits.

  27. MarcoFalke commented at 8:29 AM on February 19, 2022: member

    Your branch contains merge commits, which can't be squashed in a rebase.

    Something like this might work (untested!!):

    git checkout patch-1                                       # Verify this is on commit aa91fd400a6a23feeacdff0d2adacd18dd313e47
git fetch origin 5d254a234d8c1569b0161264cc6d5d8d0ce0d864  # Fetch the latest master
git merge 5d254a234d8c1569b0161264cc6d5d8d0ce0d864         # Merge the latest master
git reset --soft 5d254a234d8c1569b0161264cc6d5d8d0ce0d864  # Discard commit history, but keep all changes staged
git commit --message="bla bla"                             # Create a commit with all staged changes
git push origin patch-1 --force
  28. phyBrackets commented at 9:27 AM on February 19, 2022: contributor

    I tried this , it seems more mess tho.

  29. MarcoFalke commented at 9:34 AM on February 19, 2022: member

    After the steps in #24088 (comment) you need to force push, not merge

  30. Remove the deduplicating of data in rollingbloom.cpp e1fcf8dc14
  31. phyBrackets force-pushed on Feb 19, 2022
  32. phyBrackets commented at 9:43 AM on February 19, 2022: contributor

    Thank you sm @MarcoFalke . It seems like it works fine.

  33. theStack approved
  34. theStack commented at 7:30 PM on February 20, 2022: member

    Code-review ACK e1fcf8dc148615842341726c6aa35bb00c35f1d0

    Nit: I don't think the dataPtr pointer is really needed here, you could just call the serialization helper functions with data.data() as first parameter each.

  35. phyBrackets commented at 5:56 AM on February 21, 2022: contributor

    Cool, but I think it is more readable through this . But yeah if this affects on unnecessary memory use. We can remove that. Let me know your further take on this.

  36. laanwj commented at 11:22 AM on April 6, 2022: member

    Nit: I don't think the dataPtr pointer is really needed here, you could just call the serialization helper functions with data.data() as first parameter each.

    +1. The assignment is unnecessary. I doubt it affects memory use in any significant way but I see no reason not to write it shorter if you're touching this code anyway.

  37. fanquake commented at 1:07 PM on April 6, 2022: member

    I've just gone ahead and fixed this up in #24784, given this is straightforward, and likely been open long enough.

  38. fanquake closed this on Apr 6, 2022
  39. MarcoFalke referenced this in commit 323d4c09c0 on Apr 7, 2022
  40. sidhujag referenced this in commit 77cbdaf1a9 on Apr 7, 2022
  41. DrahtBot locked this on Apr 6, 2023
Contributors
phyBracketstheStacksipafanquakeMarcoFalkelaanwj
Labels
Refactoring
Linked (view graph)
#24784 refactor: deduplicate integer serialization in RollingBloom benchmark
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: 2026-04-22 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me