tests: Initialize recently introduced non-static class member lastCycles to zero in constructor #11654

practicalswift · 2017-11-10T13:47:19Z

Initialize recently introduced non-static class member lastCycles to zero in constructor.

lastCycles was introduced in 3532818 which was merged into master yesterday.

Friendly ping @laanwj :-)

promag · 2017-11-10T21:13:19Z

utACK ac78e5c.

It would be nice to have one initialization flavour. Also, split members that don't have to be initialized with a comment.

laanwj · 2017-11-11T07:35:03Z

src/bench/bench.h

@@ -65,6 +65,7 @@ namespace benchmark {
            minCycles = std::numeric_limits<uint64_t>::max();


Yeah let's change it to

State(std::string _name, duration _maxElapsed) : name(_name), maxElapsed(_maxElapsed), count(0), minTime(duration::max()), maxTime(duration::zero()), minCycles(std::numeric_limits<uint64_t>::max()), maxCycles(maxCycles = std::numeric_limits<uint64_t>::min()), countMask(1), lastCycles(0)

Makes it easier to see that everything is initialized.

practicalswift · 2017-11-12T15:47:57Z

@laanwj Good point! Now fixed :-)

maflcko · 2017-11-13T20:03:04Z

src/bench/bench.h

+        State(std::string _name, duration _maxElapsed) :
+            name(_name),
+            maxElapsed(_maxElapsed),
+            count(0),


Please obey -Wreorder

Oh, of course. Now fixed! Thanks for notifying :-)

…zero in constructor lastCycles was introduced in 3532818 which was merged into master yesterday. Also initialize beginCycles to zero for consistency and completeness.

practicalswift · 2017-11-13T21:38:15Z

Addressed @MarcoFalke:s feedback initialization order. Now also initializing beginCycles to zero for consistency and completeness.

Please re-review :-)

maflcko · 2017-11-13T22:09:11Z

utACK 069215e

@laanwj

…member lastCycles to zero in constructor 069215e Initialize recently introduced non-static class member lastCycles to zero in constructor (practicalswift) Pull request description: Initialize recently introduced non-static class member `lastCycles` to zero in constructor. `lastCycles` was introduced in 3532818 which was merged into master yesterday. Friendly ping @laanwj :-) Tree-SHA512: cb93b6a8f6e2e3b06cd05a635da95c84f3df64c21fc23fe82f98306ea571badc32040315b563e46ddb5203128226bc334269acd497beead5a5777c434060fd85

fa94614 [doc] dev-notes: Members should be initialized (MarcoFalke) Pull request description: Also, remove mention of threads that were removed long ago. Motivation: Make it easier to spot bugs such as #11654 and #12426 Tree-SHA512: 8ca1cb54e830e9368803bd98a8b08c39bf2d46f079094ed7e070b32ae15a6e611ce98d7a614f897803309f4728575e6bc9357fab1157c53d2536417eb8271653

Micro-benchmarking framework part 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6733 - bitcoin/bitcoin#6770 - bitcoin/bitcoin#6892 - Excluding changes to `src/policy/policy.h` which we don't have yet. - bitcoin/bitcoin#7934 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#8039 - bitcoin/bitcoin#8107 - bitcoin/bitcoin#8115 - bitcoin/bitcoin#8914 - Required resolving several merge conflicts in code that had been refactored upstream. The changes were simple enough that I decided it was okay to impose merge conflicts on pulling in those refactors later. - bitcoin/bitcoin#9200 - bitcoin/bitcoin#9202 - Adds support for measuring CPU cycles, which is later removed in an upstream PR after the refactor. I am including it to reduce future merge conflicts. - bitcoin/bitcoin#9281 - Only changes to `src/bench/bench.cpp` - bitcoin/bitcoin#9498 - bitcoin/bitcoin#9712 - bitcoin/bitcoin#9547 - bitcoin/bitcoin#9505 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#9792 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#10272 - bitcoin/bitcoin#10395 - Only changes to `src/bench/` - bitcoin/bitcoin#10735 - Only changes to `src/bench/base58.cpp` - bitcoin/bitcoin#10963 - bitcoin/bitcoin#11303 - Only the benchmark backend change. - bitcoin/bitcoin#11562 - bitcoin/bitcoin#11646 - bitcoin/bitcoin#11654 This pulls in all changes to the micro-benchmark framework prior to December 2017, when it was rewritten. The rewrite depends on other upstream PRs we have not pulled in yet. This does not pull in all benchmarks prior to December 2017. It leaves out benchmarks that either test code we do not have yet (except for the `FastRandomContext` refactor, which I decided to pull in), or would require rewrites to work with our changes to the codebase.

@laanwj

… class member lastCycles to zero in constructor 069215e Initialize recently introduced non-static class member lastCycles to zero in constructor (practicalswift) Pull request description: Initialize recently introduced non-static class member `lastCycles` to zero in constructor. `lastCycles` was introduced in 3532818 which was merged into master yesterday. Friendly ping @laanwj :-) Tree-SHA512: cb93b6a8f6e2e3b06cd05a635da95c84f3df64c21fc23fe82f98306ea571badc32040315b563e46ddb5203128226bc334269acd497beead5a5777c434060fd85

@laanwj

… class member lastCycles to zero in constructor 069215e Initialize recently introduced non-static class member lastCycles to zero in constructor (practicalswift) Pull request description: Initialize recently introduced non-static class member `lastCycles` to zero in constructor. `lastCycles` was introduced in 3532818 which was merged into master yesterday. Friendly ping @laanwj :-) Tree-SHA512: cb93b6a8f6e2e3b06cd05a635da95c84f3df64c21fc23fe82f98306ea571badc32040315b563e46ddb5203128226bc334269acd497beead5a5777c434060fd85

@laanwj

… class member lastCycles to zero in constructor 069215e Initialize recently introduced non-static class member lastCycles to zero in constructor (practicalswift) Pull request description: Initialize recently introduced non-static class member `lastCycles` to zero in constructor. `lastCycles` was introduced in 3532818 which was merged into master yesterday. Friendly ping @laanwj :-) Tree-SHA512: cb93b6a8f6e2e3b06cd05a635da95c84f3df64c21fc23fe82f98306ea571badc32040315b563e46ddb5203128226bc334269acd497beead5a5777c434060fd85

3f3edde [Bench] Use PIVX address in Base58Decode test (random-zebra) 5a1be90 [Travis] Disable benchmark framework for trusty test (random-zebra) 1bd89ac Initialize recently introduced non-static class member lastCycles to zero in constructor (random-zebra) ec60671 Require a steady clock for bench with at least micro precision (random-zebra) 84069ce bench: prefer a steady clock if the resolution is no worse (random-zebra) 38367b1 bench: switch to std::chrono for time measurements (random-zebra) a24633a Remove countMaskInv caching in bench framework (random-zebra) 9e9bc22 Restore default format state of cout after printing with std::fixed/setprecision (random-zebra) 3dd559d Avoid static analyzer warnings regarding uninitialized arguments (random-zebra) e85f224 Replace boost::function with std::function (C++11) (random-zebra) 98c0857 Prevent warning: variable 'x' is uninitialized (random-zebra) 7f0d4b3 FastRandom benchmark (random-zebra) d9fa0c6 Add prevector destructor benchmark (random-zebra) e1527ba Assert that what might look like a possible division by zero is actually unreachable (random-zebra) e94cf15 bench: Fix initialization order in registration (random-zebra) 151c25f Basic CCheckQueue Benchmarks (random-zebra) 51aedbc Use std:thread:hardware_concurrency, instead of Boost, to determine available cores (random-zebra) d447613 Use real number of cores for default -par, ignore virtual cores (random-zebra) 9162a56 [Refactoring] Removed using namespace <xxx> from bench/ sources (random-zebra) 5c07f67 bench: Add support for measuring CPU cycles (random-zebra) 41ce1ed bench: Fix subtle counting issue when rescaling iteration count (random-zebra) 68ea794 Avoid integer division in the benchmark inner-most loop. (random-zebra) 3fa4f27 bench: Added base58 encoding/decoding benchmarks (random-zebra) 4442118 bench: Add crypto hash benchmarks (random-zebra) a5179b6 [Trivial] ensure minimal header conventions (random-zebra) 8607d6b Support very-fast-running benchmarks (random-zebra) 4aebb60 Simple benchmarking framework (random-zebra) Pull request description: Introduces the benchmarking framework, loosely based on google's micro-benchmarking library (https://github.com/google/benchmark), ported from Bitcoin, up to 0.16. The benchmark framework is hard-coded to run each benchmark for one wall-clock second, and then spits out .csv-format timing information to stdout. Backported PR: - bitcoin#6733 - bitcoin#6770 - bitcoin#6892 - bitcoin#8039 - bitcoin#8107 - bitcoin#8115 - bitcoin#9200 - bitcoin#9202 - bitcoin#9281 - bitcoin#6361 - bitcoin#10271 - bitcoin#9498 - bitcoin#9712 - bitcoin#9547 - bitcoin#9505 (benchmark only. Rest was in #1557) - bitcoin#9792 (benchmark only. Rest was in #643) - bitcoin#10272 - bitcoin#10395 (base58 only) - bitcoin#10963 - bitcoin#11303 (first commit) - bitcoin#11562 - bitcoin#11646 - bitcoin#11654 Current output of `src/bench/bench_pivx`: ``` #Benchmark,count,min(ns),max(ns),average(ns),min_cycles,max_cycles,average_cycles Base58CheckEncode,131072,7697,8065,7785,20015,20971,20242 Base58Decode,294912,3305,3537,3454,8595,9198,8981 Base58Encode,180224,5498,6020,5767,14297,15652,14994 CCheckQueueSpeed,320,3159960,3535173,3352787,8216030,9191602,8717388 CCheckQueueSpeedPrevectorJob,96,9184484,11410840,10823070,23880046,29668680,28140445 FastRandom_1bit,320,3143690,4838162,3199156,8173726,12579373,8317941 FastRandom_32bit,60,17097612,17923669,17367440,44454504,46602306,45156079 PrevectorClear,3072,334741,366618,346731,870340,953224,901516 PrevectorDestructor,2816,344233,368912,357281,895022,959187,928948 RIPEMD160,288,3404503,3693917,3577774,8851850,9604334,9302363 SHA1,384,2718128,2891558,2802513,7067238,7518184,7286652 SHA256,176,6133760,6580005,6239866,15948035,17108376,16223916 SHA512,240,4251468,4358706,4313463,11054006,11332826,11215186 Sleep100ms,10,100221470,100302411,100239073,260580075,260790726,260625870 ``` NOTE: Not all the tests have been pulled yet (as we might not have the code being tested, or it would require rewrites to work with our different code base), but the framework is updated to December 2017. ACKs for top commit: Fuzzbawls: ACK 3f3edde Tree-SHA512: c283311a9accf6d2feeb93b185afa08589ebef3f18b6e86980dbc3647b9845f75ac9ecce2f1b08738d25ceac36596a2c89d41e4dbf3b463502aa695611aa1f8e

fa94614 [doc] dev-notes: Members should be initialized (MarcoFalke) Pull request description: Also, remove mention of threads that were removed long ago. Motivation: Make it easier to spot bugs such as bitcoin#11654 and bitcoin#12426 Tree-SHA512: 8ca1cb54e830e9368803bd98a8b08c39bf2d46f079094ed7e070b32ae15a6e611ce98d7a614f897803309f4728575e6bc9357fab1157c53d2536417eb8271653

@laanwj

… class member lastCycles to zero in constructor 069215e Initialize recently introduced non-static class member lastCycles to zero in constructor (practicalswift) Pull request description: Initialize recently introduced non-static class member `lastCycles` to zero in constructor. `lastCycles` was introduced in 3532818 which was merged into master yesterday. Friendly ping @laanwj :-) Tree-SHA512: cb93b6a8f6e2e3b06cd05a635da95c84f3df64c21fc23fe82f98306ea571badc32040315b563e46ddb5203128226bc334269acd497beead5a5777c434060fd85

fanquake added the Tests label Nov 10, 2017

practicalswift changed the title ~~Initialize recently introduced non-static class member lastCycles to zero in constructor~~ tests: Initialize recently introduced non-static class member lastCycles to zero in constructor Nov 10, 2017

laanwj reviewed Nov 11, 2017

View reviewed changes

practicalswift force-pushed the uninitialized-members branch from ac78e5c to 22ce5f1 Compare November 12, 2017 15:38

maflcko reviewed Nov 13, 2017

View reviewed changes

practicalswift force-pushed the uninitialized-members branch from 22ce5f1 to 4255c30 Compare November 13, 2017 21:32

Initialize recently introduced non-static class member lastCycles to …

069215e

…zero in constructor lastCycles was introduced in 3532818 which was merged into master yesterday. Also initialize beginCycles to zero for consistency and completeness.

practicalswift force-pushed the uninitialized-members branch from 4255c30 to 069215e Compare November 13, 2017 21:37

laanwj merged commit 069215e into bitcoin:master Nov 16, 2017

maflcko mentioned this pull request Feb 14, 2018

[doc] dev-notes: Members should be initialized #12434

Merged

str4d mentioned this pull request Feb 22, 2019

Micro-benchmarking framework part 1 zcash/zcash#3858

Merged

random-zebra mentioned this pull request May 27, 2020

[Tests] Benchmarking Framework PIVX-Project/PIVX#1650

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Initialize recently introduced non-static class member lastCycles to zero in constructor #11654

tests: Initialize recently introduced non-static class member lastCycles to zero in constructor #11654

practicalswift commented Nov 10, 2017 •

edited

promag commented Nov 10, 2017

laanwj Nov 11, 2017

practicalswift commented Nov 12, 2017

maflcko Nov 13, 2017

practicalswift Nov 13, 2017

practicalswift commented Nov 13, 2017

maflcko commented Nov 13, 2017

		@@ -65,6 +65,7 @@ namespace benchmark {
		minCycles = std::numeric_limits<uint64_t>::max();

tests: Initialize recently introduced non-static class member lastCycles to zero in constructor #11654

tests: Initialize recently introduced non-static class member lastCycles to zero in constructor #11654

Conversation

practicalswift commented Nov 10, 2017 • edited

promag commented Nov 10, 2017

laanwj Nov 11, 2017

Choose a reason for hiding this comment

practicalswift commented Nov 12, 2017

maflcko Nov 13, 2017

Choose a reason for hiding this comment

practicalswift Nov 13, 2017

Choose a reason for hiding this comment

practicalswift commented Nov 13, 2017

maflcko commented Nov 13, 2017

practicalswift commented Nov 10, 2017 •

edited