fix(runtime-params-estimator): disable rocksdb thread in it #3091

ailisp · 2020-08-06T18:54:37Z

Thanks to an external contributor, we're able to use all export apis to disable rocksdb threads in runtime param estimator

Test Plan

Observe only one thread during run runtime params estimator

gitpod-io · 2020-08-06T18:54:41Z

ailisp · 2020-08-06T21:48:38Z

Seems parameter estimator get stuck in the middle after turn off all threads, probably due to there's limit of how big the state it can have. will see if i can increase the bar to proceed.

Update: have some little clue with debug print and lldb, it is not blocking by rocksdb writing limits (have increased all of them to unlimited or very high). Looking at debug print, measure_actions never returns. Looking at lldb after it stucks, rockdb wait for a ConditionVar in the rocksdb destructor (but since there's no other thread there's no way to make progress), so avoid drop RuntimeTestbed to avoid trigger rocksdb destructor might help.
Then I have a look at rocksdb source code of DBImpl::CloseHelper, there are a few cond vars there, need add more print there to figure out what is blocked. I guess there's inconsistency in rocksdb options, although turn off all threads, there's other option make it think it need wait for some background work done before exit.

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x00007fff7f96886a libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007fff7fa2756e libsystem_pthread.dylib`_pthread_cond_wait + 722
    frame #2: 0x00000001036f355f runtime-params-estimator`rocksdb::port::CondVar::Wait() + 15
    frame #3: 0x00000001036aa677 runtime-params-estimator`rocksdb::InstrumentedCondVar::Wait() + 167
    frame #4: 0x0000000103561e44 runtime-params-estimator`rocksdb::DBImpl::CloseHelper() + 404
    frame #5: 0x00000001035629e9 runtime-params-estimator`rocksdb::DBImpl::~DBImpl() + 57
    frame #6: 0x00000001035633fe runtime-params-estimator`rocksdb::DBImpl::~DBImpl() + 14
    frame #7: 0x000000010352f767 runtime-params-estimator`rocksdb_close + 23
    frame #8: 0x000000010318bce8 runtime-params-estimator`core::ptr::drop_in_place::hb76b33f569232c9f + 520
    frame #9: 0x000000010318bdcf runtime-params-estimator`core::ptr::drop_in_place::hd364f2154be9f2ec + 15
    frame #10: 0x000000010318c130 runtime-params-estimator`alloc::sync::Arc$LT$T$GT$::drop_slow::h11db2b232d66dcbc + 32
    frame #11: 0x000000010318c2bf runtime-params-estimator`alloc::sync::Arc$LT$T$GT$::drop_slow::h3ce12527033b0af5 + 31
    frame #12: 0x0000000103346286 runtime-params-estimator`core::ptr::drop_in_place::hf435a7491c6c6bba + 54

Update: work around by set thread to 4 at drop rocksdb, i think it's okay to do that since metric recording is already finished at that time

codecov · 2020-08-07T18:33:12Z

Codecov Report

Merging #3091 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3091   +/-   ##
=======================================
  Coverage   87.46%   87.46%           
=======================================
  Files         212      212           
  Lines       41452    41452           
=======================================
  Hits        36256    36256           
  Misses       5196     5196

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d20343c...30666e5. Read the comment docs.

bowenwang1996

Why couldn't we only use it for parameter estimator, i.e, putting this change behind some feature flag?

runtime/runtime-params-estimator/src/cases.rs

evgenykuzyakov · 2020-08-07T20:41:16Z

Why couldn't we only use it for parameter estimator, i.e, putting this change behind some feature flag?

Actually, I agree with @bowenwang1996 on this. But it also can be useful not only for param estimator, so some generic feature name will be good

bowenwang1996

we should only use the our fork of rocksdb when the single thread feature is enabled.

bowenwang1996 · 2020-08-10T18:25:09Z

chain/chain/Cargo.toml

@@ -10,7 +10,7 @@ log = "0.4"
 failure = "0.1"
 failure_derive = "0.1"
 lazy_static = "1.4"
-rocksdb = "0.14"
+rocksdb = { git = "https://github.com/nearprotocol/rust-rocksdb", branch="disable-thread" }


It use facebook/rocksdb#7191 (merged, but rust-rocksdb haven't bump version) and rust-rocksdb/rust-rocksdb#448 (approved, but need to wait next time rust-rocksdb bump version). Our fork is equal to rust-rocksdb/rust-rocksdb#448

frol · 2020-08-10T18:45:03Z

core/store/src/db.rs

-    opts.set_max_total_wal_size(1 * 1024 * 1024 * 1024);
+    #[cfg(not(feature = "single_thread_rocksdb"))]
+    {
+        println!("Use background threads in rocksdb");


Should we remove this log message or convert it into a proper tracing::info/debug/trace?

Seems we didn't log anything in db config module in neard, so I'll remove it to be consistent

frol · 2020-08-10T18:45:16Z

core/store/src/db.rs

+            env.set_high_priority_background_threads(0);
+            env.set_low_priority_background_threads(0);
+            env.set_background_threads(0);
+            println!("Disabled all background threads in rocksdb");


I prefer to keep this one to as print. This is only showup in param estimator, which only used print, one log line between the raw prints looks not good

ailisp added 4 commits July 10, 2020 17:42

fix(runtime-params-estimator): disable rocksdb thread in it

a90b5c6

options have no use

0eff52b

merge master resolve conflict

7b9b262

use disablable thread branch of rocksdb

d024aee

disable thread works

3bdebab

ailisp requested review from MaksymZavershynskyi, olonho and willemneal August 6, 2020 19:42

ailisp marked this pull request as ready for review August 6, 2020 19:49

ailisp requested review from bowenwang1996, evgenykuzyakov, frol and SkidanovAlex as code owners August 6, 2020 19:49

replace depreciate apis

763e8a7

ailisp added 2 commits August 6, 2020 16:33

config attempts

8672be6

set background thread after measure so rocksdb doesn't stuck on cv

9b2bc62

frol approved these changes Aug 7, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Aug 7, 2020

Weekly Digest (31 July, 2020 - 7 August, 2020) #3100

Closed

ailisp added 3 commits August 7, 2020 10:13

turn on thread on db drop

8590679

fix typo

4b0ffe4

remove unused import

41711d6

bowenwang1996 reviewed Aug 7, 2020

View reviewed changes

willemneal approved these changes Aug 7, 2020

View reviewed changes

runtime/runtime-params-estimator/src/cases.rs Outdated Show resolved Hide resolved

remove debug print

6d9b74a

evgenykuzyakov approved these changes Aug 7, 2020

View reviewed changes

use feature to compile params estimator

2517a9e

ailisp added 4 commits August 7, 2020 16:42

fix typo

4f028a6

fix warning

0c6288e

fix warnings

f878ae0

fix a build error

f35c653

ailisp requested a review from bowenwang1996 August 10, 2020 18:24

bowenwang1996 reviewed Aug 10, 2020

View reviewed changes

frol reviewed Aug 10, 2020

View reviewed changes

remove print in neard

ad77cc5

bowenwang1996 approved these changes Aug 11, 2020

View reviewed changes

Merge branch 'master' into disable-thread-rocksdb

96aa6d3

ailisp added the automerge label Aug 11, 2020

Merge refs/heads/master into disable-thread-rocksdb

30666e5

nearprotocol-bulldozer bot merged commit 888da94 into master Aug 11, 2020

nearprotocol-bulldozer bot deleted the disable-thread-rocksdb branch August 11, 2020 17:11

bowenwang1996 mentioned this pull request Aug 12, 2020

parallel test runner enables nondefault feature #3146

Closed

weekly-digest bot mentioned this pull request Aug 14, 2020

Weekly Digest (7 August, 2020 - 14 August, 2020) #3163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(runtime-params-estimator): disable rocksdb thread in it #3091

fix(runtime-params-estimator): disable rocksdb thread in it #3091

ailisp commented Aug 6, 2020 •

edited

gitpod-io bot commented Aug 6, 2020 •

edited

ailisp commented Aug 6, 2020 •

edited

codecov bot commented Aug 7, 2020 •

edited

bowenwang1996 left a comment

evgenykuzyakov commented Aug 7, 2020

bowenwang1996 left a comment

bowenwang1996 Aug 10, 2020

ailisp Aug 10, 2020

frol Aug 10, 2020

ailisp Aug 10, 2020

frol Aug 10, 2020

ailisp Aug 10, 2020 •

edited

fix(runtime-params-estimator): disable rocksdb thread in it #3091

fix(runtime-params-estimator): disable rocksdb thread in it #3091

Conversation

ailisp commented Aug 6, 2020 • edited

Test Plan

gitpod-io bot commented Aug 6, 2020 • edited

ailisp commented Aug 6, 2020 • edited

codecov bot commented Aug 7, 2020 • edited

Codecov Report

bowenwang1996 left a comment

Choose a reason for hiding this comment

evgenykuzyakov commented Aug 7, 2020

bowenwang1996 left a comment

Choose a reason for hiding this comment

bowenwang1996 Aug 10, 2020

Choose a reason for hiding this comment

ailisp Aug 10, 2020

Choose a reason for hiding this comment

frol Aug 10, 2020

Choose a reason for hiding this comment

ailisp Aug 10, 2020

Choose a reason for hiding this comment

frol Aug 10, 2020

Choose a reason for hiding this comment

ailisp Aug 10, 2020 • edited

Choose a reason for hiding this comment

ailisp commented Aug 6, 2020 •

edited

gitpod-io bot commented Aug 6, 2020 •

edited

ailisp commented Aug 6, 2020 •

edited

codecov bot commented Aug 7, 2020 •

edited

ailisp Aug 10, 2020 •

edited