Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Hunter: Use upstream leveldb 1.22 #5532

Merged
merged 4 commits into from
Jun 7, 2019
Merged

Hunter: Use upstream leveldb 1.22 #5532

merged 4 commits into from
Jun 7, 2019

Conversation

chfast
Copy link
Member

@chfast chfast commented Mar 26, 2019

No description provided.

@halfalicious
Copy link
Contributor

FYI this version of LevelDB isn't backwards compatible, I get a "corrupted compresssed block contents" exception when trying to sync using my existing database (but presumably this is expected since from what I remember the cached version that we're using on Windows is very very old).

Other than that it appears to work fine...I can sync from scratch and stop/resume sync fine. Merge it! 😄

@halfalicious
Copy link
Contributor

All blockchain tests passed on Windows though runtimes were ~50% slower on average, here are the results without the upgrade:

*** No errors detected
Total Time:                                       : 4840
bcExploitTest                                 time: 3667.71
bcRandomBlockhashTest                         time: 349.691
bcForkStressTest                              time: 152.041
bcUncleTest                                   time: 148.003
bcUncleHeaderValidity                         time: 105.717
bcGasPricerTest                               time: 93.8263
bcValidBlockTest                              time: 77.8208
bcTotalDifficultyTest                         time: 67.1224
bcStateTests                                  time: 53.3059
bcMultiChainTest                              time: 47.6339
bcInvalidHeaderTest                           time: 46.3486
bcBlockGasLimitTest                           time: 21.2276
bcUncleSpecialTests                           time: 15.9632
bcForgedTest                                  time: 0.558981

And here are the results with the upgrade:

*** No errors detected
Total Time:                                       : 6622
bcExploitTest                                 time: 5072.59
bcRandomBlockhashTest                         time: 444.637
bcForkStressTest                              time: 206.37
bcUncleTest                                   time: 203.25
bcUncleHeaderValidity                         time: 148.105
bcGasPricerTest                               time: 125.697
bcValidBlockTest                              time: 105.932
bcTotalDifficultyTest                         time: 91.8656
bcInvalidHeaderTest                           time: 66.4888
bcMultiChainTest                              time: 65.4647
bcStateTests                                  time: 51.3074
bcBlockGasLimitTest                           time: 24.0419
bcUncleSpecialTests                           time: 22.9861
bcForgedTest                                  time: 0.705761

I wonder if our current version has some special optimizations?

@chfast
Copy link
Member Author

chfast commented Mar 29, 2019

cc @jwasinger

@halfalicious does the new leveldb matches the performance of rocksdb?

I see a lot of questions here:

  1. Why is the performance lower for new leveldb.
    • I believe we used the bitcoin fork previously.
    • Snappy compression is not available (yet) in the newer one. Not sure if was available in the old one. Rather not.
    • Accelerated crc32 is not available in the newer one. Same story as with snappy.
  2. Maybe this benchmark is not a good one. Maybe the newer leveldb is faster to read.

@halfalicious
Copy link
Contributor

halfalicious commented Mar 30, 2019

cc @jwasinger

@halfalicious does the new leveldb matches the performance of rocksdb?

I see a lot of questions here:

1. Why is the performance lower for new leveldb.
   
   * I believe we used the bitcoin fork previously.
   * Snappy compression is not available (yet) in the newer one. Not sure if was available in the old one. Rather not.
   * Accelerated crc32 is not available in the newer one. Same story as with snappy.

2. Maybe this benchmark is not a good one. Maybe the newer leveldb is faster to read.

Actually, it looks like I had full page heap enabled for both the base (old version of LevelDB) and test (upgraded version of LevelDB) runs, I've disabled it and the delta shrunk to ~17%. I've also done a RocksDB run and the delta is ~69%. Here's the data:

Test LevelDB (original) LevelDB (upgraded) Delta % (original vs upgraded) RocksDB Delta % (original LDB vs RocksDB)
bcRandomBlockhashtest 169.251 197.524 +16.70477575 382.807 +126.1770979
bcExploitTest 146.287 156.533 +7.004040004 97.1758 -33.57181431
bcUncleTest 81.141 109.804 +35.32492821 110.94 +36.72496025
bcUncleHeaderValidity 55.023 103.1 +87.37618814 92.8071 +68.66964724
bcForkStressTest 81.2451 73.1317 -9.986325329 110.94 +36.54977346
bcValidBlockTest 40.6418 45.5205 +12.00414352 95.712 +135.5013804
bcGasPricerTest 46.1366 43.3969 -5.938235587 74.8996 +62.34312888
bcTotalDifficultyTest 33.6455 37.8139 +12.38917537 67.6295 +101.0060781
bcInvalidHeaderTest 25.5206 29.0938 +14.00123822 44.8248 +75.64163852
bcMultiChainTest 23.41 25.6886 +9.733447245 48.5755 +107.4989321
bcStateTests 23.1741 24.0614 +3.828843407 40.7194 +75.71081509
bcUncleSpecialTests 7.76947 10.0188 +28.95088082 14.0128 +80.3572187
bcBlockGasLimitTest 8.75234 9.48248 +8.342226193 16.592 +89.57216013
bcForgedTest 0.031796 0.134246 +322.2103409 0.022751 -28.44697446
Total 737 860 +16.68928087 1242 +68.52103121

@chfast
Copy link
Member Author

chfast commented Apr 1, 2019

Ok, it does not looks so bad now. I think we should go with it then, but let me work a bit on leveldb in Hunter. It is not perfectly integrated as it looks for dependencies in the OS which might cause some problems.

I might want to test it also on linux.

FYI, we are using this code on Windows currently: https://github.com/ethereum/leveldb. It looks pretty old. Even bitcoin renamed the repo to leveldb-old. It also does not have snappy nor crc32c integrated.

@codecov-io
Copy link

codecov-io commented May 29, 2019

Codecov Report

Merging #5532 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5532      +/-   ##
=========================================
- Coverage   62.61%   62.5%   -0.11%     
=========================================
  Files         350     350              
  Lines       29698   29649      -49     
  Branches     3344    3335       -9     
=========================================
- Hits        18595   18532      -63     
- Misses       9893    9909      +16     
+ Partials     1210    1208       -2

@chfast chfast changed the title Hunter: Use upstream leveldb, commit 7035af5f… Hunter: Use upstream leveldb 1.22 May 29, 2019
@chfast
Copy link
Member Author

chfast commented May 29, 2019

Rebased on #5506

@chfast
Copy link
Member Author

chfast commented May 29, 2019

@halfalicious can you retest it?

@halfalicious halfalicious self-assigned this Jun 6, 2019
@halfalicious
Copy link
Contributor

halfalicious commented Jun 6, 2019

@halfalicious can you retest it?

Apologies for the delay here, didn't notice this until you pinged me on Discord. Re-ran the blockchain tests both with and without the upgrade, here are the new numbers (these are significantly lower than the previous run times since I built RelWithDebInfo instead of Debug which I think makes more sense given that's the build type that users will be running):

Test LevelDB (original) LevelDB (upgraded) Delta % (original vs upgraded)
bcExploitTest 85.74 96.32 +10.58 (+12.33%)
bcForkStressTest 14.17 16.03 +1.86 (+13.14%)
bcRandomBlockhashTest 9.87 11.56 +1.69 (+17.13%)
bcUncleTest 5.21 6.39 +1.19 (+22.74%)
bcTotalDifficultyTest 4.15 5.04 +0.89 (+21.32%)
bcGasPricerTest 4.45 4.74 +0.29 (+6.47%)
bcUncleHeaderValidity 3.71 4.25 +0.54 (+14.41%)
bcMultiChainTest 2.47 2.95 +0.48 (+19.34%)
bcValidBlockTest 2.11 2.93 +0.83 (+38.50%)
bcStateTests 1.73 1.84 +0.11 (+6.22%)
bcInvalidHeaderTest 1.30 1.54 +0.21 (+16.14%)
bcBlockGasLimitTest 0.73 0.80 +0.08 (+10.57%)
bcUncleSpecialTests 0.50 0.61 +0.11 (+21.35%)
bcForgedTest 0.02 0.03 +0.01 (+56.16%)
Total 130 148 +18 (+13.85%)

@chfast
Copy link
Member Author

chfast commented Jun 6, 2019

Thanks. Although newer version is consistently slower, I'm for merging this.

Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Should we delete scripts/install_deps.bat / cmake/scripts/install_deps.cmake files?

@gumb0
Copy link
Member

gumb0 commented Jun 6, 2019

Also I think it's worth a changelog item. With a mention that the old database won't work and should be deleted.

@halfalicious
Copy link
Contributor

halfalicious commented Jun 7, 2019

Made it through ~2.3M blocks on mainnet, everything looks good so I think we can merge (after addressing @gumb0 's items). We should also update README.md so that it no longer references scripts\install_deps.bat

@chfast chfast merged commit 505aead into master Jun 7, 2019
@chfast chfast deleted the hunter-leveldb branch June 7, 2019 14:18
@chfast chfast mentioned this pull request Jun 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants