NotificationsYou must be signed in to change notification settings
Fork771
Star5.8k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Use state machine to parse directives#3243

Open

djc wants to merge2 commits intov0.1.x

base:v0.1.x

Choose a base branch

fromparse-directive

Open

Use state machine to parse directives#3243

djc wants to merge2 commits intov0.1.xfromparse-directive

+127 −109

Conversation

Copy link

Contributor

djc commentedMar 25, 2025

Intended tofix#3174. Have not done any benchmarks yet -- suggestions on how best to approach that are welcome.

This is a little bit more code, but IMO maybe slightly more readable?

It's correct enough that it passes the tests, there might be edge cases that aren't covered but those should be easy to solve.

djc requested review fromhawkw anda team ascode owners

March 25, 2025 20:54

djc mentioned this pull request

Mar 25, 2025

tracing-subscriber's Directive parsing adds 600us to startup time#3174

Open

dpc reviewed

Mar 26, 2025

View reviewed changes

tracing-subscriber/src/filter/env/directive.rs OutdatedShow resolvedHide resolved

Copy link

Contributor

dpc commentedMar 26, 2025•
edited
Loading

> hyperfine --warmup 30 -N ./target/release/dotr-before ./target/release/dotr-afterBenchmark 1: ./target/release/dotr-before  Time (mean ± σ):       1.2 ms ±   0.1 ms    [User: 0.4 ms, System: 0.7 ms]  Range (min … max):     1.1 ms …   1.9 ms    2044 runsBenchmark 2: ./target/release/dotr-after  Time (mean ± σ):     613.8 µs ±  79.6 µs    [User: 289.1 µs, System: 254.0 µs]  Range (min … max):   511.5 µs … 979.8 µs    5264 runsSummary  ./target/release/dotr-after ran    2.03 ± 0.34 times faster than ./target/release/dotr-before

Addedtrue just to have a baseline:

Benchmark 1: ./target/release/dotr-before  Time (mean ± σ):       1.2 ms ±   0.1 ms    [User: 0.4 ms, System: 0.7 ms]  Range (min … max):     1.1 ms …   1.8 ms    2535 runsBenchmark 2: ./target/release/dotr-after  Time (mean ± σ):     611.7 µs ±  80.3 µs    [User: 295.6 µs, System: 244.1 µs]  Range (min … max):   518.8 µs … 1663.5 µs    3825 runsBenchmark 3: true  Time (mean ± σ):     595.8 µs ±  74.1 µs    [User: 311.4 µs, System: 215.4 µs]  Range (min … max):   512.3 µs … 1035.5 µs    5362 runs

This app setsRUST_LOG internally, so setting it from the outside doesn't make a difference.

AFAICT you fixed it and it's nearly free now.