Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unmodularize GoLevelDB + use build tags for tests. #229

Closed
wants to merge 2 commits into from
Closed

Unmodularize GoLevelDB + use build tags for tests. #229

wants to merge 2 commits into from

Conversation

johnsonjh
Copy link

@johnsonjh johnsonjh commented Nov 24, 2020

  • Demodularize GoLevelDB, fully integrate in-tree
  • Use "long_tests" build tag to separate out all long-running tests
  • Go go list for deterministic ordering and to eliminate any multiple inclusions for testing and coverage speed-up
  • Exclude test directories from tests & coverage
  • Fix script EOF's with POSIX-compliant newlines
  • Add shellcheck linter hinting, as appropriate
  • Neutrino: non-default ALL_TESTS -> neutrino_long_tests tag (usage still relies on fixing the test harness, as before)
  • do builds to default to short testing path, unless LONG_TESTS is defined; coverage will use the long testing path by default
  • Avoid scripts shadowing predeclared identifiers

* Demodularize GoLevelDB, fully integrate in-tree
* Use "long_tests" build tag to separate out all
  long running tests.
* Go `go list` for deterministic ordering and to
  eliminate any multiple inclusions for speed-up.
* Exclude test directories from tests & coverage.
* Fix script EOF's with POSIX-compliant newlines.
* Add shellcheck linter hinting, as appropriate.
* Neutrino: ALL_TESTS -> neutrino_long_tests tag.
* `do` builds to default to short testing path,
  unless LONG_TESTS is defined, coverage will use
  the long testing path by default.
@johnsonjh
Copy link
Author

johnsonjh commented Nov 24, 2020

@cjdelisle

@johnsonjh
Copy link
Author

Auto-merging pktwallet/wallet/createtx_test.go
Auto-merging neutrino/sync_test.go
Auto-merging neutrino/blockmanager_test.go
Auto-merging neutrino/banman/store_test.go
Auto-merging neutrino/bamboozle_unit_test.go
Auto-merging goleveldb/leveldb/version_test.go
Auto-merging goleveldb/leveldb/db_test.go
Removing goleveldb/go.sum
Removing goleveldb/go.mod
Auto-merging database/ffldb/driver_test.go
Auto-merging database/ffldb/bench_test.go
Auto-merging blockchain/fullblocks_test.go
Merge made by the 'recursive' strategy.
 blockchain/bench_test.go                           |  2 +
 blockchain/fullblocks_test.go                      |  2 +
 btcec/bench_test.go                                |  2 +
 btcec/btcec_test.go                                |  2 +
 btcec/signature_test.go                            |  2 +
 btcutil/base58/base58bench_test.go                 |  2 +
 btcutil/gcs/gcsbench_test.go                       |  2 +
 btcutil/hdkeychain/bench_test.go                   |  2 +
 cov_report.sh                                      | 42 +++++--------
 database/ffldb/bench_test.go                       |  2 +
 database/ffldb/driver_test.go                      |  2 +
 do                                                 | 22 ++++---
 go.mod                                             | 11 +++-
 go.sum                                             |  2 -
 goleveldb/go.mod                                   | 14 -----
 goleveldb/go.sum                                   | 73 ----------------------
 goleveldb/leveldb/batch_test.go                    |  2 +
 goleveldb/leveldb/bench_test.go                    |  2 +
 goleveldb/leveldb/cache/bench_test.go              |  2 +
 goleveldb/leveldb/cache/cache_test.go              |  2 +
 goleveldb/leveldb/corrupt_test.go                  |  2 +
 goleveldb/leveldb/db_test.go                       |  2 +
 goleveldb/leveldb/external_test.go                 |  2 +
 goleveldb/leveldb/key_test.go                      |  2 +
 goleveldb/leveldb/leveldb_suite_test.go            |  2 +
 goleveldb/leveldb/memdb/bench_test.go              |  2 +
 goleveldb/leveldb/memdb/memdb_suite_test.go        |  2 +
 goleveldb/leveldb/memdb/memdb_test.go              |  2 +
 goleveldb/leveldb/session_record_test.go           |  2 +
 goleveldb/leveldb/table/block_test.go              |  2 +
 goleveldb/leveldb/table/table_suite_test.go        |  2 +
 goleveldb/leveldb/table/table_test.go              |  2 +
 goleveldb/leveldb/table_test.go                    |  2 +
 goleveldb/leveldb/testutil_test.go                 |  2 +
 goleveldb/leveldb/util/buffer_pool_test.go         |  2 +
 goleveldb/leveldb/util/buffer_test.go              |  2 +
 goleveldb/leveldb/util/hash_test.go                |  2 +
 goleveldb/leveldb/version_test.go                  |  2 +
 neutrino/bamboozle_unit_test.go                    |  2 +
 neutrino/banman/store_test.go                      |  2 +
 neutrino/blockmanager_test.go                      |  2 +
 neutrino/sync_test.go                              |  6 +-
 .../internal/legacy/keystore/keystore_test.go      |  2 +
 pktwallet/internal/rpchelp/genrpcserverhelp.go     |  2 +-
 pktwallet/internal/zero/benchmark_test.go          |  2 +
 pktwallet/wallet/createtx_test.go                  |  2 +
 pktwallet/wallet/seedwords/seed_test.go            |  2 +
 wire/bench_test.go                                 |  2 +
 48 files changed, 117 insertions(+), 135 deletions(-)
 delete mode 100644 goleveldb/go.mod
 delete mode 100644 goleveldb/go.sum

@johnsonjh
Copy link
Author

johnsonjh commented Nov 27, 2020

Part 1: Impact to existing testing and current code coverage

Benchmark results for ./do
(combined build and test timings)

BEFORE:

  • Initial build with test run, no filesystem or toolchain build caching, base develop: 111.71s (1m51.71s)
    • User time (seconds): 102.19s
    • System time (seconds): 9.52s

  • Rebuild with test run, allowing filesystem and toolchain build caching, base develop: 70.51s (1m10.51s)
    • User time (seconds): 65.34s
    • System time (seconds): 5.17s

Adjusted build time value ⁜ 1m39.1s


AFTER:


Adjusted build time value ⁜ 53.6s


RESULT:

Adjusted percentage change ‡ 45.9% decrease
Adjusted percentage difference ‡ 59.6% difference


CONCLUSION:

  • The current testing configuration, as calculated by gocov, excluding GoLevelDB, results in 51.92% coverage (16,609 out of 31,988 statements).

  • The new testing configuration, as calculated by gocov, excluding GoLevelDB and tests identified as long running†, results in 48.64% coverage (15,558 out of 31,988 statements), and a speed increase of ~1.85 X.

Code coverage percentage change ‡ 6.3% decrease
Code coverage percentage difference ‡ 6.5% difference


Overall coverage reduction ~6.4%
Overall speed increase ~92%


ADDENDUM:

  • The following tests (excluding benchmarks) were identified as long running† and are now excluded by default:

    • blockchain/fullblocks
    • btcec/btcec
    • btcec/signature
    • database/ffldb/driver
    • neutrino/banman/store
    • pktwallet/internal/legacy/keystore/keystore
    • pktwallet/wallet/createtx
    • pktwallet/wallet/seedwords/seed

Part 2: Impact of new testing and increased code coverage

Benchmark results for ./do
(combined build and test timings)
(LONG_TESTS† enabled)

RESULT:


Adjusted build time value8m22.8s


CONCLUSION:

  • LONG_TESTS† now enables the GoLevelDB test suite.
  • Overall test coverage is increased to 56.03% (21,470 out of 38,320 statements).

GoLevelDB test coverage is currently 82.59%


DEFINITIONS:

  • LONG_TESTS enables the long_tests conditional build tag. These are tests that take longer than 1.01s to complete [averaged over 2,500 runs on the test system], or which exceed average runtime of other tests by at least two orders of magnitude. In addition, the GoLevelDB test suite is enabled with LONG_TESTS. This test suite is either I/O-wait-time bound or memory bandwidth bound, depending on the test system configuration (tmp mounted on tmpfs vs. a physical disk). On the reference test system (Intel Core i7-8700, 64GB DDR4-2667), the standard average runtime of this test suite, based on 600 iterations, using tmp mounted on tmpfs, is 7m15s.

  • ⁜ Adjusted build time values are calculated as an average of the median of combined user and system timings added to the calculated harmonic mean, adjusted via standard averaging against the test iteration count, and the result reduced in precision to the first decimal place using the Object Management Group's standard rounding methodology.

  • ‡ The adjusted percentage change values are calculated as the difference of the adjusted build time values as described above, divided by the absolute value of the original value, multiplied by 100, with the result reduced in precision to the first decimal place using the OMG standard rounding methodology. The adjusted percentage difference values are calculated based on the difference of the adjusted percentage change values, divided by the absolute value of the original value, multiplied by 100, with the result reduced in precision to the first decimal place using the OMG standard rounding methodology.

* Remove gocov_report_pktd.json in cleanUp()
This was referenced Dec 19, 2020
@johnsonjh johnsonjh closed this Dec 28, 2020
@johnsonjh johnsonjh deleted the ldbmerge+fasttest branch December 28, 2020 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant