feat: use napi-rs pubkey-index-map

twoeths commented 1 week ago

Motivation

reduce heap memory for Pubkey-Index map
no intermediate string creation when looking for index of an Uint8Array pubkey

Description

consume "@chainsafe/pubkey-index-map napi-rs implementation

benchmark on Mac M1 is comparable to the current PubkeyIndexMap:

  get/set
    ✓ get values - 1000                                                    2808989 ops/s    356.0000 ns/op   x0.450    1313217 runs  0.707 s
    ✓ get values - naive - 1000                                            3802281 ops/s    263.0000 ns/op   x0.358    1423603 runs  0.606 s
    ✓ set values - 1000                                                    2932551 ops/s    341.0000 ns/op   x0.432    2900838 runs   1.51 s
    ✓ set values - naive - 1000                                            2136752 ops/s    468.0000 ns/op   x0.497     637433 runs  0.404 s
    ✓ get values - 1000000                                                948766.6 ops/s    1.054000 us/op   x0.568     407133 runs  0.505 s
    ✓ get values - naive - 1000000                                         1254705 ops/s    797.0000 ns/op   x0.680     627761 runs  0.606 s
    ✓ set values - 1000000                                                972762.6 ops/s    1.028000 us/op   x0.544     499266 runs  0.606 s
    ✓ set values - naive - 1000000                                        637755.1 ops/s    1.568000 us/op   x0.805     346993 runs  0.606 s

github-actions[bot] commented 1 week ago

:warning: Performance Alert :warning:

Possible performance regression was detected for some benchmarks. Benchmark result of this commit is worse than the previous benchmark result exceeding threshold.

Benchmark suite	Current: db5d962f247e4f74bbe31a0e9c713dffa65d5a4e	Previous: cd98c237683456be7959d752bbd7d10f8b02b8ec	Ratio
getExpectedWithdrawals 250000 eb:0,eth1:1,we:0,wn:0,nocache,smpl:16384	9.0531 ms/op	2.7366 ms/op	3.31
Array.fill - length 1000000	8.3394 ms/op	2.4610 ms/op	3.39
Array push - length 1000000	52.419 ms/op	14.302 ms/op	3.67
phase0 processEffectiveBalanceUpdates - 250000 worstcase 0.5	7.4445 ms/op	1.4955 ms/op	4.98
Buffer.compare 123687377	12.803 ms/op	3.7482 ms/op	3.42

Full benchmark results

by benchmarkbot/action

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 46.15385% with 7 lines in your changes missing coverage. Please review.

Project coverage is 50.82%. Comparing base (cd98c23) to head (0edc38f). Report is 13 commits behind head on unstable.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## unstable #7091 +/- ## ============================================ - Coverage 50.84% 50.82% -0.02% ============================================ Files 597 597 Lines 39835 39827 -8 Branches 2069 2060 -9 ============================================ - Hits 20256 20244 -12 - Misses 19579 19583 +4 ```

twoeths commented 1 week ago

gc has been reduced with this PR. I guess that's because we don't have to convert to string for the map thanks to the default trait implementation Rust, this is amazing @wemeetagain

this is on a mainnet node

wemeetagain commented 5 days ago

Only thing we need to ensure is that @chainsafe/pubkey-index-map supports all the necessary platforms we want to support

twoeths commented 4 days ago

Only thing we need to ensure is that @chainsafe/pubkey-index-map supports all the necessary platforms we want to support

my understand is @chainsafe/pubkey-index-map does not have native dependencies like in @chainsafe/blst or @chainsafe/hashtree so perhaps we're safe

would like @matthewkeil to confirm this, this is the same situation to the napi-rs work we're gonna do for epoch shuffling computation

matthewkeil commented 1 day ago

Only thing we need to ensure is that @chainsafe/pubkey-index-map supports all the necessary platforms we want to support

my understand is @chainsafe/pubkey-index-map does not have native dependencies like in @chainsafe/blst or @chainsafe/hashtree so perhaps we're safe

would like @matthewkeil to confirm this, this is the same situation to the napi-rs work we're gonna do for epoch shuffling computation

Platforms look ok. I would think we do not want to support musl because of the performance aspect but not sure how important that is relative to having "support for everything"... 🤷‍♂️ I think its ok as is though.

twoeths commented 22 hours ago

metrics after ~9h of deployment on the unstable mainnet node

both heap and gc are improved a little bit

ChainSafe / lodestar

feat: use napi-rs pubkey-index-map #7091

:warning: Performance Alert :warning:

Codecov Report