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.

Sign up for GitHub

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

Jump to bottom

refactor: rename ConnectionError to CloseReason #1872

Merged
merged 1 commit into from
May 3, 2024
Merged

refactor: rename ConnectionError to CloseReason #1872

merged 1 commit into from
May 3, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented May 3, 2024

The neqo_transport::ConnectionError enum contains the two non-error variants Error::NoError and CloseReason::Application(0). In other words, ConnectionError contains variants that are not errors.

This commit renames ConnectionError to the more descriptive name CloseReason.

See suggestion in #1866 (comment).

To ease the upgrade for downstream users, this commit adds a deprecated ConnectionError, guiding users to rename to CloseReason via a deprecation warning.

#[deprecated(note = "use `CloseReason` instead")]
pub type ConnectionError = CloseReason;

@mxinden
refactor: rename ConnectionError to CloseReason
c8c1d03 
The `neqo_transport::ConnectionError` enum contains the two non-error variants
`Error::NoError` and `CloseReason::Application(0)`. In other words,
`ConnectionError` contains variants that are not errors.

This commit renames `ConnectionError` to the more descriptive name
`CloseReason`.

See suggestion in mozilla#1866 (comment).

To ease the upgrade for downstream users, this commit adds a deprecated
`ConnectionError`, guiding users to rename to `CloseReason` via a deprecation warning.

``` rust
pub type ConnectionError = CloseReason;
```
larseggert
larseggert approved these changes May 3, 2024
View reviewed changes
Copy link
Collaborator

@larseggert larseggert left a comment

Choose a reason for hiding this comment

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

Two suggestions for your consideration (not sure if they make sense in the context of the rest of the code), otherwise LGTM.

neqo-bin/src/client/http3.rs Show resolved Hide resolved
neqo-transport/src/lib.rs Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024

Failed Interop Tests

QUIC Interop Runner, client vs. server

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

@larseggert larseggert enabled auto-merge May 3, 2024 07:58
@larseggert larseggert added this pull request to the merge queue May 3, 2024
@codecov Codecov
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 93.24%. Comparing base (87bf852) to head (c8c1d03).

Files Patch % Lines
neqo-transport/src/frame.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1872      +/-   ##
==========================================
- Coverage   93.24%   93.24%   -0.01%     
==========================================
  Files         110      110              
  Lines       35812    35810       -2     
==========================================
- Hits        33392    33390       -2     
  Misses       2420     2420

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

Merged via the queue into mozilla:main with commit ed19eb2 May 3, 2024
48 of 49 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented May 3, 2024

Benchmark results

Performance differences relative to 87bf852.

  • drain a timer quickly time: [312.61 ns 320.43 ns 327.39 ns]
    change: [-2.2234% +0.4053% +3.2871%] (p = 0.77 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [197.27 ns 197.71 ns 198.17 ns]
    change: [-1.1445% -0.8332% -0.5106%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 3+1 entries
    time: [240.90 ns 241.64 ns 242.44 ns]
    change: [-0.7822% -0.2410% +0.2606%] (p = 0.37 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 10+1 entries
    time: [239.54 ns 240.23 ns 241.08 ns]
    change: [-0.9290% -0.2863% +0.4752%] (p = 0.44 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1000+1 entries
    time: [217.86 ns 225.02 ns 241.10 ns]
    change: [-10.766% -3.0009% +4.0750%] (p = 0.60 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [118.79 ms 118.87 ms 118.96 ms]
    change: [-0.6583% -0.5705% -0.4761%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [117.92 ms 118.16 ms 118.40 ms]
    thrpt: [33.783 MiB/s 33.852 MiB/s 33.922 MiB/s]
    change:
    time: [-0.1149% +0.1904% +0.4969%] (p = 0.22 > 0.05)
    thrpt: [-0.4944% -0.1901% +0.1150%]
    No change in performance detected.

  • transfer/Run multiple transfers with the same seed
    time: [117.95 ms 118.19 ms 118.42 ms]
    thrpt: [33.778 MiB/s 33.845 MiB/s 33.912 MiB/s]
    change:
    time: [+0.3107% +0.6031% +0.8770%] (p = 0.00 < 0.05)
    thrpt: [-0.8694% -0.5994% -0.3097%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.0931 s 1.1030 s 1.1130 s]
    thrpt: [89.848 MiB/s 90.659 MiB/s 91.483 MiB/s]
    change:
    time: [-4.2167% -3.2892% -2.3304%] (p = 0.00 < 0.05)
    thrpt: [+2.3860% +3.4010% +4.4023%]
    💚 Performance has improved.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [424.85 ms 426.86 ms 428.87 ms]
    thrpt: [23.317 Kelem/s 23.427 Kelem/s 23.537 Kelem/s]
    change:
    time: [-1.7680% -1.0818% -0.3912%] (p = 0.00 < 0.05)
    thrpt: [+0.3927% +1.0937% +1.7998%]
    Change within noise threshold.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [49.664 ms 49.794 ms 49.967 ms]
    thrpt: [20.013 elem/s 20.083 elem/s 20.135 elem/s]
    change:
    time: [-0.3417% +0.0010% +0.3811%] (p = 0.99 > 0.05)
    thrpt: [-0.3796% -0.0010% +0.3429%]
    No change in performance detected.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 654.0 ± 308.4 399.1 1378.1 1.00
neqo msquic reno on 1017.0 ± 312.2 804.9 1751.9 1.00
neqo msquic reno 934.3 ± 266.6 681.1 1333.1 1.00
neqo msquic cubic on 1002.1 ± 228.3 824.1 1374.4 1.00
neqo msquic cubic 920.9 ± 235.9 751.2 1393.1 1.00
msquic neqo reno on 4577.7 ± 258.7 4191.3 5011.7 1.00
msquic neqo reno 4386.0 ± 232.3 4105.1 4762.5 1.00
msquic neqo cubic on 4459.5 ± 175.6 4178.2 4671.5 1.00
msquic neqo cubic 4574.2 ± 343.7 4220.1 5352.4 1.00
neqo neqo reno on 3782.5 ± 321.6 3403.3 4345.7 1.00
neqo neqo reno 3471.6 ± 248.3 3169.4 3905.6 1.00
neqo neqo cubic on 4184.7 ± 321.4 3462.1 4522.9 1.00
neqo neqo cubic 4253.3 ± 246.7 3704.3 4618.9 1.00

⬇️ Download logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larseggert larseggert larseggert approved these changes

@KershawChang KershawChang Awaiting requested review from KershawChang KershawChang is a code owner

@martinthomson martinthomson Awaiting requested review from martinthomson martinthomson is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mxinden @larseggert