hashgraph / hedera-services

Crypto, token, consensus, file, and smart contract services for the Hedera public ledger
Apache License 2.0
313 stars 136 forks source link

perf: Create platform ForkJoinPool with asyncMode=true #14068

Closed OlegMazurov closed 4 months ago

OlegMazurov commented 4 months ago

Description: Port #13968 to release/0.51.

Related issue(s):

Fixes #13967

Notes for reviewer:

Checklist

github-actions[bot] commented 4 months ago

Node: HAPI Test (Node Death Reconnect) Results

3 tests   3 :white_check_mark:  7m 11s :stopwatch: 3 suites  0 :zzz: 3 files    0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Restart) Results

3 tests   3 :white_check_mark:  8m 50s :stopwatch: 3 suites  0 :zzz: 3 files    0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Token) Results

 20 files   20 suites   5m 56s :stopwatch: 257 tests 257 :white_check_mark: 0 :zzz: 0 :x: 260 runs  260 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Crypto) Results

 23 files   23 suites   11m 46s :stopwatch: 351 tests 351 :white_check_mark: 0 :zzz: 0 :x: 357 runs  357 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Misc) Results

 51 files   51 suites   22m 13s :stopwatch: 357 tests 357 :white_check_mark: 0 :zzz: 0 :x: 374 runs  374 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Smart Contract) Results

 69 files   69 suites   21m 55s :stopwatch: 610 tests 610 :white_check_mark: 0 :zzz: 0 :x: 638 runs  638 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: HAPI Test (Time Consuming) Results

19 tests   19 :white_check_mark:  22m 5s :stopwatch:  4 suites   0 :zzz:  4 files     0 :x:

Results for commit 0ea3c6df.

github-actions[bot] commented 4 months ago

Node: Unit Test Results

  2 319 files  ±0    2 319 suites  ±0   3h 25m 33s :stopwatch: + 2m 59s 112 677 tests ±0  112 608 :white_check_mark: ±0  69 :zzz: ±0  0 :x: ±0  121 185 runs  ±0  121 116 :white_check_mark: ±0  69 :zzz: ±0  0 :x: ±0 

Results for commit 0ea3c6df. ± Comparison against base commit bcf069fd.

:recycle: This comment has been updated with latest results.

OlegMazurov commented 4 months ago

Does not have a design proposal. Also, as far as I am aware, this did not show a significant improvement in testing at scale (if I'm wrong on this, please post evidence).

In general, I think we should avoid merging changes into release branches unless they are bug fixes or verified stability enhancements.

There is no design to propose. The rationale for the change was provided in #13967. The purpose of the ForJoinPool parameter change is not to improve performance but provide more stability against the throbbing behavior (alternating between ACTIVE and CHECKING) of the broken back-pressure implementation. That there is no significant change in performance from this change, either improvement or regression, is not a bad thing in that context. We have not observed the problematic behavior in any testing environment with either 40M or 20M state with the change so there is a good reason to believe that it does help. Where there is no possibility to fix the root cause any improvement that reduces the risk should be desirable. Whether this change is appropriate for the target release should be evaluated on that basis.

tomzhenghedera commented 4 months ago

the change did help to remove the flappy behavior for the 40M state test, here is a run on 40M state, with the develop build that had had the change, although only under 2 days, it didn't show the sign of the symptom, compare to this run from v0.51.0 on 40M state. On the latest runs with 20M state it's true no visible difference with or without the change.