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

chore: Make the TLS epoch an enum type #2320

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

larseggert
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

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

Project coverage is 93.32%. Comparing base (5c36c79) to head (ea1ef88).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
neqo-crypto/src/agentio.rs 57.14% 3 Missing ⚠️
neqo-crypto/src/secrets.rs 66.66% 2 Missing ⚠️
neqo-crypto/src/constants.rs 93.33% 1 Missing ⚠️
neqo-transport/src/crypto.rs 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2320      +/-   ##
==========================================
- Coverage   93.33%   93.32%   -0.02%     
==========================================
  Files         114      115       +1     
  Lines       36897    36915      +18     
  Branches    36897    36915      +18     
==========================================
+ Hits        34439    34450      +11     
- Misses       1675     1688      +13     
+ Partials      783      777       -6     

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

Copy link

github-actions bot commented Jan 8, 2025

Failed Interop Tests

QUIC Interop Runner, client vs. server, differences relative to 648863b.

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

github-actions bot commented Jan 8, 2025

Benchmark results

Performance differences relative to 6013bde.

decode 4096 bytes, mask ff: Change within noise threshold.
       time:   [11.202 µs 11.254 µs 11.319 µs]
       change: [+0.1771% +0.6204% +1.2155%] (p = 0.01 < 0.05)

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

decode 1048576 bytes, mask ff: 💔 Performance has regressed.
       time:   [3.1026 ms 3.1132 ms 3.1251 ms]
       change: [+2.2519% +2.7956% +3.3124%] (p = 0.00 < 0.05)

Found 12 outliers among 100 measurements (12.00%)
12 (12.00%) high severe

decode 4096 bytes, mask 7f: No change in performance detected.
       time:   [19.559 µs 19.619 µs 19.686 µs]
       change: [-0.7844% -0.0895% +0.4529%] (p = 0.80 > 0.05)

Found 18 outliers among 100 measurements (18.00%)
1 (1.00%) low severe
2 (2.00%) low mild
1 (1.00%) high mild
14 (14.00%) high severe

decode 1048576 bytes, mask 7f: No change in performance detected.
       time:   [5.1693 ms 5.1821 ms 5.1955 ms]
       change: [-0.2432% +0.1150% +0.5042%] (p = 0.54 > 0.05)

Found 16 outliers among 100 measurements (16.00%)
16 (16.00%) high severe

decode 4096 bytes, mask 3f: No change in performance detected.
       time:   [5.5324 µs 5.5562 µs 5.5868 µs]
       change: [-0.9016% -0.1957% +0.4285%] (p = 0.60 > 0.05)

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

decode 1048576 bytes, mask 3f: No change in performance detected.
       time:   [1.7582 ms 1.7597 ms 1.7626 ms]
       change: [-0.7047% -0.3146% +0.0080%] (p = 0.11 > 0.05)

Found 8 outliers among 100 measurements (8.00%)
5 (5.00%) high mild
3 (3.00%) high severe

coalesce_acked_from_zero 1+1 entries: Change within noise threshold.
       time:   [99.444 ns 99.729 ns 100.02 ns]
       change: [+0.3834% +0.8567% +1.3478%] (p = 0.00 < 0.05)

Found 13 outliers among 100 measurements (13.00%)
10 (10.00%) high mild
3 (3.00%) high severe

coalesce_acked_from_zero 3+1 entries: Change within noise threshold.
       time:   [117.80 ns 118.42 ns 119.22 ns]
       change: [+0.2142% +1.3961% +2.7329%] (p = 0.02 < 0.05)

Found 21 outliers among 100 measurements (21.00%)
2 (2.00%) low severe
3 (3.00%) low mild
2 (2.00%) high mild
14 (14.00%) high severe

coalesce_acked_from_zero 10+1 entries: Change within noise threshold.
       time:   [117.53 ns 118.13 ns 118.79 ns]
       change: [+0.2853% +0.9298% +1.4689%] (p = 0.00 < 0.05)

Found 16 outliers among 100 measurements (16.00%)
6 (6.00%) low severe
10 (10.00%) high severe

coalesce_acked_from_zero 1000+1 entries: No change in performance detected.
       time:   [98.543 ns 98.670 ns 98.817 ns]
       change: [-0.0802% +1.0901% +2.2092%] (p = 0.05 > 0.05)

Found 13 outliers among 100 measurements (13.00%)
4 (4.00%) high mild
9 (9.00%) high severe

RxStreamOrderer::inbound_frame(): Change within noise threshold.
       time:   [111.28 ms 111.33 ms 111.38 ms]
       change: [+0.0071% +0.2046% +0.3299%] (p = 0.01 < 0.05)

Found 14 outliers among 100 measurements (14.00%)
3 (3.00%) low severe
2 (2.00%) low mild
3 (3.00%) high mild
6 (6.00%) high severe

SentPackets::take_ranges: No change in performance detected.
       time:   [5.4791 µs 5.6607 µs 5.8542 µs]
       change: [-1.4958% +1.1967% +4.2019%] (p = 0.42 > 0.05)

Found 8 outliers among 100 measurements (8.00%)
4 (4.00%) high mild
4 (4.00%) high severe

transfer/pacing-false/varying-seeds: Change within noise threshold.
       time:   [42.262 ms 42.354 ms 42.453 ms]
       change: [+1.1975% +1.5156% +1.7944%] (p = 0.00 < 0.05)

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

transfer/pacing-true/varying-seeds: Change within noise threshold.
       time:   [42.483 ms 42.559 ms 42.639 ms]
       change: [+1.0471% +1.2853% +1.5328%] (p = 0.00 < 0.05)

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

transfer/pacing-false/same-seed: Change within noise threshold.
       time:   [42.100 ms 42.167 ms 42.240 ms]
       change: [+0.5840% +0.8357% +1.0900%] (p = 0.00 < 0.05)

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

transfer/pacing-true/same-seed: No change in performance detected.
       time:   [42.078 ms 42.157 ms 42.246 ms]
       change: [-0.1459% +0.1310% +0.3959%] (p = 0.35 > 0.05)

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

1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected.
       time:   [872.06 ms 881.90 ms 892.17 ms]
       thrpt:  [112.09 MiB/s 113.39 MiB/s 114.67 MiB/s]
change:
       time:   [-2.0207% -0.5634% +0.9926%] (p = 0.49 > 0.05)
       thrpt:  [-0.9828% +0.5665% +2.0624%]

Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild

1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected.
       time:   [300.28 ms 302.60 ms 304.94 ms]
       thrpt:  [32.794 Kelem/s 33.047 Kelem/s 33.302 Kelem/s]
change:
       time:   [-0.9141% +0.0741% +1.1306%] (p = 0.89 > 0.05)
       thrpt:  [-1.1179% -0.0741% +0.9226%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected.
       time:   [34.096 ms 34.288 ms 34.503 ms]
       thrpt:  [28.983  elem/s 29.165  elem/s 29.329  elem/s]
change:
       time:   [-0.8458% -0.0511% +0.8054%] (p = 0.90 > 0.05)
       thrpt:  [-0.7990% +0.0511% +0.8530%]

Found 10 outliers among 100 measurements (10.00%)
4 (4.00%) low mild
6 (6.00%) high severe

1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: No change in performance detected.
       time:   [1.6201 s 1.6360 s 1.6523 s]
       thrpt:  [60.521 MiB/s 61.126 MiB/s 61.724 MiB/s]
change:
       time:   [-2.8855% -1.4057% +0.1546%] (p = 0.07 > 0.05)
       thrpt:  [-0.1544% +1.4258% +2.9712%]

Client/server transfer results

Transfer of 33554432 bytes over loopback.

Client Server CC Pacing MTU Mean [ms] Min [ms] Max [ms]
gquiche gquiche 1504 561.0 ± 89.4 503.1 735.3
neqo gquiche reno on 1504 809.3 ± 83.9 749.0 965.8
neqo gquiche reno 1504 788.9 ± 80.8 738.9 979.8
neqo gquiche cubic on 1504 800.1 ± 140.1 745.1 1198.4
neqo gquiche cubic 1504 770.1 ± 50.7 735.4 910.8
msquic msquic 1504 188.6 ± 101.4 108.4 357.4
neqo msquic reno on 1504 258.7 ± 79.3 203.8 458.5
neqo msquic reno 1504 299.2 ± 113.8 205.4 514.6
neqo msquic cubic on 1504 235.8 ± 47.2 209.1 391.1
neqo msquic cubic 1504 265.0 ± 78.3 205.7 408.5
gquiche neqo reno on 1504 727.3 ± 145.3 598.0 1009.3
gquiche neqo reno 1504 732.3 ± 186.0 548.4 1168.5
gquiche neqo cubic on 1504 691.5 ± 100.8 539.0 826.7
gquiche neqo cubic 1504 732.1 ± 118.2 535.3 954.8
msquic neqo reno on 1504 471.4 ± 9.1 458.2 480.2
msquic neqo reno 1504 502.7 ± 62.9 460.6 665.2
msquic neqo cubic on 1504 500.6 ± 48.4 454.6 592.7
msquic neqo cubic 1504 512.1 ± 67.2 458.8 643.6
neqo neqo reno on 1504 485.4 ± 46.7 442.6 598.1
neqo neqo reno 1504 519.1 ± 66.6 474.1 693.6
neqo neqo cubic on 1504 565.3 ± 73.5 478.2 754.0
neqo neqo cubic 1504 528.4 ± 47.6 460.6 612.5

⬇️ Download logs

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.

👍 the more we can enforce constraints through the type system the better.

neqo-crypto/src/constants.rs Outdated Show resolved Hide resolved
Comment on lines 51 to 53
fn put(&mut self, epoch: Epoch, key: SymKey) {
assert!(epoch > 0);
let i = (epoch - 1) as usize;
assert!(i < self.secrets.len());
// assert!(self.secrets[i].is_none());
self.secrets[i] = Some(key);
self.secrets[epoch] = Some(key);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Previously this function would panic on epoch Initial. Is it safe to change that behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we never stored initial secrets. Can restore the assert or maybe make it a debug_assert. But would like to hear from @martinthomson what the original intent was. Let's wait for him.

neqo-crypto/src/constants.rs Outdated Show resolved Hide resolved
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.

Looks good to me.

Let's give Martin a chance to comment on #2320 (comment)

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