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

feat: Make reason_phrase a String #1862

Merged
merged 5 commits into from
May 2, 2024
Merged

feat: Make reason_phrase a String #1862

merged 5 commits into from
May 2, 2024

Conversation

larseggert
Copy link
Collaborator

To make the debug logs a bit easier to parse.

@larseggert
feat: Make reason_phrase a String
a63dfac 
To make the debug logs a bit easier to parse.
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2024 •
edited
Loading

Failed Interop Tests

QUIC Interop Runner, client vs. server

Succeeded and unsupported tests

Succeeded Interop Tests

Unsupported Interop Tests

@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (d4b3e4c) to head (d664806).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1862      +/-   ##
==========================================
- Coverage   93.25%   93.24%   -0.01%     
==========================================
  Files         110      110              
  Lines       35793    35792       -1     
==========================================
- Hits        33377    33376       -1     
  Misses       2416     2416

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2024 •
edited
Loading

Benchmark results

Performance differences relative to dcc88e3.

  • drain a timer quickly time: [309.91 ns 318.24 ns 325.76 ns]
    change: [-2.5746% -0.4328% +1.8839%] (p = 0.71 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [197.32 ns 197.77 ns 198.24 ns]
    change: [+1.4161% +1.8342% +2.4073%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 3+1 entries
    time: [238.48 ns 239.14 ns 239.81 ns]
    change: [+0.1616% +0.5448% +0.9166%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 10+1 entries
    time: [237.57 ns 238.36 ns 239.26 ns]
    change: [+0.0815% +0.5091% +1.1015%] (p = 0.03 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 1000+1 entries
    time: [220.64 ns 220.87 ns 221.12 ns]
    change: [+0.4393% +1.0066% +1.5752%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • RxStreamOrderer::inbound_frame()
    time: [118.22 ms 118.29 ms 118.35 ms]
    change: [-0.0647% +0.0231% +0.1117%] (p = 0.62 > 0.05)
    No change in performance detected.

  • transfer/Run multiple transfers with varying seeds
    time: [117.96 ms 118.24 ms 118.52 ms]
    thrpt: [33.749 MiB/s 33.828 MiB/s 33.911 MiB/s]
    change:
    time: [-0.4182% -0.0801% +0.2453%] (p = 0.63 > 0.05)
    thrpt: [-0.2447% +0.0802% +0.4199%]
    No change in performance detected.

  • transfer/Run multiple transfers with the same seed
    time: [118.17 ms 118.39 ms 118.59 ms]
    thrpt: [33.729 MiB/s 33.788 MiB/s 33.849 MiB/s]
    change:
    time: [-0.1870% +0.0703% +0.3291%] (p = 0.60 > 0.05)
    thrpt: [-0.3280% -0.0702% +0.1873%]
    No change in performance detected.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1112 s 1.1142 s 1.1172 s]
    thrpt: [89.508 MiB/s 89.751 MiB/s 89.995 MiB/s]
    change:
    time: [+0.5148% +1.3794% +2.1874%] (p = 0.01 < 0.05)
    thrpt: [-2.1406% -1.3606% -0.5122%]
    Change within noise threshold.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [425.83 ms 427.64 ms 429.42 ms]
    thrpt: [23.287 Kelem/s 23.384 Kelem/s 23.484 Kelem/s]
    change:
    time: [-0.7300% -0.0567% +0.6287%] (p = 0.87 > 0.05)
    thrpt: [-0.6248% +0.0568% +0.7354%]
    No change in performance detected.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [47.381 ms 47.821 ms 48.240 ms]
    thrpt: [20.730 elem/s 20.911 elem/s 21.105 elem/s]
    change:
    time: [-4.2194% -3.1388% -2.1022%] (p = 0.00 < 0.05)
    thrpt: [+2.1473% +3.2405% +4.4053%]
    💚 Performance has improved.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 710.3 ± 218.6 410.4 996.8 1.00
neqo msquic reno on 881.3 ± 130.6 777.3 1216.3 1.00
neqo msquic reno 880.1 ± 189.2 752.3 1359.9 1.00
neqo msquic cubic on 882.3 ± 168.1 744.3 1186.3 1.00
neqo msquic cubic 840.6 ± 153.8 745.5 1269.2 1.00
msquic neqo reno on 4300.0 ± 192.5 3943.3 4540.9 1.00
msquic neqo reno 4358.2 ± 240.6 4081.4 4737.1 1.00
msquic neqo cubic on 4423.5 ± 297.3 4119.8 5188.1 1.00
msquic neqo cubic 4324.2 ± 156.0 4075.0 4581.7 1.00
neqo neqo reno on 3466.0 ± 265.9 3099.0 3852.1 1.00
neqo neqo reno 3557.3 ± 292.9 3177.4 4224.9 1.00
neqo neqo cubic on 4215.0 ± 423.7 3106.4 4614.8 1.00
neqo neqo cubic 4068.3 ± 508.5 2803.5 4842.0 1.00

⬇️ Download logs

@larseggert
Copy link
Collaborator Author

Waiting to merge to see if @martinthomson has a preference here.

mxinden
mxinden reviewed Apr 30, 2024
View reviewed changes
martinthomson
martinthomson approved these changes May 1, 2024
View reviewed changes
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.

WFM. I think we erred on the side of making the reading path non-lossy, but I don't see much point in that. We'd need a better way to access the information to make good on that anyway.

@larseggert larseggert enabled auto-merge May 1, 2024 08:11
larseggert and others added 3 commits May 2, 2024 08:24
@larseggert @martinthomson
Update neqo-transport/src/frame.rs
e183594 
Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert @mxinden
Update neqo-transport/src/qlog.rs
44c0447 
Co-authored-by: Max Inden <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>
@larseggert
Address code review
f1dddab
martinthomson
martinthomson approved these changes May 2, 2024
View reviewed changes
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.

The change to the From implementation looks fine.

@larseggert larseggert added this pull request to the merge queue May 2, 2024
Merged via the queue into mozilla:main with commit 620244d May 2, 2024
47 checks passed
@larseggert larseggert deleted the feat-reason-phrease-string branch May 2, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden left review comments

@martinthomson martinthomson martinthomson approved these changes

@KershawChang KershawChang KershawChang approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@larseggert @martinthomson @KershawChang @mxinden