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 panic in neqo_crypto::init() #1775

Merged
merged 15 commits into from
Mar 28, 2024
Merged

Conversation

larseggert
Copy link
Collaborator

Return Res<()> instead.

Fixes #1675

Copy link

github-actions bot commented Mar 26, 2024

Benchmark results

Performance differences relative to 36fae62.

  • drain a timer quickly time: [390.52 ns 397.20 ns 403.45 ns]
    change: [-1.7286% -0.1217% +1.5068%] (p = 0.88 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [198.19 ns 198.71 ns 199.28 ns]
    change: [-0.8197% -0.3595% +0.0399%] (p = 0.10 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 3+1 entries
    time: [235.95 ns 236.53 ns 237.16 ns]
    change: [-1.2591% -0.1382% +0.6790%] (p = 0.82 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 10+1 entries
    time: [235.05 ns 235.49 ns 236.10 ns]
    change: [-0.3319% +0.0149% +0.3248%] (p = 0.93 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1000+1 entries
    time: [215.38 ns 215.59 ns 215.83 ns]
    change: [-1.5675% -0.7550% +0.1067%] (p = 0.07 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [119.38 ms 119.45 ms 119.53 ms]
    change: [+0.1589% +0.2453% +0.3365%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [118.82 ms 119.09 ms 119.35 ms]
    thrpt: [33.516 MiB/s 33.589 MiB/s 33.663 MiB/s]
    change:
    time: [-2.0934% -1.7636% -1.4360%] (p = 0.00 < 0.05)
    thrpt: [+1.4569% +1.7952% +2.1381%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [119.34 ms 119.52 ms 119.70 ms]
    thrpt: [33.417 MiB/s 33.467 MiB/s 33.518 MiB/s]
    change:
    time: [-1.7536% -1.5539% -1.3513%] (p = 0.00 < 0.05)
    thrpt: [+1.3698% +1.5784% +1.7849%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1104 s 1.1175 s 1.1257 s]
    thrpt: [88.833 MiB/s 89.487 MiB/s 90.060 MiB/s]
    change:
    time: [-0.9645% +0.6941% +2.2627%] (p = 0.43 > 0.05)
    thrpt: [-2.2127% -0.6893% +0.9739%]
    No change in performance detected.

  • 1-conn/10_000-1b-seq-resp (aka. RPS)/client
    time: [424.76 ms 426.66 ms 428.58 ms]
    thrpt: [23.333 Kelem/s 23.438 Kelem/s 23.543 Kelem/s]
    change:
    time: [-0.9410% -0.2392% +0.4674%] (p = 0.50 > 0.05)
    thrpt: [-0.4652% +0.2397% +0.9499%]
    No change in performance detected.

  • 100-seq-conn/1-1b-resp (aka. HPS)/client
    time: [3.3665 s 3.3697 s 3.3731 s]
    thrpt: [29.646 elem/s 29.676 elem/s 29.705 elem/s]
    change:
    time: [-0.5669% -0.4296% -0.2920%] (p = 0.00 < 0.05)
    thrpt: [+0.2928% +0.4314% +0.5701%]
    Change within noise threshold.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 766.4 ± 203.8 417.7 1000.9 1.00
neqo msquic reno on 2200.0 ± 235.2 1892.9 2657.7 1.00
neqo msquic reno 2048.4 ± 202.1 1872.8 2503.6 1.00
neqo msquic cubic on 2066.4 ± 123.6 1913.1 2273.4 1.00
neqo msquic cubic 1995.1 ± 227.0 1866.8 2635.6 1.00
msquic neqo reno on 4422.6 ± 166.3 4136.2 4618.0 1.00
msquic neqo reno 4527.0 ± 266.9 4194.4 5090.5 1.00
msquic neqo cubic on 4666.3 ± 399.7 4247.4 5561.6 1.00
msquic neqo cubic 4404.3 ± 156.0 4151.2 4591.2 1.00
neqo neqo reno on 3680.0 ± 192.4 3378.2 4017.7 1.00
neqo neqo reno 3815.9 ± 338.2 3276.2 4393.3 1.00
neqo neqo cubic on 4460.9 ± 184.3 4169.8 4720.7 1.00
neqo neqo cubic 4366.3 ± 220.3 3989.0 4721.1 1.00

⬇️ Download logs

larseggert and others added 4 commits March 27, 2024 08:48
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert larseggert enabled auto-merge March 27, 2024 07:40
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

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

Project coverage is 93.07%. Comparing base (76630a5) to head (296b72c).
Report is 3 commits behind head on main.

Files Patch % Lines
neqo-crypto/src/lib.rs 81.48% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1775      +/-   ##
==========================================
- Coverage   93.08%   93.07%   -0.01%     
==========================================
  Files         117      117              
  Lines       36422    36420       -2     
==========================================
- Hits        33903    33898       -5     
- Misses       2519     2522       +3     

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

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.

Suggestions on naming is all.

@larseggert larseggert added this pull request to the merge queue Mar 28, 2024
Merged via the queue into mozilla:main with commit 3151adc Mar 28, 2024
13 checks passed
@larseggert larseggert deleted the fix-1675 branch March 28, 2024 07:04
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.

Crash in [@ core::result::unwrap_failed | neqo_crypto::init::closure$0]
3 participants