Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't retire zero-length connection IDs #2182

Closed

Conversation

larseggert
Copy link
Collaborator

@larseggert larseggert commented Oct 15, 2024

This makes us pass the rebind-addr QNS test.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.38%. Comparing base (9b5ec71) to head (b236d08).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
neqo-transport/src/path.rs 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
- Coverage   95.39%   95.38%   -0.01%     
==========================================
  Files         112      112              
  Lines       36373    36373              
==========================================
- Hits        34697    34694       -3     
- Misses       1676     1679       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Failed Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

Copy link

Benchmark results

Performance differences relative to 9b5ec71.

coalesce_acked_from_zero 1+1 entries: No change in performance detected.
       time:   [99.038 ns 99.334 ns 99.640 ns]
       change: [-0.7238% -0.2269% +0.3052%] (p = 0.42 > 0.05)

Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe

coalesce_acked_from_zero 3+1 entries: Change within noise threshold.
       time:   [116.96 ns 117.37 ns 117.80 ns]
       change: [-1.3189% -0.7962% -0.2748%] (p = 0.00 < 0.05)

Found 17 outliers among 100 measurements (17.00%)
2 (2.00%) low mild
2 (2.00%) high mild
13 (13.00%) high severe

coalesce_acked_from_zero 10+1 entries: Change within noise threshold.
       time:   [116.35 ns 116.66 ns 117.07 ns]
       change: [-0.9016% -0.4888% -0.1127%] (p = 0.01 < 0.05)

Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) low mild
5 (5.00%) high severe

coalesce_acked_from_zero 1000+1 entries: No change in performance detected.
       time:   [97.618 ns 97.775 ns 97.954 ns]
       change: [-1.7296% -0.6509% +0.5515%] (p = 0.27 > 0.05)

Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severe

RxStreamOrderer::inbound_frame(): Change within noise threshold.
       time:   [111.15 ms 111.21 ms 111.26 ms]
       change: [-0.6945% -0.6207% -0.5469%] (p = 0.00 < 0.05)

Found 18 outliers among 100 measurements (18.00%)
1 (1.00%) low severe
6 (6.00%) low mild
6 (6.00%) high mild
5 (5.00%) high severe

transfer/pacing-false/varying-seeds: No change in performance detected.
       time:   [26.625 ms 27.729 ms 28.857 ms]
       change: [-3.9929% +1.4545% +7.2250%] (p = 0.60 > 0.05)

Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

transfer/pacing-true/varying-seeds: No change in performance detected.
       time:   [34.667 ms 36.386 ms 38.156 ms]
       change: [-8.2041% -2.2232% +4.4709%] (p = 0.50 > 0.05)

Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

transfer/pacing-false/same-seed: No change in performance detected.
       time:   [24.955 ms 25.775 ms 26.612 ms]
       change: [-4.1507% +0.0970% +4.4603%] (p = 0.97 > 0.05)

Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild

transfer/pacing-true/same-seed: No change in performance detected.
       time:   [39.632 ms 41.619 ms 43.684 ms]
       change: [-7.3968% -1.4326% +5.1747%] (p = 0.66 > 0.05)
1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected.
       time:   [116.26 ms 116.70 ms 117.20 ms]
       thrpt:  [853.25 MiB/s 856.92 MiB/s 860.15 MiB/s]
change:
       time:   [-15.019% -4.0547% +2.8436%] (p = 0.68 > 0.05)
       thrpt:  [-2.7650% +4.2260% +17.673%]

Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) low mild
1 (1.00%) high mild
1 (1.00%) high severe

1-conn/10_000-parallel-1b-resp (aka. RPS)/client: 💔 Performance has regressed.
       time:   [318.92 ms 322.94 ms 327.02 ms]
       thrpt:  [30.579 Kelem/s 30.965 Kelem/s 31.356 Kelem/s]
change:
       time:   [+1.7463% +3.3982% +5.1187%] (p = 0.00 < 0.05)
       thrpt:  [-4.8694% -3.2866% -1.7163%]
1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected.
       time:   [33.701 ms 33.867 ms 34.047 ms]
       thrpt:  [29.371  elem/s 29.528  elem/s 29.673  elem/s]
change:
       time:   [-1.4490% -0.5513% +0.3081%] (p = 0.22 > 0.05)
       thrpt:  [-0.3072% +0.5543% +1.4703%]

Found 12 outliers among 100 measurements (12.00%)
5 (5.00%) low mild
5 (5.00%) high mild
2 (2.00%) high severe

Client/server transfer results

Transfer of 33554432 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 157.5 ± 66.0 91.6 331.6 1.00
neqo msquic reno on 230.9 ± 12.7 213.0 252.3 1.00
neqo msquic reno 275.8 ± 89.0 217.3 468.4 1.00
neqo msquic cubic on 233.3 ± 26.2 209.4 299.4 1.00
neqo msquic cubic 267.1 ± 76.9 217.1 459.8 1.00
msquic neqo reno on 98.5 ± 17.9 81.4 139.3 1.00
msquic neqo reno 147.7 ± 91.1 82.1 350.8 1.00
msquic neqo cubic on 104.5 ± 32.1 81.2 216.4 1.00
msquic neqo cubic 93.9 ± 17.8 81.6 156.7 1.00
neqo neqo reno on 149.8 ± 34.0 121.5 270.3 1.00
neqo neqo reno 179.0 ± 61.9 129.3 348.1 1.00
neqo neqo cubic on 262.4 ± 126.1 119.9 419.4 1.00
neqo neqo cubic 167.3 ± 70.2 126.5 411.1 1.00

⬇️ Download logs

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wheee!

Test case?

@larseggert
Copy link
Collaborator Author

Working on test cases in #2170. Should I merge all the feature fixes there?

@martinthomson
Copy link
Member

Moving the code seems fine to me.

@larseggert
Copy link
Collaborator Author

Now part of #2170.

@larseggert larseggert closed this Oct 21, 2024
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.

2 participants