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(server): log error when failing to read file #1788

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Apr 2, 2024

As part of the QUIC Interop testcases, a server needs to be able to respond to a request with the content of a local file.

Previously, when failing to read a file, the server would simply reply with the predefined SELF::MESSAGE.

Now the server logs the error and moves on.

As part of the QUIC Interop testcases, a server needs to be able to respond to a
request with the content of a local file.

Previously, when failing to read a file, the server would simply reply with the
predefined `SELF::MESSAGE`.

Now the server logs the error and moves on.
Copy link

github-actions bot commented Apr 2, 2024

Benchmark results

Performance differences relative to 32a2a59.

  • drain a timer quickly time: [368.24 ns 375.38 ns 382.14 ns]
    change: [+1.2407% +3.0134% +4.7758%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 1+1 entries
    time: [195.72 ns 196.13 ns 196.57 ns]
    change: [-0.0785% +0.3594% +0.7917%] (p = 0.14 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 3+1 entries
    time: [240.50 ns 241.10 ns 241.74 ns]
    change: [+1.7406% +2.0694% +2.4287%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 10+1 entries
    time: [239.11 ns 239.73 ns 240.52 ns]
    change: [+1.3696% +1.8024% +2.1760%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 1000+1 entries
    time: [219.31 ns 219.58 ns 219.89 ns]
    change: [-0.1802% +0.5869% +1.4018%] (p = 0.15 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [118.97 ms 119.03 ms 119.10 ms]
    change: [-0.2632% -0.1628% -0.0635%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [119.82 ms 120.11 ms 120.40 ms]
    thrpt: [33.222 MiB/s 33.302 MiB/s 33.383 MiB/s]
    change:
    time: [-2.1619% -1.8560% -1.5250%] (p = 0.00 < 0.05)
    thrpt: [+1.5486% +1.8911% +2.2096%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [120.97 ms 121.15 ms 121.34 ms]
    thrpt: [32.964 MiB/s 33.017 MiB/s 33.066 MiB/s]
    change:
    time: [-1.6658% -1.4605% -1.2551%] (p = 0.00 < 0.05)
    thrpt: [+1.2710% +1.4822% +1.6940%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1138 s 1.1260 s 1.1401 s]
    thrpt: [87.713 MiB/s 88.811 MiB/s 89.781 MiB/s]
    change:
    time: [-0.2758% +2.2226% +4.5287%] (p = 0.09 > 0.05)
    thrpt: [-4.3325% -2.1742% +0.2766%]
    No change in performance detected.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [429.26 ms 431.49 ms 433.72 ms]
    thrpt: [23.057 Kelem/s 23.175 Kelem/s 23.296 Kelem/s]
    change:
    time: [+10.252% +11.205% +12.160%] (p = 0.00 < 0.05)
    thrpt: [-10.842% -10.076% -9.2990%]
    💔 Performance has regressed.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [51.287 ms 51.696 ms 52.096 ms]
    thrpt: [19.195 elem/s 19.344 elem/s 19.498 elem/s]
    change:
    time: [+20.258% +21.365% +22.387%] (p = 0.00 < 0.05)
    thrpt: [-18.292% -17.604% -16.845%]
    💔 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 671.8 ± 187.4 486.4 1061.6 1.00
neqo msquic reno on 1126.6 ± 328.4 746.2 1723.1 1.00
neqo msquic reno 832.6 ± 149.2 719.4 1233.6 1.00
neqo msquic cubic on 998.9 ± 283.3 762.4 1402.8 1.00
neqo msquic cubic 946.6 ± 197.0 766.6 1334.2 1.00
msquic neqo reno on 4494.5 ± 237.9 4154.4 4877.7 1.00
msquic neqo reno 4400.6 ± 262.7 4181.1 5077.1 1.00
msquic neqo cubic on 4595.8 ± 294.2 4181.1 4986.0 1.00
msquic neqo cubic 4546.5 ± 218.3 4271.7 4957.2 1.00
neqo neqo reno on 3939.9 ± 348.6 3591.2 4493.8 1.00
neqo neqo reno 3544.0 ± 196.6 3141.2 3772.1 1.00
neqo neqo cubic on 4378.9 ± 114.8 4221.5 4536.9 1.00
neqo neqo cubic 4298.5 ± 185.5 4026.4 4656.1 1.00

⬇️ Download logs

@mxinden mxinden marked this pull request as ready for review April 9, 2024 09:02
@mxinden
Copy link
Collaborator Author

mxinden commented Apr 9, 2024

@martinthomson would you mind taking another look?

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.13%. Comparing base (3f3c5f6) to head (1144d3f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1788   +/-   ##
=======================================
  Coverage   93.13%   93.13%           
=======================================
  Files         117      117           
  Lines       36363    36363           
=======================================
  Hits        33865    33865           
  Misses       2498     2498           

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

@larseggert larseggert added this pull request to the merge queue Apr 9, 2024
Merged via the queue into mozilla:main with commit 1cc44f0 Apr 9, 2024
15 checks passed
mxinden added a commit to mxinden/neqo that referenced this pull request May 4, 2024
There are two server implementations based on neqo:

1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server
  - http3 and http09 implementation
  - used for manual testing and QUIC Interop

2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs
  - used to test Firefox

I assume one was once an exact copy of the other. Both implement their own I/O,
event loop, ... Since then, the two implementations diverged significantly.
Especially (1) saw a lot of improvements in recent months:

- mozilla#1564
- mozilla#1569
- mozilla#1578
- mozilla#1581
- mozilla#1604
- mozilla#1612
- mozilla#1676
- mozilla#1692
- mozilla#1707
- mozilla#1708
- mozilla#1727
- mozilla#1753
- mozilla#1756
- mozilla#1766
- mozilla#1772
- mozilla#1786
- mozilla#1787
- mozilla#1788
- mozilla#1794
- mozilla#1806
- mozilla#1808
- mozilla#1848
- mozilla#1866

At this point, bugs in (2) are hard to fix, see e.g.
mozilla#1801.

This commit merges (2) into (1), thus removing all duplicate logic and
having (2) benefit from all the recent improvements to (1).
KershawChang pushed a commit to KershawChang/neqo that referenced this pull request May 7, 2024
There are two server implementations based on neqo:

1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server
  - http3 and http09 implementation
  - used for manual testing and QUIC Interop

2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs
  - used to test Firefox

I assume one was once an exact copy of the other. Both implement their own I/O,
event loop, ... Since then, the two implementations diverged significantly.
Especially (1) saw a lot of improvements in recent months:

- mozilla#1564
- mozilla#1569
- mozilla#1578
- mozilla#1581
- mozilla#1604
- mozilla#1612
- mozilla#1676
- mozilla#1692
- mozilla#1707
- mozilla#1708
- mozilla#1727
- mozilla#1753
- mozilla#1756
- mozilla#1766
- mozilla#1772
- mozilla#1786
- mozilla#1787
- mozilla#1788
- mozilla#1794
- mozilla#1806
- mozilla#1808
- mozilla#1848
- mozilla#1866

At this point, bugs in (2) are hard to fix, see e.g.
mozilla#1801.

This commit merges (2) into (1), thus removing all duplicate logic and
having (2) benefit from all the recent improvements to (1).
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2024
* refactor(bin): introduce server/http3.rs and server/http09.rs

The QUIC Interop Runner requires an http3 and http09 implementation for both
client and server. The client code is already structured into an http3 and an
http09 implementation since #1727.

This commit does the same for the server side, i.e. splits the http3 and http09
implementation into separate Rust modules.

* refactor: merge mozilla-central http3 server into neqo-bin

There are two server implementations based on neqo:

1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server
  - http3 and http09 implementation
  - used for manual testing and QUIC Interop

2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs
  - used to test Firefox

I assume one was once an exact copy of the other. Both implement their own I/O,
event loop, ... Since then, the two implementations diverged significantly.
Especially (1) saw a lot of improvements in recent months:

- #1564
- #1569
- #1578
- #1581
- #1604
- #1612
- #1676
- #1692
- #1707
- #1708
- #1727
- #1753
- #1756
- #1766
- #1772
- #1786
- #1787
- #1788
- #1794
- #1806
- #1808
- #1848
- #1866

At this point, bugs in (2) are hard to fix, see e.g.
#1801.

This commit merges (2) into (1), thus removing all duplicate logic and
having (2) benefit from all the recent improvements to (1).

* Move firefox.rs to mozilla-central

* Reduce HttpServer trait functions

* Extract constructor

* Remove unused deps

* Remove clap color feature

Nice to have. Adds multiple dependencies. Hard to justify for mozilla-central.
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.

3 participants