Skip to content

Run channel state tests sequentially#2271

Merged
pm47 merged 4 commits intomasterfrom
split-tests
May 17, 2022
Merged

Run channel state tests sequentially#2271
pm47 merged 4 commits intomasterfrom
split-tests

Conversation

@pm47
Copy link
Member

@pm47 pm47 commented May 16, 2022

From scalatest's ParallelTestExecution doc:

ScalaTest's normal approach for running suites of tests in parallel is to run different suites in parallel, but the tests of any one suite sequentially.
[...]
To make it easier for users to write tests that run in parallel, this trait runs each test in its own instance of the class. Running each test in its own instance enables tests to use the same instance vars and mutable objects referenced from instance variables without needing to synchronize. Although ScalaTest provides functional approaches to factoring out common test code that can help avoid such issues, running each test in its own instance is an insurance policy that makes running tests in parallel easier and less error prone.

This means that for each single test of the channel.states package, we instantiate one actor system, which contains two thread pools. With default settings, that's a minimum of 2*8 threads per individual test.

That's already pretty bad, and with 65cf238 (#2270) we add a factor of 3 on top of that, which makes us go past the OS limits on github CI.

setup peak thread count run time
baseline 5447 5m 44s
sequential 1927 5m 9s (*)
sequential + split 1724 5m 20s (**)
sequential + split + 65cf238 2129 5m 6s

(*) It's actually so bad, that tests run actually faster without parallelization!

(**) I tried to split large test files (e.g. NormalStateSpec, ClosingStateSpec) in order to still preserve some parallelization. In theory it should have been faster and used a bit more threads than the full sequential, but I guess it depends on the run.

@pm47 pm47 requested a review from t-bast May 16, 2022 14:40
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #2271 (ca7a3ba) into master (10eb9e9) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2271      +/-   ##
==========================================
+ Coverage   84.31%   84.36%   +0.04%     
==========================================
  Files         194      194              
  Lines       14394    14394              
  Branches      570      570              
==========================================
+ Hits        12136    12143       +7     
+ Misses       2258     2251       -7     
Impacted Files Coverage Δ
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 85.60% <0.00%> (-0.76%) ⬇️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 88.27% <0.00%> (-0.24%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 74.17% <0.00%> (+1.32%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 92.45% <0.00%> (+1.50%) ⬆️
...q/eclair/channel/publish/ReplaceableTxFunder.scala 89.52% <0.00%> (+2.61%) ⬆️

@pm47 pm47 merged commit 8e2fc7a into master May 17, 2022
@pm47 pm47 deleted the split-tests branch May 17, 2022 08:03
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.

3 participants