refactor: Improve CRollingBloomFilter::reset by using std::fill #16073

pull promag wants to merge 2 commits into bitcoin:master from promag:2019-05-rollingbloomfilter-reset changing 2 files +11 −3
  1. promag commented at 11:13 PM on May 21, 2019: member

    Cleaner code. Also improves performance with --enable-debug (which is meaningless to non-developers).

    Before:

    # Benchmark, evals, iterations, total, min, max, median
RollingBloomReset, 5, 150, 19.3008, 0.0254917, 0.0259195, 0.0257395

    After:

    # Benchmark, evals, iterations, total, min, max, median
RollingBloomReset, 5, 150, 5.43269, 0.00720651, 0.00729697, 0.00724854
  2. promag commented at 11:16 PM on May 21, 2019: member

    I was going to use memset but I think std::fill is much nicer and probably as fast as memset.

  3. gmaxwell commented at 11:18 PM on May 21, 2019: contributor

    Seems okay to me, but why are we resetting a big rolling bloom filter this often?

    It may be better to use a generation counter. (this isn't a reason not to take this improvement, but maybe an area for bigger improvement).

  4. MarcoFalke commented at 11:30 PM on May 21, 2019: member

    Any intuition why this is faster? Could add a trivial benchmark as first commit?

  5. DrahtBot added the label Refactoring on May 21, 2019
  6. promag commented at 6:51 AM on May 22, 2019: member

    @MarcoFalke one reason may be a specialized version of fill for uint64_t. Will add the benchmark.

  7. jonasschnelli commented at 9:21 AM on May 22, 2019: contributor

    Well spotted! Concept ACK

  8. promag commented at 1:46 PM on May 22, 2019: member

    Updated OP with benchmark result.

    Profiling before and after this change:

    Screenshot 2019-05-22 at 00 11 14

    Screenshot 2019-05-22 at 00 10 12

  9. promag force-pushed on May 22, 2019
  10. promag commented at 1:51 PM on May 22, 2019: member

    @MarcoFalke benchmark added.

  11. laanwj commented at 1:51 PM on May 22, 2019: member

    Any intuition why this is faster? Could add a trivial benchmark as first commit?

    IIRC std::fill tends to be optimized to a memset, which uses arch-specific instructions for the fastest filling of memory, whereas a loop is not

  12. promag commented at 1:54 PM on May 22, 2019: member

    but maybe an area for bigger improvement @gmaxwell maybe open an issue and throw some suggestions?

  13. jamesob approved
  15. MarcoFalke commented at 2:12 PM on May 22, 2019: member

    What compiler and hardware are you using?

    utACK 8d774bbbe8454119161446225451210c23fe56d3

    Couldn't find a performance difference on my system, but the code looks cleaner.

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK 8d774bbbe8454119161446225451210c23fe56d3

Couldn't find a performance difference on my system, but the code looks cleaner.
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiX4gwAsi8srAjhAkRx0STvjJ75oiaU5ADUonn7zrstBc17F2kNONLedb+vR9J/
elm5lUmqrucGVItm1UlFgaOB/yq100B688xovcaOez2RKuSTT51y26Aur1Amrtyk
CDiP1dYabzImeSUPbaKEP33Hx0fNbzp77NAqbUW/ufJUYqAlmbh5QJsKP/baMKyo
f+1Jt5n/fDA9B9YNIWckb5qLAqlLDQE07RrMLRROcIHW3+ipsxdHEpug49QEzlTE
15lQAjzTXOfVzAX78dxdFc0lRzVr1MmPyAWlA26xhqe+4Lo4ObW7OMeSWHhMlISL
fntaqw2K8iYz8rBo218A+nY4BjcJSMEcjOAP2n1p+kU2rSm/NbyIjwZaHOv4xIWT
S/Y5DbZIZdPmIcEM/CXsGUzGENjIUeCyCa77+Rt6O0swC2OKaW3cJ02u+Iksq9iY
UnVv029AZdjRz+RGDHk2ubosddk/hczXTUju9+lUDoRpH0FKu0T5efn7Co1yT3gR
gwypjCZu
=k0jH
-----END PGP SIGNATURE-----

    Timestamp of file with hash 0afe9d4d13b7b6fae0b5c6625b924bdb707bd6bf085d76618e964707100296a0 -

    </details>

  16. promag commented at 2:24 PM on May 22, 2019: member

    @MarcoFalke Apple LLVM version 10.0.1 (clang-1001.0.46.4) with --enable-debug --with-sanitizers=thread.

  17. in src/bench/rollingbloom.cpp:40 in 8d774bbbe8 outdated 
      35 | +        filter.reset();
  36 | +    }
  37 | +}
  38 | +
  39 |  BENCHMARK(RollingBloom, 1500 * 1000);
  40 | +BENCHMARK(RollingBloomReset, 150);
    MarcoFalke commented at 2:44 PM on May 22, 2019:

    Please adjust this, so that it takes as much time as the other benchmarks (~4seconds, depending on arch)

    promag commented at 3:02 PM on May 22, 2019:

    Done.

  18. MarcoFalke commented at 2:46 PM on May 22, 2019: member

    Please mention that in the OP. Performance improvements when run with a sanitizer are completely meaningless to non-developers.

  19. laanwj commented at 2:49 PM on May 22, 2019: member

    Also, always benchmark in release mode. --enable-debug dials down the optimization level. It's very possible that you'll see no difference without that.

  20. bench: Add benchmark for CRollingBloomFilter::reset d2dbc7da26
  21. refactor: Improve CRollingBloomFilter::reset by using std::fill df9e15f092
  22. promag force-pushed on May 22, 2019
  23. promag commented at 3:02 PM on May 22, 2019: member

    After all in release mode I have the same result as @MarcoFalke. Updated OP.

  24. MarcoFalke commented at 3:22 PM on May 22, 2019: member

    re-utACK df9e15f092

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-utACK df9e15f092
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUipzwwAha/6gluoBoDmrtyN06+eiKHYnCrPhkDCFXmUptsHMMkCLnxpzWslG9zX
CXO6RZq48KQj6ZSx0TjdVFZuq5JEICqbiSk3u9McQxl2sILJx7x0fdz7yS/elu5m
RvT+cFIxav/eb7Bgnm6Zoz9EdRHFqyvglN3xBKowF5M2AHXbDq9trhM8oL9sgSFZ
dtHpaAern/L6s1UWF5zgVpDIqVB6Xr0mp0HodjMo8UurMTzB4nzZR6PAABJOeXPG
CGA7Xfkqj/gC/MP4gUKndS1aRlsY0uERPWclMd4NDF6fEX1mSAQHzJ7GO8pDBM5D
RwDpmV99ZdYUiD0Uv0IM2LfQQjWMSXNgoYm4CNf3R9FTLMUmM15xU8BcXTAXwLh2
0baT6VIdDhy12BtpcDm9cxrIoKtU5OBFxjBLRY7ZkyMs8LlEjBxadlSBBOlDEUsm
6uAj8b2Mek26y5fRJ+eBFok9B/up9G7ci5GuCmMhVf4syA1bSr+q/sIBr2U0Phea
TPfT5e04
=joN1
-----END PGP SIGNATURE-----

    Timestamp of file with hash 156b97ccbe8a8aa40230d38498141d8b529ac9c16533d56aa09c7bd632f6c12c -

    </details>

  26. practicalswift commented at 3:47 PM on May 22, 2019: contributor

    utACK d2dbc7da26e1ca40200521c05a0b1ca75578acd2

  27. sipa commented at 6:31 PM on May 22, 2019: member

    utACK d2dbc7da26e1ca40200521c05a0b1ca75578acd2

  28. laanwj merged this on May 22, 2019
  29. laanwj closed this on May 22, 2019
  30. laanwj referenced this in commit fdc951ad04 on May 22, 2019
  31. promag deleted the branch on May 22, 2019
  32. MarcoFalke commented at 8:38 PM on May 22, 2019: member

    @sipa, @practicalswift Just FYI, you reviewed an incorrect commit and your ACKs haven't been recorded in the merge.

  33. promag commented at 8:42 PM on May 22, 2019: member

    Sort by date FTW.

  34. practicalswift commented at 8:56 PM on May 22, 2019: contributor

    @MarcoFalke Oh sorry about that and thanks for the notification!

  35. deadalnix referenced this in commit 910e126589 on Jun 9, 2020
  36. ftrader referenced this in commit 4d055e0a7d on Aug 17, 2020
  37. zkbot referenced this in commit be459619a8 on Mar 5, 2021
  38. zkbot referenced this in commit 78de0cdf46 on Apr 15, 2021
  39. PastaPastaPasta referenced this in commit 4f416ff6c1 on Jun 27, 2021
  40. PastaPastaPasta referenced this in commit 74fc146cda on Jun 28, 2021
  41. PastaPastaPasta referenced this in commit 7643552ea7 on Jun 29, 2021
  42. PastaPastaPasta referenced this in commit 6d26a244e6 on Jul 1, 2021
  43. PastaPastaPasta referenced this in commit 8ac49d737f on Jul 1, 2021
  44. pravblockc referenced this in commit cc1574e516 on Sep 25, 2021
  45. pravblockc referenced this in commit aeb82bc6ed on Sep 28, 2021
  46. pravblockc referenced this in commit b5316a8592 on Sep 29, 2021
  47. PastaPastaPasta referenced this in commit 2f845d8074 on Sep 30, 2021
  48. kittywhiskers referenced this in commit a9b645bacb on Oct 12, 2021
  49. MarcoFalke locked this on Dec 16, 2021
Contributors
promaggmaxwellMarcoFalkejonasschnellilaanwjjamesobpracticalswiftsipa
Labels
Refactoring
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 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me