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

chore(neqo-udp): drop tokio dependency #1988

Merged
merged 2 commits into from
Jul 17, 2024
Merged

chore(neqo-udp): drop tokio dependency #1988

merged 2 commits into from
Jul 17, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Jul 17, 2024

neqo-udp is used in Firefox. Firefox uses cargo vet to audit its dependencies. tokio, the dependency of neqo-udp, is not audited as safe-to-deploy. cargo vet will require safe-to-deploy for tokio even when behind a feature flag.

To work around this limitation in cargo vet, drop tokio dependency in neqo-udp, moving tokio specific logic into neqo-bin.

See details in:

@mxinden
chore(neqo-udp): drop tokio dependency
bccdcc7 
`neqo-udp` is used in Firefox. Firefox uses `cargo vet` to audit its
dependencies. `tokio`, the dependency of `neqo-udp`, is not audited as
`safe-to-deploy`. `cargo vet` will require `safe-to-deploy` for `tokio` even
when behind a feature flag.

To work around this limitation in `cargo vet`, drop `tokio` dependency in
`neqo-udp`, moving `tokio` specific logic into `neqo-bin`.

See details in:

- mozilla/cargo-vet#626
- https://bugzilla.mozilla.org/show_bug.cgi?id=1907810
- https://phabricator.services.mozilla.com/D212959#inline-1194604
@mxinden
Copy link
Collaborator Author

mxinden commented Jul 17, 2024

cargo vet output before:

➜  mozilla-unified git:(quic-rust-udp-io) ✗ ./mach cargo vet
[INFO] Using /home/mxinden/code/mozilla.org/mozilla-unified/.cargo/config.toml.
Vetting Failed!

4 unvetted dependencies:
  tokio:1.29.1 missing ["safe-to-deploy"]
  tracing:0.1.37 missing ["safe-to-deploy"]
  tracing-attributes:0.1.24 missing ["safe-to-deploy"]
  tracing-core:0.1.30 missing ["safe-to-deploy"]

recommended audits for safe-to-deploy:
    Command                                      Publisher   Used By                                   Audit Size
    cargo vet inspect tracing-attributes 0.1.24  hawkw       tracing                                   5025 lines
    cargo vet inspect tracing-core 0.1.30        hawkw       tracing                                   7032 lines
    cargo vet inspect tracing 0.1.34             hawkw       h2, warp, hyper, quinn-udp, and 2 others  11016 lines
    cargo vet inspect tokio 1.29.1               carllerche  h2, warp, hyper, neqo-bin, and 7 others   112764 lines
      NOTE: this project trusts Carl Lerche (carllerche) - consider cargo vet trust tokio carllerche

estimated audit backlog: 135837 lines

cargo vet output after:

➜  mozilla-unified git:(quic-rust-udp-io) ✗ ./mach cargo vet 
[INFO] Using /home/mxinden/code/mozilla.org/mozilla-unified/.cargo/config.toml.
Vetting Failed!

3 unvetted dependencies:
  tracing:0.1.37 missing ["safe-to-deploy"]
  tracing-attributes:0.1.24 missing ["safe-to-deploy"]
  tracing-core:0.1.30 missing ["safe-to-deploy"]

recommended audits for safe-to-deploy:
    Command                                      Publisher  Used By                                   Audit Size
    cargo vet inspect tracing-attributes 0.1.24  hawkw      tracing                                   5025 lines
    cargo vet inspect tracing-core 0.1.30        hawkw      tracing                                   7032 lines
    cargo vet inspect tracing 0.1.34             hawkw      h2, warp, hyper, quinn-udp, and 2 others  11016 lines

estimated audit backlog: 23073 lines

Use |cargo vet certify| to record the audits.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 17, 2024 •
edited
Loading

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

@mxinden mxinden mentioned this pull request Jul 17, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 17, 2024 •
edited
Loading

Firefox builds for this PR

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

@larseggert larseggert added the needs-review Needs a review by a CODEOWNER. label Jul 17, 2024
@larseggert
Fix clippy & doc
017458e 
@mxinden please double-check
@codecov Codecov
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.99%. Comparing base (e05cc70) to head (017458e).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1988      +/-   ##
==========================================
+ Coverage   94.96%   94.99%   +0.03%     
==========================================
  Files         112      112              
  Lines       36504    36459      -45     
==========================================
- Hits        34667    34636      -31     
+ Misses       1837     1823      -14

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

@github-actions GitHub Actions
Copy link

Benchmark results

Performance differences relative to ef4a06c.

coalesce_acked_from_zero 1+1 entries: No change in performance detected.
       time:   [191.52 ns 191.89 ns 192.29 ns]
       change: [-0.4732% -0.1538% +0.1820%] (p = 0.35 > 0.05)

Found 8 outliers among 100 measurements (8.00%)

3 (3.00%) high mild

5 (5.00%) high severe

coalesce_acked_from_zero 3+1 entries: No change in performance detected.
       time:   [232.49 ns 233.02 ns 233.61 ns]
       change: [-0.2158% +0.1157% +0.4337%] (p = 0.52 > 0.05)

Found 11 outliers among 100 measurements (11.00%)

11 (11.00%) high severe

coalesce_acked_from_zero 10+1 entries: No change in performance detected.
       time:   [231.84 ns 232.48 ns 233.30 ns]
       change: [-1.0180% -0.2648% +0.5237%] (p = 0.49 > 0.05)

Found 9 outliers among 100 measurements (9.00%)

2 (2.00%) low mild

7 (7.00%) high severe

coalesce_acked_from_zero 1000+1 entries: No change in performance detected.
       time:   [213.98 ns 214.32 ns 214.67 ns]
       change: [-5.7084% -1.6131% +0.8165%] (p = 0.61 > 0.05)

Found 5 outliers among 100 measurements (5.00%)

1 (1.00%) high mild

4 (4.00%) high severe

RxStreamOrderer::inbound_frame(): Change within noise threshold.
       time:   [118.98 ms 119.17 ms 119.41 ms]
       change: [-0.0073% +0.2660% +0.5418%] (p = 0.04 < 0.05)

Found 2 outliers among 100 measurements (2.00%)

1 (1.00%) high mild

1 (1.00%) high severe

transfer/Run multiple transfers with varying seeds: No change in performance detected.
       time:   [54.390 ms 57.359 ms 60.391 ms]
       thrpt:  [66.235 MiB/s 69.737 MiB/s 73.542 MiB/s]
change:
       time:   [-7.8324% -0.9998% +6.2566%] (p = 0.78 > 0.05)
       thrpt:  [-5.8882% +1.0099% +8.4980%]

Found 1 outliers among 100 measurements (1.00%)

1 (1.00%) high mild

transfer/Run multiple transfers with the same seed: No change in performance detected.
       time:   [63.946 ms 70.626 ms 77.281 ms]
       thrpt:  [51.759 MiB/s 56.636 MiB/s 62.553 MiB/s]
change:
       time:   [-13.289% -0.8248% +14.109%] (p = 0.90 > 0.05)
       thrpt:  [-12.365% +0.8317% +15.325%]
1-conn/1-100mb-resp (aka. Download)/client: No change in performance detected.
       time:   [278.85 ms 286.29 ms 294.89 ms]
       thrpt:  [339.11 MiB/s 349.30 MiB/s 358.62 MiB/s]
change:
       time:   [-2.8253% +0.3915% +3.8635%] (p = 0.82 > 0.05)
       thrpt:  [-3.7198% -0.3899% +2.9075%]

Found 1 outliers among 10 measurements (10.00%)

1 (10.00%) high mild

1-conn/10_000-parallel-1b-resp (aka. RPS)/client: 💔 Performance has regressed.
       time:   [452.09 ms 455.00 ms 457.86 ms]
       thrpt:  [21.841 Kelem/s 21.978 Kelem/s 22.119 Kelem/s]
change:
       time:   [+2.1492% +3.1238% +4.2305%] (p = 0.00 < 0.05)
       thrpt:  [-4.0588% -3.0292% -2.1040%]
1-conn/1-1b-resp (aka. HPS)/client: No change in performance detected.
       time:   [68.321 ms 68.683 ms 69.094 ms]
       thrpt:  [14.473  elem/s 14.560  elem/s 14.637  elem/s]
change:
       time:   [-1.3993% -0.5159% +0.3939%] (p = 0.26 > 0.05)
       thrpt:  [-0.3923% +0.5186% +1.4192%]

Found 10 outliers among 100 measurements (10.00%)

1 (1.00%) low mild

1 (1.00%) high mild

8 (8.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 118.3 ± 21.0 103.5 177.3 1.00
neqo msquic reno on 267.2 ± 10.2 257.0 285.5 1.00
neqo msquic reno 262.6 ± 8.9 253.4 280.6 1.00
neqo msquic cubic on 268.4 ± 14.4 242.2 286.8 1.00
neqo msquic cubic 264.4 ± 10.0 250.4 282.6 1.00
msquic neqo reno on 154.1 ± 13.1 115.4 178.2 1.00
msquic neqo reno 145.7 ± 10.5 135.5 163.7 1.00
msquic neqo cubic on 152.9 ± 15.8 116.7 176.2 1.00
msquic neqo cubic 171.1 ± 26.6 113.4 207.3 1.00
neqo neqo reno on 182.7 ± 10.8 161.8 206.5 1.00
neqo neqo reno 175.6 ± 18.9 156.2 220.7 1.00
neqo neqo cubic on 180.2 ± 17.8 149.9 202.3 1.00
neqo neqo cubic 177.9 ± 11.1 164.0 196.0 1.00

⬇️ Download logs

@mxinden
Copy link
Collaborator Author

mxinden commented Jul 17, 2024

image

Looks good. Thank you @larseggert.

mxinden
mxinden commented Jul 17, 2024
View reviewed changes
neqo-udp/src/lib.rs Show resolved Hide resolved
neqo-udp/src/lib.rs Show resolved Hide resolved
neqo-bin/src/udp.rs Show resolved Hide resolved
KershawChang
KershawChang approved these changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@KershawChang KershawChang left a comment

Choose a reason for hiding this comment

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

Make sense to me.

@mxinden
Copy link
Collaborator Author

mxinden commented Jul 17, 2024

Correcting an (at least) misleading statement I made earlier in the h3 call:

@larseggert: ~"Why haven't we hit this issue of cargo vet requiring an audit for an unused feature earlier?

cargo vet will require an audit for an unused feature iff that feature is indeed used somewhere else within the workspace. In this concrete case, the tokio feature wasn't used by Firefox, but it was used by the http3server testing server. This significantly narrows the scope where this issue can occur and thus this might explain why we haven't hit this issue earlier.

Note that this does not change the plan here. In other words, we should still continue with this pull request.

@larseggert larseggert added this pull request to the merge queue Jul 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 17, 2024
@larseggert larseggert added this pull request to the merge queue Jul 17, 2024
@larseggert larseggert removed the needs-review Needs a review by a CODEOWNER. label Jul 17, 2024
Merged via the queue into mozilla:main with commit e82da39 Jul 17, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KershawChang KershawChang KershawChang approved these changes

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

@larseggert larseggert Awaiting requested review from larseggert larseggert 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.
3 participants
@mxinden @KershawChang @larseggert