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: Allow turning off greasing and connection migration #2100

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

larseggert
Copy link
Collaborator

There was some plumbing missing. CC @mxinden

Copy link

github-actions bot commented Sep 12, 2024

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
Collaborator

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

Though given that today is the first day I fully read RFC 9287, I think someone else should take a look as well. Maybe the author of RFC 9287? 😇

Do we have a test testing the client-server exchange of the grease transport parameter? I can't find one. If it doesn't exist, I am happy to add one. No need to block this pull request.

neqo-transport/src/connection/tests/mod.rs Outdated Show resolved Hide resolved
neqo-transport/src/connection/tests/mod.rs Outdated Show resolved Hide resolved
larseggert and others added 3 commits September 12, 2024 18:39
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: Lars Eggert <lars@eggert.org>
Copy link

github-actions bot commented Sep 12, 2024

Firefox builds for this PR

The following builds are available for testing. Crossed-out builds did not succeed.

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.34%. Comparing base (4aa0319) to head (12aa8ff).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2100      +/-   ##
==========================================
+ Coverage   95.33%   95.34%   +0.01%     
==========================================
  Files         112      112              
  Lines       36300    36309       +9     
==========================================
+ Hits        34607    34620      +13     
+ Misses       1693     1689       -4     

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

Copy link

github-actions bot commented Sep 12, 2024

Benchmark results

Performance differences relative to 0b106c2.

coalesce_acked_from_zero 1+1 entries: No change in performance detected.
       time:   [99.400 ns 99.716 ns 100.03 ns]
       change: [-0.3266% +0.2637% +0.7937%] (p = 0.38 > 0.05)

Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe

coalesce_acked_from_zero 3+1 entries: No change in performance detected.
       time:   [117.33 ns 117.69 ns 118.10 ns]
       change: [-0.1049% +0.3050% +0.6939%] (p = 0.13 > 0.05)

Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) low mild
1 (1.00%) high mild
6 (6.00%) high severe

coalesce_acked_from_zero 10+1 entries: Change within noise threshold.
       time:   [117.12 ns 117.60 ns 118.17 ns]
       change: [+0.0649% +0.6486% +1.2226%] (p = 0.03 < 0.05)

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

coalesce_acked_from_zero 1000+1 entries: No change in performance detected.
       time:   [97.338 ns 97.507 ns 97.699 ns]
       change: [-1.1985% -0.3415% +0.4624%] (p = 0.44 > 0.05)

Found 13 outliers among 100 measurements (13.00%)
6 (6.00%) high mild
7 (7.00%) high severe

RxStreamOrderer::inbound_frame(): Change within noise threshold.
       time:   [111.60 ms 111.75 ms 111.98 ms]
       change: [+0.1987% +0.3359% +0.5552%] (p = 0.00 < 0.05)

Found 25 outliers among 100 measurements (25.00%)
11 (11.00%) low severe
1 (1.00%) high mild
13 (13.00%) high severe

transfer/pacing-false/varying-seeds: No change in performance detected.
       time:   [26.119 ms 27.175 ms 28.218 ms]
       change: [-7.8299% -3.0786% +2.1893%] (p = 0.25 > 0.05)

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

transfer/pacing-true/varying-seeds: No change in performance detected.
       time:   [35.131 ms 36.667 ms 38.230 ms]
       change: [-4.6677% +1.3907% +7.4441%] (p = 0.66 > 0.05)

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

transfer/pacing-false/same-seed: No change in performance detected.
       time:   [31.825 ms 32.615 ms 33.387 ms]
       change: [-3.5836% -0.3916% +2.8532%] (p = 0.81 > 0.05)

Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) low mild

transfer/pacing-true/same-seed: No change in performance detected.
       time:   [41.513 ms 44.123 ms 46.690 ms]
       change: [-11.743% -2.9970% +6.4575%] (p = 0.52 > 0.05)
1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected.
       time:   [113.99 ms 114.95 ms 116.17 ms]
       thrpt:  [860.79 MiB/s 869.94 MiB/s 877.28 MiB/s]
change:
       time:   [-0.9942% -0.0895% +0.9690%] (p = 0.88 > 0.05)
       thrpt:  [-0.9597% +0.0896% +1.0042%]

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

1-conn/10_000-parallel-1b-resp (aka. RPS)/client: No change in performance detected.
       time:   [314.28 ms 318.10 ms 321.86 ms]
       thrpt:  [31.069 Kelem/s 31.437 Kelem/s 31.819 Kelem/s]
change:
       time:   [-1.8467% -0.1396% +1.6065%] (p = 0.87 > 0.05)
       thrpt:  [-1.5811% +0.1398% +1.8815%]
1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected.
       time:   [33.734 ms 33.908 ms 34.097 ms]
       thrpt:  [29.328  elem/s 29.491  elem/s 29.644  elem/s]
change:
       time:   [-1.5113% -0.6923% +0.1412%] (p = 0.11 > 0.05)
       thrpt:  [-0.1410% +0.6971% +1.5344%]

Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.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 161.6 ± 100.6 90.8 379.0 1.00
neqo msquic reno on 252.1 ± 66.0 208.8 432.1 1.00
neqo msquic reno 255.0 ± 69.3 214.6 419.4 1.00
neqo msquic cubic on 251.4 ± 63.6 215.1 425.8 1.00
neqo msquic cubic 254.5 ± 71.8 212.1 432.4 1.00
msquic neqo reno on 158.4 ± 88.9 93.2 333.8 1.00
msquic neqo reno 133.3 ± 81.9 81.3 334.1 1.00
msquic neqo cubic on 158.9 ± 81.2 91.1 325.9 1.00
msquic neqo cubic 181.2 ± 146.2 83.6 587.2 1.00
neqo neqo reno on 174.2 ± 78.2 128.8 388.2 1.00
neqo neqo reno 240.7 ± 124.2 136.5 525.4 1.00
neqo neqo cubic on 197.1 ± 84.9 122.2 406.8 1.00
neqo neqo cubic 176.2 ± 58.7 123.5 355.0 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.

This looks fine.

One thing I noticed is that we have a public method called "migrate" on the connection. This method can be called when the server has set disable_active_migration. That's not ideal. That's worth another change, I think.

@larseggert larseggert added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 13, 2024
@mxinden
Copy link
Collaborator

mxinden commented Sep 13, 2024

Ah, sorry, merged main too early. #2105 is still pending.

@larseggert larseggert added this pull request to the merge queue Sep 13, 2024
Merged via the queue into mozilla:main with commit c1e7dc3 Sep 13, 2024
54 of 56 checks passed
@larseggert larseggert deleted the fix-no-grease branch September 13, 2024 13:13
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