Skip to content

Conversation

@Empact
Copy link
Contributor

@Empact Empact commented Nov 13, 2018

This is clean-up post #14651:

  • Use one implementation of prevector::fill, as it's possible now that the implementations are identical.
  • Only apply the IS_TRIVIALLY_CONSTRUCTIBLE handling to the bench file where it is used, and drop the now-unnecessary associated compat includes.

@practicalswift
Copy link
Contributor

Concept ACK

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@laanwj
Copy link
Member

laanwj commented Nov 13, 2018

concept ACK

It's now only referenced from the bench, so leave it there. This allows us to
drop the associated includes as well.
Now that the implementation is identical, we can use a default value to
distinguish them.
@sipa
Copy link
Member

sipa commented Nov 15, 2018

utACK 69ca487

1 similar comment
@maflcko
Copy link
Member

maflcko commented Nov 15, 2018

utACK 69ca487

@promag
Copy link
Contributor

promag commented Nov 15, 2018

utACK 69ca487.

@practicalswift
Copy link
Contributor

utACK 69ca487

1 similar comment
@laanwj
Copy link
Member

laanwj commented Nov 22, 2018

utACK 69ca487

@laanwj laanwj merged commit 69ca487 into bitcoin:master Nov 22, 2018
laanwj added a commit that referenced this pull request Nov 22, 2018
69ca487 Implement prevector::fill once (Ben Woosley)
7bad78c Drop defunct IS_TRIVIALLY_CONSTRUCTIBLE handling from prevector.h (Ben Woosley)

Pull request description:

  This is clean-up post #14651:
  * Use one implementation of `prevector::fill`, as it's possible now that the implementations are identical.
  * Only apply the `IS_TRIVIALLY_CONSTRUCTIBLE` handling to the bench file where it is used, and drop the now-unnecessary associated compat includes.

Tree-SHA512: 5930b3a17fccd39af10add40202ad97a297aebecc049af72ca920d0d55b3e4c3c30ce864c8a683355895f0196396d4ea56ba9f9637bdc7d16964cdf66c195485
@Empact Empact deleted the trivial-prevector branch November 22, 2018 14:21
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Dec 20, 2020
d856989 warnings: Compiler warning on memset usage for non-trivial type (Lenny Maiorani)
4cb188e Drop defunct IS_TRIVIALLY_CONSTRUCTIBLE handling from prevector.h (Ben Woosley)
c412d5f Reduce redundant code of prevector and speed it up (random-zebra)
e6741d0 Add new prevector benchmarks. (random-zebra)

Pull request description:

  Backports from Bitcoin a series of optimizations for `prevector` operations, especially around resizing, which takes a significative portion of the time spent by of `ReadBlockFromDisk()`.

  Running the benchmark tool before the patch:
  ```
  #Benchmark,                    count,  min,    max,    avg,    minc,   maxc,   avgc
  -----------                    ------  ----    ----    ----    -----   -----   ----
  PrevectorClearTrivial,         3328,   312631, 329431, 320311, 812854, 856536, 832823
  PrevectorDestructorTrivial,    3328,   317011, 353344, 325141, 824243, 918711, 845382
  PrevectorResizeTrivial,        3328,   321333, 327672, 324713, 835482, 851963, 844269
  ```

  and after:
  ```
  #Benchmark,                    count, min,    max,    avg,    minc,   maxc,   avgc
  -----------                    ------ ----    ----    ----    -----   -----   ----
  PrevectorClearTrivial,         65536, 15726,  16368,  16067,  40889,  42559,  41775
  PrevectorDestructorTrivial,    65536, 15865,  16416,  16141,  41249,  42684,  41968
  PrevectorResizeTrivial,        65536, 16036,  16320,  16213,  41696,  42433,  42156
  ```

  shows that `prevector::clear()`, `prevector::~prevector()`, and `prevector::resize()` become (on avg) **20X** faster.

  Based on:
  - bitcoin#12549
  - bitcoin#14651
  - bitcoin#14715

ACKs for top commit:
  Fuzzbawls:
    utACK d856989
  furszy:
    Quite cool. utACK d856989

Tree-SHA512: 8060458bfd5373448d83a3d905fbc95379cfc1c1aed19b750c98b06a1a6a4ba85a6715865388752a5bd579676e2b2e26d3584ec5b26d51e9ba1380bc4fa73f33
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
christiancfifi added a commit to christiancfifi/dash that referenced this pull request Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants