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(bin): consider env when no verbosity flag is set #1848

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Apr 19, 2024

There are two ways to specify the maximum log level of neqo-server and neqo-client. Via the RUST_LOG environment variable and since #1692 via the verbosity flags (e.g. -v).

Previously, if no verbosity flag was provided, INFO was set as the maximum log level, the RUST_LOG environment variable was simply ignored.

With this commit, if no flag is set, the RUST_LOG environment variable is considered, and only then the env_logger crate default value (ERROR) is used.

In other words, the precedence order now is:

  1. command line flags (e.g. -v)
  2. RUST_LOG environment variable
  3. default ERROR level

Among other places, ensures the RUST_LOG=debug in the QUIC Interop tests isn't ignored.

RUST_LOG=debug RUST_BACKTRACE=1 neqo-client --cc cubic --qns-test "$TESTCASE" \

There are two ways to specify the maximum log level of `neqo-server` and
`neqo-client`. Via the `RUST_LOG` environment variable and since
mozilla#1692 via the verbosity flags (e.g. `-v`).

Previously, if no verbosity flag was provided, `INFO` was set as the maximum log
level, the `RUST_LOG` environment variable was simply ignored.

With this commit, if no flag is set, the `RUST_LOG` environment variable is
considered, and only then the `env_logger` crate default value (`ERROR`) is used.

In other words, the precedence order now is:

1. command line flags (e.g. `-v`)
2. `RUST_LOG` environment variable
3. default `ERROR` level
@mxinden
Copy link
Collaborator Author

mxinden commented Apr 19, 2024

We can as well set the final default log level to INFO instead of ERROR. Not sure whether that interferes with Firefox though. Does Firefox explicitly set the log level?

modified   neqo-common/src/log.rs
@@ -12,7 +12,7 @@ use std::{
     time::{Duration, Instant},
 };
 
-use env_logger::Builder;
+use env_logger::{Builder, Env};
 
 #[macro_export]
 macro_rules! do_log {
@@ -58,7 +58,7 @@ pub fn init(level_filter: Option<log::LevelFilter>) {
     }
 
     INIT_ONCE.call_once(|| {
-        let mut builder = Builder::from_env("RUST_LOG");
+        let mut builder = Builder::from_env(Env::default().default_filter_or("info"));
         if let Some(filter) = level_filter {
             builder.filter_level(filter);
         }

Copy link

github-actions bot commented Apr 22, 2024

Benchmark results

Performance differences relative to 0a84268.

  • drain a timer quickly time: [311.14 ns 318.91 ns 325.91 ns]
    change: [-2.6316% -0.4498% +1.9091%] (p = 0.70 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [196.19 ns 196.65 ns 197.20 ns]
    change: [-0.1377% +0.2320% +0.5800%] (p = 0.22 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 3+1 entries
    time: [239.10 ns 239.65 ns 240.24 ns]
    change: [-0.4100% -0.0646% +0.2613%] (p = 0.71 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 10+1 entries
    time: [238.59 ns 239.34 ns 240.24 ns]
    change: [-0.1477% +0.4100% +1.0657%] (p = 0.19 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1000+1 entries
    time: [218.53 ns 218.75 ns 219.01 ns]
    change: [-1.4254% -0.7080% +0.0315%] (p = 0.05 > 0.05)
    No change in performance detected.

  • RxStreamOrderer::inbound_frame()
    time: [118.72 ms 118.81 ms 118.91 ms]
    change: [+0.3366% +0.4299% +0.5321%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [120.36 ms 120.65 ms 120.92 ms]
    thrpt: [33.079 MiB/s 33.155 MiB/s 33.234 MiB/s]
    change:
    time: [+2.0714% +2.3897% +2.7089%] (p = 0.00 < 0.05)
    thrpt: [-2.6374% -2.3339% -2.0293%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [120.08 ms 120.32 ms 120.56 ms]
    thrpt: [33.178 MiB/s 33.244 MiB/s 33.311 MiB/s]
    change:
    time: [+1.5877% +1.8971% +2.1931%] (p = 0.00 < 0.05)
    thrpt: [-2.1460% -1.8618% -1.5629%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.0757 s 1.0798 s 1.0860 s]
    thrpt: [92.080 MiB/s 92.607 MiB/s 92.967 MiB/s]
    change:
    time: [-1.8684% -0.5350% +0.5988%] (p = 0.46 > 0.05)
    thrpt: [-0.5952% +0.5379% +1.9040%]
    No change in performance detected.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [431.64 ms 433.92 ms 436.18 ms]
    thrpt: [22.926 Kelem/s 23.046 Kelem/s 23.168 Kelem/s]
    change:
    time: [+0.0912% +0.8867% +1.6414%] (p = 0.02 < 0.05)
    thrpt: [-1.6149% -0.8789% -0.0911%]
    Change within noise threshold.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [48.594 ms 48.986 ms 49.364 ms]
    thrpt: [20.257 elem/s 20.414 elem/s 20.579 elem/s]
    change:
    time: [-0.8885% +0.2097% +1.3438%] (p = 0.72 > 0.05)
    thrpt: [-1.3260% -0.2093% +0.8965%]
    No change in performance detected.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 737.9 ± 337.6 350.0 1330.1 1.00
neqo msquic reno on 851.8 ± 128.0 753.8 1191.6 1.00
neqo msquic reno 919.4 ± 251.1 735.3 1467.1 1.00
neqo msquic cubic on 1038.9 ± 304.0 766.8 1485.7 1.00
neqo msquic cubic 900.8 ± 204.7 756.0 1337.4 1.00
msquic neqo reno on 4600.0 ± 265.2 4262.3 5030.9 1.00
msquic neqo reno 4388.5 ± 292.3 4124.4 5023.0 1.00
msquic neqo cubic on 4473.1 ± 213.0 4149.2 4941.3 1.00
msquic neqo cubic 4523.1 ± 302.9 4135.9 4973.4 1.00
neqo neqo reno on 3623.2 ± 271.2 3252.6 4121.6 1.00
neqo neqo reno 3638.6 ± 341.2 3301.1 4345.5 1.00
neqo neqo cubic on 4451.8 ± 162.6 4146.5 4719.5 1.00
neqo neqo cubic 4278.8 ± 385.1 3425.1 4966.2 1.00

⬇️ Download logs

Copy link
Collaborator

@larseggert larseggert left a comment

Choose a reason for hiding this comment

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

Is there a way to (easily) move this to neqo-bin/lib.rs instead of doing it in both the client and server?

@mxinden
Copy link
Collaborator Author

mxinden commented Apr 24, 2024

Is there a way to (easily) move this to neqo-bin/lib.rs instead of doing it in both the client and server?

@larseggert I moved the verbose field into SharedArgs. Not sure how to de-deuplicate this further. Mind taking another look?

Copy link

Failed Interop Tests

Succeeded and unsupported tests

Succeeded Interop Tests

Unsupported Interop Tests

@larseggert larseggert added this pull request to the merge queue Apr 25, 2024
Merged via the queue into mozilla:main with commit 17c5895 Apr 25, 2024
47 checks passed
larseggert added a commit to larseggert/neqo that referenced this pull request Apr 30, 2024
larseggert added a commit that referenced this pull request Apr 30, 2024
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.

2 participants