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

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

Merged
merged 1 commit into from
Apr 9, 2024
Merged

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

merged 1 commit into from
Apr 9, 2024

Conversation

KershawChang
Copy link
Collaborator

@KershawChang KershawChang commented Apr 8, 2024 •
edited
Loading

It appears that PR #1784 caused some test failures (failed tests on windows). After reverting it, all http3 tests are passed.

Given that we are reaching the end of this cycle, I think it's probably better to revert PR #1784 first and do a proper fix later.

@codecov Codecov
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

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

Project coverage is 93.12%. Comparing base (aca1352) to head (e93d2ed).
Report is 3 commits behind head on main.

Files Patch % Lines
neqo-common/src/timer.rs 94.33% 15 Missing ⚠️
neqo-transport/src/server.rs 96.77% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #1800    +/-   ##
========================================
  Coverage   93.12%   93.12%            
========================================
  Files         116      117     +1     
  Lines       36097    36362   +265     
========================================
+ Hits        33614    33863   +249     
- Misses       2483     2499    +16

☔ 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 8, 2024

Benchmark results

Performance differences relative to 32a2a59.

  • drain a timer quickly time: [360.30 ns 368.53 ns 376.12 ns]
    change: [-1.0563% +0.6074% +2.4596%] (p = 0.52 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [194.46 ns 194.98 ns 195.50 ns]
    change: [-1.4046% -0.9911% -0.5571%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 3+1 entries
    time: [238.14 ns 238.77 ns 239.41 ns]
    change: [+0.4596% +0.7930% +1.1345%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • coalesce_acked_from_zero 10+1 entries
    time: [236.00 ns 236.65 ns 237.45 ns]
    change: [-0.0671% +0.3838% +0.8078%] (p = 0.08 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1000+1 entries
    time: [219.24 ns 219.33 ns 219.45 ns]
    change: [-0.3752% +0.3033% +1.0468%] (p = 0.42 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [118.56 ms 118.65 ms 118.73 ms]
    change: [-0.5953% -0.4855% -0.3799%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [119.52 ms 119.79 ms 120.05 ms]
    thrpt: [33.318 MiB/s 33.391 MiB/s 33.467 MiB/s]
    change:
    time: [-2.3963% -2.1182% -1.8120%] (p = 0.00 < 0.05)
    thrpt: [+1.8455% +2.1641% +2.4551%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [119.99 ms 120.17 ms 120.35 ms]
    thrpt: [33.237 MiB/s 33.287 MiB/s 33.335 MiB/s]
    change:
    time: [-2.4544% -2.2576% -2.0665%] (p = 0.00 < 0.05)
    thrpt: [+2.1101% +2.3097% +2.5162%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1052 s 1.1125 s 1.1231 s]
    thrpt: [89.036 MiB/s 89.885 MiB/s 90.485 MiB/s]
    change:
    time: [-1.2449% +1.0018% +3.0833%] (p = 0.40 > 0.05)
    thrpt: [-2.9910% -0.9919% +1.2606%]
    No change in performance detected.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [428.81 ms 430.84 ms 432.84 ms]
    thrpt: [23.103 Kelem/s 23.210 Kelem/s 23.320 Kelem/s]
    change:
    time: [+10.129% +11.037% +11.948%] (p = 0.00 < 0.05)
    thrpt: [-10.673% -9.9400% -9.1977%]
    💔 Performance has regressed.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [50.504 ms 50.928 ms 51.307 ms]
    thrpt: [19.491 elem/s 19.635 elem/s 19.800 elem/s]
    change:
    time: [+18.539% +19.564% +20.532%] (p = 0.00 < 0.05)
    thrpt: [-17.034% -16.363% -15.640%]
    💔 Performance has regressed.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 621.4 ± 285.7 387.8 1268.7 1.00
neqo msquic reno on 2026.5 ± 181.0 1906.2 2493.3 1.00
neqo msquic reno 1988.4 ± 166.6 1880.3 2454.9 1.00
neqo msquic cubic on 2042.9 ± 235.3 1892.2 2527.8 1.00
neqo msquic cubic 1930.9 ± 55.2 1827.4 2006.0 1.00
msquic neqo reno on 4473.8 ± 352.0 4129.2 5196.8 1.00
msquic neqo reno 4432.0 ± 187.7 4162.8 4795.2 1.00
msquic neqo cubic on 4506.5 ± 259.5 4219.6 5036.7 1.00
msquic neqo cubic 4585.5 ± 418.8 4231.3 5716.5 1.00
neqo neqo reno on 3750.3 ± 161.7 3471.3 3996.1 1.00
neqo neqo reno 3559.7 ± 323.8 3242.0 4122.5 1.00
neqo neqo cubic on 4098.4 ± 517.9 2816.5 4794.7 1.00
neqo neqo cubic 4397.3 ± 297.7 3932.1 4938.8 1.00

⬇️ Download logs

@larseggert larseggert mentioned this pull request Apr 9, 2024
Closed
@larseggert larseggert added this pull request to the merge queue Apr 9, 2024
Merged via the queue into mozilla:main with commit 342e4e7 Apr 9, 2024
15 checks passed
mxinden added a commit to mxinden/neqo that referenced this pull request May 13, 2024
@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-merge-queue bot pushed a commit that referenced this pull request May 15, 2024
@mxinden
Reapply "perf(transport): remove Server::timers (#1784)" (#1800) (#1902)
aebc9ca 
This reverts commit 342e4e7.

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:

``` 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larseggert larseggert larseggert approved these changes

@martinthomson martinthomson Awaiting requested review from martinthomson martinthomson 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
@KershawChang @larseggert