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

Reapply "perf(transport): remove Server::timers (#1784)" (#1800) #1902

Merged
merged 1 commit into from
May 15, 2024
Merged

Reapply "perf(transport): remove Server::timers (#1784)" (#1800) #1902

merged 1 commit into from
May 15, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented May 13, 2024 •
edited
Loading

This reverts commit 342e4e7 which reverted 61fcd28.

With #1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now reapply the patch removing Server::timers.

More specifically, the actual bug fix on mozilla-central side:

let output = if self.response_to_send.is_empty() {
    output
} else {
    // In case there are pending responses to send, make sure a reasonable
    // callback is returned.
    const MIN_INTERVAL: Duration = Duration::from_millis(100);

    match output {
        Output::None => Output::Callback(MIN_INTERVAL),
        o @ Output::Datagram(_) => o,
        Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)),
    }
};

See https://phabricator.services.mozilla.com/D209574.

(I don't know how to link to a specific line of a phabricator patch. Is that possible?)

@mxinden
Reapply "perf(transport): remove Server::timers (mozilla#1784)" (mozi…
b26876c 
…lla#1800)

This reverts commit 342e4e7.

With mozilla#1878 merged and
https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now
reapply the patch removing `Server::timers`.

More specifically, the actual bug fix on mozilla-central side:

``` rust
let output = if self.response_to_send.is_empty() {
    output
} else {
    // In case there are pending responses to send, make sure a reasonable
    // callback is returned.
    const MIN_INTERVAL: Duration = Duration::from_millis(100);

    match output {
        Output::None => Output::Callback(MIN_INTERVAL),
        o @ Output::Datagram(_) => o,
        Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)),
    }
};
```

See https://phabricator.services.mozilla.com/D209574.
@github-actions GitHub Actions
Copy link

Failed Interop Tests

None 🎉

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

@KershawChang
Copy link
Collaborator

I'll make a try push with PR and let you know the result.

@codecov Codecov
Copy link

codecov bot commented May 13, 2024

Codecov Report

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

Project coverage is 93.19%. Comparing base (e44c472) to head (b26876c).

Files Patch % Lines
neqo-transport/src/server.rs 96.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1902      +/-   ##
==========================================
- Coverage   93.20%   93.19%   -0.01%     
==========================================
  Files         111      110       -1     
  Lines       36025    35760     -265     
==========================================
- Hits        33578    33328     -250     
+ Misses       2447     2432      -15

☔ 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 e44c472.

  • coalesce_acked_from_zero 1+1 entries
    time: [196.61 ns 197.12 ns 197.66 ns]
    change: [-0.1042% +0.7193% +1.3009%] (p = 0.03 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 3+1 entries
    time: [237.28 ns 237.86 ns 238.49 ns]
    change: [-0.2128% +0.1422% +0.4833%] (p = 0.44 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 10+1 entries
    time: [236.79 ns 237.52 ns 238.37 ns]
    change: [-0.2989% +0.1506% +0.5955%] (p = 0.52 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1000+1 entries
    time: [218.97 ns 219.15 ns 219.35 ns]
    change: [-0.1458% +0.5675% +1.3307%] (p = 0.15 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [119.75 ms 119.85 ms 119.95 ms]
    change: [-0.0280% +0.1759% +0.3261%] (p = 0.04 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [119.32 ms 119.58 ms 119.85 ms]
    thrpt: [33.374 MiB/s 33.450 MiB/s 33.524 MiB/s]
    change:
    time: [-0.9328% -0.6117% -0.2914%] (p = 0.00 < 0.05)
    thrpt: [+0.2923% +0.6155% +0.9416%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [119.49 ms 119.73 ms 119.98 ms]
    thrpt: [33.340 MiB/s 33.409 MiB/s 33.475 MiB/s]
    change:
    time: [-0.8154% -0.5306% -0.2342%] (p = 0.00 < 0.05)
    thrpt: [+0.2348% +0.5335% +0.8221%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1165 s 1.1360 s 1.1607 s]
    thrpt: [86.153 MiB/s 88.031 MiB/s 89.566 MiB/s]
    change:
    time: [+1.1422% +3.3390% +5.7141%] (p = 0.01 < 0.05)
    thrpt: [-5.4052% -3.2311% -1.1293%]
    💔 Performance has regressed.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [390.34 ms 393.28 ms 396.24 ms]
    thrpt: [25.237 Kelem/s 25.427 Kelem/s 25.619 Kelem/s]
    change:
    time: [-9.1912% -8.3330% -7.4816%] (p = 0.00 < 0.05)
    thrpt: [+8.0866% +9.0905% +10.122%]
    💚 Performance has improved.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [41.720 ms 41.839 ms 41.977 ms]
    thrpt: [23.823 elem/s 23.901 elem/s 23.970 elem/s]
    change:
    time: [-16.717% -16.273% -15.829%] (p = 0.00 < 0.05)
    thrpt: [+18.806% +19.435% +20.073%]
    💚 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 596.9 ± 239.8 381.4 985.2 1.00
neqo msquic reno on 1026.2 ± 268.1 788.8 1376.5 1.00
neqo msquic reno 897.1 ± 211.0 767.7 1365.2 1.00
neqo msquic cubic on 926.3 ± 224.7 766.0 1424.6 1.00
neqo msquic cubic 908.1 ± 212.1 753.6 1391.5 1.00
msquic neqo reno on 4255.1 ± 187.5 4109.3 4776.7 1.00
msquic neqo reno 4304.7 ± 298.5 4054.2 4977.5 1.00
msquic neqo cubic on 3987.0 ± 82.6 3912.6 4161.6 1.00
msquic neqo cubic 3927.3 ± 103.1 3763.8 4142.6 1.00
neqo neqo reno on 3354.4 ± 236.5 2972.3 3675.1 1.00
neqo neqo reno 3341.2 ± 87.3 3221.4 3472.0 1.00
neqo neqo cubic on 3800.2 ± 202.9 3562.9 4298.9 1.00
neqo neqo cubic 3781.2 ± 71.2 3639.8 3844.8 1.00

⬇️ Download logs

@github-actions GitHub Actions
Copy link

Firefox builds for this PR

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

@mxinden
Copy link
Collaborator Author

mxinden commented May 14, 2024

I'll make a try push with PR and let you know the result.

Forgot to mention that I tested this PR locally via:

./mach mochitest --use-http3-server devtools/client/inspector/test/browser_inspector_reload_iframe.js --headless

That said, a "try push" to run all tests is great. Thank you. Let me know in case anything comes up. Happy to help debugging @KershawChang.

@KershawChang
Copy link
Collaborator

Try push looks good.
Thanks, Max!

@KershawChang KershawChang added this pull request to the merge queue May 15, 2024
Merged via the queue into mozilla:main with commit aebc9ca May 15, 2024
54 of 56 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.
2 participants
@mxinden @KershawChang