p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio() #22284

jonatack · 2021-06-19T16:59:25Z

This follow-up to #21261 improves ProtectEvictionCandidatesByRatio() for better performance.

Benchmarks are added; the performance improvement is between 2x and 5x for the benchmarked cases (CPU 2.50GHz, Turbo off, performance mode, Debian Clang 11 non-debug build).

$ ./src/bench/bench_bitcoin -filter="EvictionProtection*.*"

The refactored code is well-covered by existing unit tests and also a fuzzer.

$ ./src/test/test_bitcoin -t net_peer_eviction_tests
$ FUZZ=node_eviction ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/node_eviction

jonatack · 2021-06-20T14:39:43Z

Added benchmarks. This pull speeds up ProtectEvictionCandidatesByRatio() between 2x and 6x for the benchmarked cases. It's not a hotspot, but the improvement is significant.

$ ./src/bench/bench_bitcoin -filter=EvictionProtection*.*

master

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    5,403,043,444.00 |                0.19 |    1.8% |     59.74 | `EvictionProtection1Network250Candidates`
|    2,946,547,379.00 |                0.34 |    2.7% |     33.45 | `EvictionProtection3Networks050Candidates`
|    2,798,691,071.00 |                0.36 |    2.2% |     30.79 | `EvictionProtection3Networks100Candidates`
|    7,288,364,370.00 |                0.14 |    0.7% |     80.16 | `EvictionProtection3Networks250Candidates`

this PR

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    1,183,198,205.00 |                0.85 |    1.2% |     13.12 | `EvictionProtection1Network250Candidates`
|    1,151,709,186.00 |                0.87 |    1.7% |     12.67 | `EvictionProtection3Networks050Candidates`
|    1,261,170,266.00 |                0.79 |    2.9% |     14.26 | `EvictionProtection3Networks100Candidates`
|    1,157,625,808.00 |                0.86 |    1.0% |     12.95 | `EvictionProtection3Networks250Candidates`

(intermediate commits)

p2p: iterate over eviction candidates once instead of thrice

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    3,952,529,089.00 |                0.25 |    1.8% |     43.47 | `EvictionProtection1Network250Candidates`
|    2,239,435,919.00 |                0.45 |    3.8% |     25.85 | `EvictionProtection3Networks050Candidates`
|    2,060,389,113.00 |                0.49 |    1.9% |     22.85 | `EvictionProtection3Networks100Candidates`
|    5,258,681,959.00 |                0.19 |    1.0% |     58.26 | `EvictionProtection3Networks250Candidates`

p2p: iterate eviction protection only on networks having candidates

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    1,224,597,989.00 |                0.82 |    2.2% |     13.60 | `EvictionProtection1Network250Candidates`

klementtan · 2021-06-20T15:18:18Z

Tested on macOs 11.4 and this PR indeed optimize it by 1.3x to 5x.

Test result

On 272f327(master with benchmarks):

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      756,331,588.00 |                1.32 |    1.2% |      8.38 | `EvictionProtection1Network250Candidates`
|    1,579,971,776.00 |                0.63 |    1.1% |     17.45 | `EvictionProtection3Networks050Candidates`
|    1,395,431,168.00 |                0.72 |    1.0% |     15.70 | `EvictionProtection3Networks100Candidates`
|    3,716,091,921.00 |                0.27 |    1.0% |     41.32 | `EvictionProtection3Networks250Candidates`

This PR:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      492,425,456.00 |                2.03 |    0.9% |      5.45 | `EvictionProtection1Network250Candidates`
|      739,073,603.00 |                1.35 |    1.3% |      8.13 | `EvictionProtection3Networks050Candidates`
|      937,281,186.00 |                1.07 |    0.7% |     10.38 | `EvictionProtection3Networks100Candidates`
|      828,820,224.00 |                1.21 |    1.6% |      9.10 | `EvictionProtection3Networks250Candidates`

Not sure if it is an issue on my side but I had to test with ./src/bench/bench_bitcoin -filter="EvictionProtection*.*"(quotes around regex) instead of the command provided by the PR description.

DrahtBot · 2021-06-20T17:24:58Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

bench: Add OrphanTxPool benchmark #19377 by hebasto

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jarolrod

ACK ab29933

This is quite a performance improvement and is a clear change which is easy to follow. My benchmark tests confirm the performance improvement. Thanks for working on this improvement and for adding additional benchmark coverage.

Ran benchmarks upon each intermediate commit to test performance improvement at every stage. Cherry-picked each commit on top of current master and ran unit tests which each commit cherry-picked.

Master

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|    2,283,211,027.00 |                0.44 |    0.1% |     25.13 | `EvictionProtection1Network250Candidates`
|    1,417,424,933.00 |                0.71 |    0.1% |     15.59 | `EvictionProtection3Networks050Candidates`
|    1,293,719,559.00 |                0.77 |    0.1% |     14.25 | `EvictionProtection3Networks100Candidates`
|    3,350,000,724.00 |                0.30 |    0.1% |     36.88 | `EvictionProtection3Networks250Candidates`

PR

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      778,011,162.00 |                1.29 |    0.3% |      8.56 | `EvictionProtection1Network250Candidates`
|      811,555,934.00 |                1.23 |    0.1% |      8.93 | `EvictionProtection3Networks050Candidates`
|      806,194,432.00 |                1.24 |    0.1% |      8.87 | `EvictionProtection3Networks100Candidates`
|      776,191,251.00 |                1.29 |    0.1% |      8.54 | `EvictionProtection3Networks250Candidates`

Notes on commits

272f327
- Definition of test is appropriate and effective in terms of benchmarking what we care about
99a25fb
- the performance boost is from moving away from looping through each network and then interating through all eviction_candidates -> iterate through eviction_candidates once; for each candidate we iterate through add count to appropriate network
- Still appropriately counts eviction candidates per network
- Performance improvement at this stage is very modest compared to master+272f327fbfcb8bb61f8e1c9d65c665372cd60976
06b898c
- uses lambda expression in std::count_if to efficiently determine networks we have candidates from
- PR description states that "... increase the number protected per peer iteration", this would only come into play if there was a descrease in the number of networks to protect because we detected there were no candidates in that network. If you have to retouch, can you document this more clearly?
- this and 99a25fb make up the bulk of the performance improvement
4785343
- makes sense 🥃
ab29933
- Is an additional performance improvement

theStack · 2021-06-22T16:22:17Z

Concept ACK

src/bench/peer_eviction.cpp

src/net.cpp

d8513fe doc: update doc/benchmarking.md (Jon Atack) 84e2d5b bench: bench_bitcoin.cpp help fixups (Jon Atack) 10f4ce2 bench: bench.h fixes and improvements (Jon Atack) Pull request description: Fixups and updates I noticed while writing benchmarks for #22284. ACKs for top commit: za-kk: ACK d8513fe theStack: ACK d8513fe 🚤 Tree-SHA512: d494956b5d6a3329e98e8b6f4405a10613b8fce51a04bbf4493d8b3497b8d5b177c1a9a3eeb828796eb4edb92b0ace769595151e223671c0dc8f09bcf631ebb5

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

jonatack · 2021-07-06T16:46:07Z

Thank you @vasild, @jarolrod, @klementtan and @theStack; feedback taken (and rebased, apologies).

klementtan · 2021-07-07T01:41:33Z

Tested on 524aa5d

Didn't notice any significant improvement with c0d658f
d2bb623 reduced time for EvictionProtection1Network250Candidates by about 40%
Didn't notice any significant improvement with edc59d0
524aa5d reduced time for EvictionProtection3Networks050Candidates by about 50%, EvictionProtection3Networks100Candidates by about 40% and EvictionProtection3Networks250Candidates by about 75%
Not sure if it is intended but the warning message :wavy_dash: EvictionProtection3Networks100Candidates (Unstable with ~100.0 iters. Increase minEpochIterations to e.g. 1000) seems to be quite flaky

Bench Results

Note: Results are in reverse chronological order of commits

Master + Bench(edebbe7) results

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           14,383.08 |           69,526.14 |    5.1% |      0.02 | :wavy_dash: `EvictionProtection1Network250Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000)
|            5,818.85 |          171,855.12 |    0.3% |      0.01 | `EvictionProtection3Networks050Candidates`
|           13,127.23 |           76,177.56 |    1.8% |      0.01 | `EvictionProtection3Networks100Candidates`
|           92,835.73 |           10,771.71 |    7.0% |      0.09 | :wavy_dash: `EvictionProtection3Networks250Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000)

c0d658f results

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           13,408.72 |           74,578.36 |    1.7% |      0.01 | `EvictionProtection1Network250Candidates`
|            5,610.19 |          178,247.08 |    0.1% |      0.01 | `EvictionProtection3Networks050Candidates`
|           13,536.82 |           73,872.57 |    6.3% |      0.01 | :wavy_dash: `EvictionProtection3Networks100Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000)
|           87,720.01 |           11,399.91 |    2.2% |      0.09 | `EvictionProtection3Networks250Candidates`

d2bb623 results

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|            8,080.88 |          123,748.98 |    2.8% |      0.01 | `EvictionProtection1Network250Candidates`
|            5,650.52 |          176,974.71 |    0.1% |      0.01 | `EvictionProtection3Networks050Candidates`
|           13,847.64 |           72,214.45 |   11.6% |      0.01 | :wavy_dash: `EvictionProtection3Networks100Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000)
|           88,185.71 |           11,339.71 |    1.8% |      0.09 | `EvictionProtection3Networks250Candidates`

edc59d0 resutls

``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 7,874.32 | 126,995.09 | 0.9% | 0.01 | `EvictionProtection1Network250Candidates` | 5,736.20 | 174,331.44 | 1.0% | 0.01 | `EvictionProtection3Networks050Candidates` | 13,836.98 | 72,270.11 | 9.0% | 0.02 | 〰️ `EvictionProtection3Networks100Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000) | 98,370.21 | 10,165.68 | 5.4% | 0.10 | 〰️ `EvictionProtection3Networks250Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000) ```

524aa5d results

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|            7,854.82 |          127,310.36 |    0.6% |      0.01 | `EvictionProtection1Network250Candidates`
|            2,774.58 |          360,414.26 |    0.1% |      0.00 | `EvictionProtection3Networks050Candidates`
|            8,513.58 |          117,459.47 |    0.1% |      0.01 | `EvictionProtection3Networks100Candidates`
|           22,571.88 |           44,302.90 |   10.1% |      0.02 | :wavy_dash: `EvictionProtection3Networks250Candidates` (Unstable with ~100.0 iters. Increase `minEpochIterations` to e.g. 1000)

jonatack · 2021-07-07T11:23:42Z

Thanks for testing @klementtan!

Didn't notice any significant improvement with c0d658f

Thanks for pointing this out. The benchmarks don't test it. Will look at adding a bench with no protected networks.

Didn't notice any significant improvement with edc59d0

This commit doesn't help performance but fixes an off-by-one in the logic that doesn't affect the protection currently, but without it, the last commit would fail the node_eviction fuzzer by trying to save (1 - 2) = -1 into Net::count, which is size_t (unsigned long). The CI initially reported this, but I can no longer reproduce it locally (thanks @vasild for testing this) and have dropped the commit.

Not sure if it is intended but the warning message :wavy_dash: EvictionProtection3Networks100Candidates (Unstable with ~100.0 iters. Increase minEpochIterations to e.g. 1000) seems to be quite flaky

Thanks for reporting. The benchmark was sped up in the last push, but am looking at maybe increasing the iterations to improve this.

jonatack · 2021-07-08T10:25:20Z

Updated per git diff 524aa5d b1d905c

adjusted the benchmarks to use the default Epochs of 11, increased the epochIterations from 100 to 1000, and added a warmup of 100; I didn't find a magic sweet spot (I have to run it several times) but this seems to improve the err% somewhat while keeping the benchmark fast
added two benchmarks, EvictionProtection0Networks250Candidates and EvictionProtection2Networks250Candidates
dropped commit edc59d0 "p2p: don't protect more than the network candidates count"

benchmark results, CPU 2.50GHz, Turbo off, performance mode, Debian Clang 11 non-debug build, lowest err% runs of several

5adb064 bench: add peer eviction protection benchmarks

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           74,657.56 |           13,394.49 |    2.5% |      0.85 | `EvictionProtection0Networks250Candidates`
|           71,502.14 |           13,985.60 |    1.1% |      0.78 | `EvictionProtection1Networks250Candidates`
|           74,242.65 |           13,469.35 |    2.1% |      0.88 | `EvictionProtection2Networks250Candidates`
|            9,280.36 |          107,754.43 |    3.0% |      0.11 | `EvictionProtection3Networks050Candidates`
|           22,320.52 |           44,801.83 |    2.5% |      0.25 | `EvictionProtection3Networks100Candidates`
|          149,821.48 |            6,674.61 |    2.7% |      1.64 | `EvictionProtection3Networks250Candidates`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           73,715.17 |           13,565.73 |    1.9% |      0.82 | `EvictionProtection0Networks250Candidates`
|           71,505.03 |           13,985.03 |    1.7% |      0.79 | `EvictionProtection1Networks250Candidates`
|           74,924.25 |           13,346.81 |    2.6% |      0.86 | `EvictionProtection2Networks250Candidates`
|            9,493.08 |          105,339.84 |    4.4% |      0.11 | `EvictionProtection3Networks050Candidates`
|           23,528.00 |           42,502.55 |    2.7% |      0.26 | `EvictionProtection3Networks100Candidates`
|          148,696.37 |            6,725.11 |    0.6% |      1.68 | `EvictionProtection3Networks250Candidates`


b1d905c p2p: earlier continuation when no remaining eviction candidates

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           19,051.62 |           52,488.98 |    2.6% |      0.21 | `EvictionProtection0Networks250Candidates`
|           25,607.00 |           39,051.82 |    1.9% |      0.28 | `EvictionProtection1Networks250Candidates`
|           31,392.03 |           31,855.22 |    3.5% |      0.40 | `EvictionProtection2Networks250Candidates`
|            4,873.49 |          205,191.84 |    2.1% |      0.06 | `EvictionProtection3Networks050Candidates`
|           13,827.88 |           72,317.64 |    3.9% |      0.16 | `EvictionProtection3Networks100Candidates`
|           34,605.17 |           28,897.42 |    1.5% |      0.39 | `EvictionProtection3Networks250Candidates`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           19,255.11 |           51,934.26 |    2.9% |      0.21 | `EvictionProtection0Networks250Candidates`
|           26,797.82 |           37,316.47 |    3.4% |      0.30 | `EvictionProtection1Networks250Candidates`
|           31,007.27 |           32,250.50 |    1.8% |      0.35 | `EvictionProtection2Networks250Candidates`
|            5,075.53 |          197,023.62 |    3.3% |      0.06 | `EvictionProtection3Networks050Candidates`
|           13,964.15 |           71,611.94 |    3.5% |      0.16 | `EvictionProtection3Networks100Candidates`
|           34,294.63 |           29,159.08 |    1.6% |      0.39 | `EvictionProtection3Networks250Candidates`

It looks like the performance gains of 2x-5x will be even higher when additional disadvantaged networks are added.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

in ProtectEvictionCandidatesByRatio(). Thank you to Vasil Dimov, whose suggestions during a post-merge discussion about PR 21261 reminded me that I had done this in earlier versions of the PR, e.g. commits like ef411cd. Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

for the usual case when some of the protected networks don't have eviction candidates, to reduce the number of iterations in ProtectEvictionCandidatesByRatio(). Picks up an idea in ef411cd that I had dropped.

in ProtectEvictionCandidatesByRatio(). With this change, `if (n.count == 0) continue;` will be true if a network had candidates protected in the first iterations and has no candidates remaining to be protected in later iterations. Co-authored-by: Jon Atack <jon@atack.com>

klementtan · 2021-07-08T13:49:29Z

Tested and code review ACK b1d905c.

Also no longer seeing ... Unstable with ~100.0 iters. Increase minEpochIterations to e.g. 1000) warning.

Bench results

5adb064:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           18,612.89 |           53,726.22 |    1.3% |      0.21 | `EvictionProtection0Networks250Candidates`
|           15,604.12 |           64,085.64 |    1.1% |      0.17 | `EvictionProtection1Networks250Candidates`
|           46,098.96 |           21,692.46 |    1.4% |      0.51 | `EvictionProtection2Networks250Candidates`
|            7,573.28 |          132,043.17 |    2.2% |      0.09 | `EvictionProtection3Networks050Candidates`
|           16,631.50 |           60,126.87 |    3.2% |      0.18 | `EvictionProtection3Networks100Candidates`
|           97,775.81 |           10,227.48 |    3.3% |      1.09 | `EvictionProtection3Networks250Candidates`

02e411e


|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           18,389.64 |           54,378.44 |    1.7% |      0.20 | `EvictionProtection0Networks250Candidates`
|           13,869.52 |           72,100.52 |    1.3% |      0.15 | `EvictionProtection1Networks250Candidates`
|           46,016.06 |           21,731.55 |    3.4% |      0.50 | `EvictionProtection2Networks250Candidates`
|            6,408.73 |          156,037.13 |    3.6% |      0.07 | `EvictionProtection3Networks050Candidates`
|           16,160.17 |           61,880.55 |    2.1% |      0.18 | `EvictionProtection3Networks100Candidates`
|           96,984.48 |           10,310.93 |    4.5% |      1.07 | `EvictionProtection3Networks250Candidates`

c9e8d8f


|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           13,422.68 |           74,500.80 |    1.3% |      0.15 | `EvictionProtection0Networks250Candidates`
|           10,170.63 |           98,322.35 |    1.5% |      0.11 | `EvictionProtection1Networks250Candidates`
|           17,204.80 |           58,123.30 |    1.0% |      0.19 | `EvictionProtection2Networks250Candidates`
|            7,508.82 |          133,176.80 |    2.6% |      0.08 | `EvictionProtection3Networks050Candidates`
|           16,172.34 |           61,833.97 |    1.1% |      0.17 | `EvictionProtection3Networks100Candidates`
|           95,083.59 |           10,517.06 |    4.0% |      1.05 | `EvictionProtection3Networks250Candidates`

b1d905c

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           23,721.76 |           42,155.39 |    1.2% |      0.27 | `EvictionProtection0Networks250Candidates`
|           17,557.76 |           56,954.87 |    0.2% |      0.20 | `EvictionProtection1Networks250Candidates`
|           29,148.45 |           34,307.14 |    0.4% |      0.33 | `EvictionProtection2Networks250Candidates`
|            6,159.56 |          162,349.31 |    0.1% |      0.07 | `EvictionProtection3Networks050Candidates`
|           19,989.47 |           50,026.33 |    0.6% |      0.22 | `EvictionProtection3Networks100Candidates`
|           42,009.48 |           23,804.15 |    0.8% |      0.47 | `EvictionProtection3Networks250Candidates`

vasild

ACK b1d905c

base (5adb064 bench: add peer eviction protection benchmarks)

ns/op	op/s	err%	total	benchmark
15,636.87	63,951.43	0.6%	0.17	`EvictionProtection0Networks250Candidates`
12,885.39	77,607.27	0.5%	0.14	`EvictionProtection1Networks250Candidates`
35,732.17	27,985.99	0.3%	0.39	`EvictionProtection2Networks250Candidates`
5,514.20	181,349.88	1.9%	0.06	`EvictionProtection3Networks050Candidates`
12,243.04	81,679.08	0.2%	0.13	`EvictionProtection3Networks100Candidates`
76,015.27	13,155.25	0.4%	0.85	`EvictionProtection3Networks250Candidates`

all improvements (b1d905c p2p: earlier continuation when no remaining eviction candidates)

ns/op	op/s	err%	total	benchmark
10,587.04	94,455.10	0.7%	0.12	`EvictionProtection0Networks250Candidates`
8,046.60	124,276.05	0.8%	0.09	`EvictionProtection1Networks250Candidates`
12,479.92	80,128.74	1.0%	0.14	`EvictionProtection2Networks250Candidates`
2,784.53	359,127.39	4.3%	0.03	`EvictionProtection3Networks050Candidates`
8,286.96	120,671.54	0.4%	0.09	`EvictionProtection3Networks100Candidates`
18,450.28	54,199.71	0.7%	0.20	`EvictionProtection3Networks250Candidates`

Benchmarks complete in ~2 seconds.

jarolrod

ACK b1d905c

The way this has progressed since my last review makes sense. This is still a performance and improvement and code looks good.

I do still run into instability:

Unstable with ~1,001.0 iters. Increase `minEpochIterations` to e.g. 10010

Not sure what to make of it because I'm running the macOS 12 Beta (not a super stable system overall). Just wanted to document that I am running into that, I will have time later today to try on other systems. If no one else is running into it, let's assume it's my system.

The warning is flaky though, here's the results of my runs where I didn't run into the warning:

Base: Master + (566357f + 5adb064)

ns/op	op/s	err%	total	benchmark
18,439.40	54,231.71	4.0%	0.21	`EvictionProtection0Networks250Candidates`
16,029.50	62,384.99	3.1%	0.18	`EvictionProtection1Networks250Candidates`
43,125.31	23,188.24	2.6%	0.47	`EvictionProtection2Networks250Candidates`
7,225.39	138,400.78	3.1%	0.08	`EvictionProtection3Networks050Candidates`
15,869.58	63,013.65	3.5%	0.18	`EvictionProtection3Networks100Candidates`
97,586.25	10,247.35	1.1%	1.06	`EvictionProtection3Networks250Candidates`

All Improvements: b1d905c

ns/op	op/s	err%	total	benchmark
12,764.72	78,340.94	2.1%	0.14	`EvictionProtection0Networks250Candidates`
9,367.04	106,757.28	3.0%	0.10	`EvictionProtection1Networks250Candidates`
14,846.86	67,354.33	1.4%	0.16	`EvictionProtection2Networks250Candidates`
3,493.85	286,217.28	4.8%	0.04	`EvictionProtection3Networks050Candidates`
10,208.73	97,955.35	2.0%	0.11	`EvictionProtection3Networks100Candidates`
22,498.62	44,447.18	0.9%	0.25	`EvictionProtection3Networks250Candidates`

jonatack · 2021-07-09T15:56:37Z

Not sure what to make of it because I'm running the macOS 12 Beta (not a super stable system overall).

Yes, I tuned my system and yet still see that most of the time on at least half of the benchmarks when running all of them with ./src/bench/bench_bitcoin. It came down to the preference of being able to run these new benchmarks 10 times in ~20 seconds rather than once in 30-60 seconds, and having a rapid result that works for the most people. There just seems to be decreasing utility in adding iterations.

…ctEvict…

…yRatio() Summary: > This follow-up to #21261 improves ProtectEvictionCandidatesByRatio() for better performance. > > Benchmarks are added; the performance improvement is between 2x and 5x for the benchmarked cases. > > The refactored code is well-covered by existing unit tests. > p2p: iterate eviction protection only on networks having candidates in ProtectEvictionCandidatesByRatio(). > p2p: process more candidates per protection iteration for the usual case when some of the protected networks don't have eviction candidates, to reduce the number of iterations in ProtectEvictionCandidatesByRatio() > p2p: earlier continuation when no remaining eviction candidates in ProtectEvictionCandidatesByRatio(). > > With this change, `if (n.count == 0) continue;` will be true > if a network had candidates protected in the first iterations > and has no candidates remaining to be protected in later iterations. This is a backport of [[bitcoin/bitcoin#22284 | core#22284]] Depends on D11051 Test Plan: `ninja all check-all bitcoin-bench && src/bench/bitcoin-bench` Before: ``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 308,564.89 | 3,240.81 | 0.1% | 0.00 | `EvictionProtection0Networks250Candidates` | 329,332.71 | 3,036.44 | 0.0% | 0.00 | `EvictionProtection1Networks250Candidates` | 361,197.71 | 2,768.57 | 0.1% | 0.00 | `EvictionProtection2Networks250Candidates` | 49,956.19 | 20,017.54 | 0.0% | 0.00 | `EvictionProtection3Networks050Candidates` | 109,924.79 | 9,097.13 | 0.1% | 0.00 | `EvictionProtection3Networks100Candidates` | 681,640.58 | 1,467.05 | 0.1% | 0.01 | `EvictionProtection3Networks250Candidates` ``` After: ``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 81,663.94 | 12,245.31 | 0.1% | 0.00 | `EvictionProtection0Networks250Candidates` | 110,304.82 | 9,065.79 | 0.0% | 0.00 | `EvictionProtection1Networks250Candidates` | 147,154.10 | 6,795.60 | 0.1% | 0.00 | `EvictionProtection2Networks250Candidates` | 26,983.78 | 37,059.30 | 0.1% | 0.00 | `EvictionProtection3Networks050Candidates` | 69,721.16 | 14,342.85 | 0.0% | 0.00 | `EvictionProtection3Networks100Candidates` | 157,214.90 | 6,360.72 | 0.1% | 0.00 | `EvictionProtection3Networks250Candidates` ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11061

DrahtBot added the P2P label Jun 19, 2021

jonatack force-pushed the ProtectEvictionCandidatesByRatio-perf-enhancements branch 3 times, most recently from acc3f69 to ab29933 Compare June 20, 2021 14:33

jonatack mentioned this pull request Jun 20, 2021

bench: Add OrphanTxPool benchmark #19377

Closed

jarolrod reviewed Jun 21, 2021

View reviewed changes

This was referenced Jun 23, 2021

Add an option for evenly connecting to nodes on different types of networks #11537

Closed

bench, doc: benchmarking updates and fixups #22292

Merged

vasild reviewed Jun 30, 2021

View reviewed changes

src/bench/peer_eviction.cpp Outdated Show resolved Hide resolved

src/bench/peer_eviction.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

refactor: move GetRandomNodeEvictionCandidates() to test utilities

566357f

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

jonatack force-pushed the ProtectEvictionCandidatesByRatio-perf-enhancements branch from ab29933 to 524aa5d Compare July 6, 2021 16:41

jonatack force-pushed the ProtectEvictionCandidatesByRatio-perf-enhancements branch from 524aa5d to 4bef482 Compare July 8, 2021 09:42

jonatack and others added 4 commits July 8, 2021 12:28

bench: add peer eviction protection benchmarks

5adb064

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

p2p: process more candidates per protection iteration

c9e8d8f

for the usual case when some of the protected networks don't have eviction candidates, to reduce the number of iterations in ProtectEvictionCandidatesByRatio(). Picks up an idea in ef411cd that I had dropped.

jonatack force-pushed the ProtectEvictionCandidatesByRatio-perf-enhancements branch from 4bef482 to b1d905c Compare July 8, 2021 10:29

vasild approved these changes Jul 8, 2021

View reviewed changes

jarolrod reviewed Jul 9, 2021

View reviewed changes

laanwj merged commit 21998bc into bitcoin:master Jul 15, 2021

jonatack deleted the ProtectEvictionCandidatesByRatio-perf-enhancements branch July 15, 2021 13:11

hebasto mentioned this pull request Jul 15, 2021

-Wnarrowing warning on 32-bit system #22459

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

Merge bitcoin#22284: p2p, refactor: performance improvements to Prote…

4745a73

…ctEvict…

jonatack mentioned this pull request Sep 17, 2021

bench: add -quiet and -iters=<n> benchmark config args #22999

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 21998bc into merged_master (Bitcoin PR bitcoin/bitcoin#22284)

84b41b3

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio() #22284

p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio() #22284

Uh oh!

jonatack commented Jun 19, 2021 •

edited

Loading

Uh oh!

jonatack commented Jun 20, 2021 •

edited

Loading

Uh oh!

klementtan commented Jun 20, 2021

Uh oh!

DrahtBot commented Jun 20, 2021 •

edited

Loading

Uh oh!

jarolrod left a comment •

edited

Loading

Uh oh!

theStack commented Jun 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 6, 2021

Uh oh!

klementtan commented Jul 7, 2021 •

edited

Loading

Uh oh!

jonatack commented Jul 7, 2021 •

edited

Loading

Uh oh!

jonatack commented Jul 8, 2021 •

edited

Loading

Uh oh!

klementtan commented Jul 8, 2021

Uh oh!

vasild left a comment

Uh oh!

jarolrod left a comment •

edited

Loading

Uh oh!

jonatack commented Jul 9, 2021

Uh oh!

Uh oh!

p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio() #22284

p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio() #22284

Uh oh!

Conversation

jonatack commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jun 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klementtan commented Jun 20, 2021

Uh oh!

DrahtBot commented Jun 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jarolrod left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Notes on commits

Uh oh!

theStack commented Jun 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 6, 2021

Uh oh!

klementtan commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klementtan commented Jul 8, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jul 9, 2021

Uh oh!

Uh oh!

jonatack commented Jun 19, 2021 •

edited

Loading

jonatack commented Jun 20, 2021 •

edited

Loading

DrahtBot commented Jun 20, 2021 •

edited

Loading

jarolrod left a comment •

edited

Loading

klementtan commented Jul 7, 2021 •

edited

Loading

jonatack commented Jul 7, 2021 •

edited

Loading

jonatack commented Jul 8, 2021 •

edited

Loading

jarolrod left a comment •

edited

Loading