Refactor: Fix compiler warning in prevector.h #14651

Fixing warnings reported by GCC: memset of non-trivial type

Please send the leveldb changes upstream.

Please send the leveldb changes upstream.

leveldb changes sent upstream. Rebased to only include the non-leveldb patch.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ldm5180 Can you post the warnings emitted? What gcc version are you using?

What are the performance impacts of this change?

@ldm5180 Can you post the warnings emitted? What gcc version are you using?

gcc version 8.2.0 (Ubuntu 8.2.0-7ubuntu1)

In file included from bench/prevector.cpp:6:
./prevector.h: In instantiation of ‘void prevector<N, T, Size, Diff>::fill(T*, ptrdiff_t) [with unsigned int N = 28; T = nontrivial_t; Size = unsigned int; Diff = int; ptrdiff_t = long int]’:                                                                                
./prevector.h:340:9:   required from ‘void prevector<N, T, Size, Diff>::resize(prevector<N, T, Size, Diff>::size_type) [with unsigned int N = 28; T = nontrivial_t; Size = unsigned int; Diff = int; prevector<N, T, Size, Diff>::size_type = unsigned int]’                   
bench/prevector.cpp:47:13:   required from ‘void PrevectorClear(benchmark::State&) [with T = nontrivial_t]’
bench/prevector.cpp:102:1:   required from here
./prevector.h:205:21: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct nontrivial_t’; use assignment or value-initialization instead [-Wclass-memaccess]                                                                             
             ::memset(dst, 0, count * sizeof(T));
             ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
bench/prevector.cpp:12:8: note: ‘struct nontrivial_t’ declared here
 struct nontrivial_t {
        ^~~~~~~~~~~~

What are the performance impacts of this change?

Expect, no impact. Is there a performance analysis test suite?

There are a few different cases. Here are them shown in the Compiler Explorer:

1. Default fill w/ trivial type: https://gcc.godbolt.org/z/qzIKUJ (no difference)
2. Default fill w/ nontrivial type: https://gcc.godbolt.org/z/h38Swd (difference discussed below)
3. Copy fill w/ trivial type: https://gcc.godbolt.org/z/a2z3MJ (possibly better w/ std::fill)
4. Copy fill w/ nontrivial type: https://gcc.godbolt.org/z/gB50DT (difference discussed below)

In both functions, when a trivial type is used there is no difference or the std::fill version is slightly better.

In both functions, when a nontrivial type is used, it gets better optimized with the std::fill version. You can see this by changing the optimization level from -O2 to -O3 and notice them both turn into almost the same code, but with the std::fill version still being about 10 instructions shorter. Without diving deeper into this, my estimation is that the std::fill version can be "seen" more clearly by the optimizer which is why it results in the same code with fewer optimizers enabled.

Concept ACK; from what I read is that for character types std::fill is generally compiled similar to memset would, so I expect no performance regression here.

utACK aa6f3d7db443c1154eda78f6fb006a69aa20ce3a

Expect, no impact. Is there a performance analysis test suite?

Yes, there is src/bench/bench_bitcoin. As the whole point of prevector is to be an optimization, I'd really prefer to see benchmarks here (if possible, both of gcc's and clang's standard library) and not make guesses.

@ldm5180 Very nice first time contribution! Hope to see more contributions from you!

Echoing @laanwj - I'd also be interested in benchmark results.

I think the important benchmarks are the PrevectorResize*, but I just included all the Prevector tests.

The differences between g++ and clang++ are pretty stark, but the numbers within each version are very similar. Running them several times shows that they are not super stable number (at least on my machine). I don't know what your target is for acceptable, but here are some results: PrevectorResizeNontrivial: clang gets ~4.8% faster, gcc gets ~0.4% slower PrevectorResizeTrivial: clang gets ~3.7% slower, gcc gets ~0.1% slower

For clang, these might be outside the margin of error. For gcc, I don't think it is.

Baseline GCC:

# Benchmark, evals, iterations, total, min, max, median
PrevectorClearNontrivial, 10, 28300, 11.1891, 3.92909e-05, 4.03267e-05, 3.94721e-05
PrevectorClearTrivial, 10, 88600, 12.7673, 1.43817e-05, 1.44577e-05, 1.44081e-05
PrevectorDeserializeNontrivial, 10, 6800, 4.29737, 6.28875e-05, 6.33766e-05, 6.32723e-05
PrevectorDeserializeTrivial, 10, 52000, 4.91249, 9.38555e-06, 9.63038e-06, 9.44697e-06
PrevectorDestructorNontrivial, 10, 28800, 11.379, 3.93948e-05, 3.99467e-05, 3.94788e-05
PrevectorDestructorTrivial, 10, 88900, 12.5935, 1.41116e-05, 1.42736e-05, 1.41555e-05
PrevectorResizeNontrivial, 10, 28900, 7.21061, 2.46638e-05, 2.51662e-05, 2.49783e-05
PrevectorResizeTrivial, 10, 90300, 8.14905, 8.97413e-06, 9.25092e-06, 9.00301e-06

New version GCC:

# Benchmark, evals, iterations, total, min, max, median
PrevectorClearNontrivial, 10, 28300, 11.1975, 3.94149e-05, 3.97891e-05, 3.95875e-05
PrevectorClearTrivial, 10, 88600, 12.814, 1.43741e-05, 1.46315e-05, 1.4468e-05
PrevectorDeserializeNontrivial, 10, 6800, 4.3056, 6.29083e-05, 6.38494e-05, 6.33461e-05
PrevectorDeserializeTrivial, 10, 52000, 4.93494, 9.41098e-06, 9.55156e-06, 9.50646e-06
PrevectorDestructorNontrivial, 10, 28800, 11.3737, 3.92276e-05, 4.03218e-05, 3.93623e-05
PrevectorDestructorTrivial, 10, 88900, 12.5981, 1.41316e-05, 1.42107e-05, 1.41824e-05
PrevectorResizeNontrivial, 10, 28900, 7.25885, 2.47887e-05, 2.59588e-05, 2.50821e-05
PrevectorResizeTrivial, 10, 90300, 8.1371, 8.97296e-06, 9.07207e-06, 9.01449e-06

Baseline Clang:

# Benchmark, evals, iterations, total, min, max, median
PrevectorClearNontrivial, 10, 28300, 0.000571983, 2.00424e-09, 2.0771e-09, 2.0183e-09
PrevectorClearTrivial, 10, 88600, 0.00176638, 1.9287e-09, 2.15058e-09, 2.00778e-09
PrevectorDeserializeNontrivial, 10, 6800, 10.3682, 0.000151424, 0.000155099, 0.00015202
PrevectorDeserializeTrivial, 10, 52000, 7.87747, 1.50409e-05, 1.55236e-05, 1.51269e-05
PrevectorDestructorNontrivial, 10, 28800, 0.000430928, 1.25281e-09, 2.40497e-09, 1.27898e-09
PrevectorDestructorTrivial, 10, 88900, 0.00119125, 1.26731e-09, 1.63589e-09, 1.26902e-09
PrevectorResizeNontrivial, 10, 28900, 2.05184, 7.05283e-06, 7.19934e-06, 7.09096e-06
PrevectorResizeTrivial, 10, 90300, 6.48936, 7.11218e-06, 7.44356e-06, 7.17658e-06

New version Clang:

# Benchmark, evals, iterations, total, min, max, median                                     
PrevectorClearNontrivial, 10, 28300, 0.000853285, 2.86014e-09, 3.48901e-09, 3.00078e-09     
PrevectorClearTrivial, 10, 88600, 0.00264027, 2.52998e-09, 3.13509e-09, 3.02476e-09         
PrevectorDeserializeNontrivial, 10, 6800, 10.4545, 0.000152216, 0.000155233, 0.000153983    
PrevectorDeserializeTrivial, 10, 52000, 9.68222, 1.85139e-05, 1.89764e-05, 1.85454e-05      
PrevectorDestructorNontrivial, 10, 28800, 0.000431799, 1.25712e-09, 2.39365e-09, 1.27613e-09
PrevectorDestructorTrivial, 10, 88900, 0.00118995, 1.25501e-09, 1.64991e-09, 1.27141e-09    
PrevectorResizeNontrivial, 10, 28900, 1.95072, 6.57969e-06, 7.14017e-06, 6.75173e-06        
PrevectorResizeTrivial, 10, 90300, 6.70796, 7.38159e-06, 7.47863e-06, 7.44035e-06           

@ldm5180 thank you for doing benchmarks; so it's not significantly slower with gcc, surprisingly even faster with clang, looks good to me!

utACK https://github.com/bitcoin/bitcoin/pull/14651/commits/76e13b586ff690dd3312f719f305c0d1021cd505

utACK 76e13b5, nice code simplification and tiny performance bonus (with clang).