[GH-ISSUE #3004] Remove lock in NameServerStats #1108

New issue

Open

opened 2026-03-16 01:37:13 +03:00 by kerem · 2 comments

kerem commented

2026-03-16 01:37:13 +03:00

Owner

Originally created by @divergentdave on GitHub (May 21, 2025).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/3004

NameServerStats currently tracks a modified exponentially-weighted moving average of name server latency, using a combination of an AtomicU32 and a parking_lot::Mutex. This lock gets taken inside tight loops, such as when sorting connections in a pool.

We could replace both fields of NameServerStats with a single AtomicU128 from the portable-atomics crate. This would let us remove the locking, while still ensuring that the smoothed RTT and last update timestamp are updated atomically. The smoothed RTT is currently stored in a 32 byte integer. We can first turn the Instant inside last_update into a Duration by taking the duration since some baseline time stored in a OnceCell. Then, we can destructure that Duration using as_secs() and subsec_nanos(). These methods return a u64 and u32 respectively. (The atomic-time crate does something similar to this) We could set aside a seconds field of u64::MAX as a sentinel value to represent None. Then, all of these would fit neatly into a u128. The documentation for portable-atomics says that there is broad support for 128-bit atomic operations on modern hardware. Failing that, the crate provides a slower fallback using global locks. Various NameServerStats methods would be changed to do a single fetch_update() on this packed field, instead of locking the mutex and then doing a fetch_update() on the other field.

Originally created by @divergentdave on GitHub (May 21, 2025). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/3004 `NameServerStats` currently tracks a modified exponentially-weighted moving average of name server latency, using a combination of an `AtomicU32` and a `parking_lot::Mutex`. This lock gets taken inside tight loops, such as when sorting connections in a pool. We could replace both fields of `NameServerStats` with a single `AtomicU128` from the `portable-atomics` crate. This would let us remove the locking, while still ensuring that the smoothed RTT and last update timestamp are updated atomically. The smoothed RTT is currently stored in a 32 byte integer. We can first turn the `Instant` inside `last_update` into a `Duration` by taking the duration since some baseline time stored in a `OnceCell`. Then, we can destructure that `Duration` using `as_secs()` and `subsec_nanos()`. These methods return a `u64` and `u32` respectively. (The `atomic-time` crate does something similar to this) We could set aside a seconds field of `u64::MAX` as a sentinel value to represent `None`. Then, all of these would fit neatly into a `u128`. The documentation for `portable-atomics` says that there is broad support for 128-bit atomic operations on modern hardware. Failing that, the crate provides a slower fallback using global locks. Various `NameServerStats` methods would be changed to do a single `fetch_update()` on this packed field, instead of locking the mutex and then doing a `fetch_update()` on the other field.

kerem commented

2026-03-16 01:37:19 +03:00

Author

Owner

@djc commented on GitHub (May 21, 2025):

Did you see this show up as hot in an actual profile? IMO we should only do this if it does.

@djc commented on GitHub (May 21, 2025): Did you see this show up as hot in an actual profile? IMO we should only do this if it does.

kerem commented

2026-03-16 01:37:24 +03:00

Author

Owner

@divergentdave commented on GitHub (May 21, 2025):

No, I haven't tried measuring this yet.

@divergentdave commented on GitHub (May 21, 2025): No, I haven't tried measuring this yet.