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

Improve IsFQDN performance #1453

Merged
merged 2 commits into from Apr 27, 2023
Merged

Improve IsFQDN performance #1453

merged 2 commits into from Apr 27, 2023

Conversation

tmthrgd
Copy link
Collaborator

@tmthrgd tmthrgd commented Apr 18, 2023

While this code may be slightly less clear, it's significantly faster and this function seems to be a hot path for certain workloads.

This function still has significant overhead for names that have an escape sequence immediately before the final dot. While an important edge case to handle, these should be vanishingly rare with most callers noticing the speedup. This change even offers a rather decent performance improvement for these names anyway.

I tried splitting IsFQDN into a fast and slow path for better inlining, but couldn't get it below the inlining threshold no matter what I tried, so I have left it as one function.

name                 old time/op    new time/op    delta
IsFQDN/no_dot-12       5.86ns ± 2%    1.48ns ± 3%  -74.71%  (p=0.000 n=10+10)
IsFQDN/unescaped-12    8.73ns ± 2%    1.57ns ± 1%  -81.98%  (p=0.000 n=9+8)
IsFQDN/escaped-12      27.4ns ± 2%    23.8ns ± 2%  -13.19%  (p=0.000 n=10+10)
FQDN/is_fqdn-12        8.36ns ± 1%    1.80ns ± 2%  -78.50%  (p=0.000 n=9+10)
FQDN/not_fqdn-12       36.8ns ±15%    33.4ns ±12%   -9.25%  (p=0.035 n=10+10)

Fixes #1452

While this code may be slightly less clear, it's significantly faster
and this function seems to be a hot path for certain workloads.

name                 old time/op    new time/op    delta
IsFQDN/no_dot-12       5.86ns ± 2%    1.48ns ± 3%  -74.71%  (p=0.000 n=10+10)
IsFQDN/unescaped-12    8.73ns ± 2%    1.57ns ± 1%  -81.98%  (p=0.000 n=9+8)
IsFQDN/escaped-12      27.4ns ± 2%    23.8ns ± 2%  -13.19%  (p=0.000 n=10+10)
FQDN/is_fqdn-12        8.36ns ± 1%    1.80ns ± 2%  -78.50%  (p=0.000 n=9+10)
FQDN/not_fqdn-12       36.8ns ±15%    33.4ns ±12%   -9.25%  (p=0.035 n=10+10)
@tmthrgd tmthrgd requested a review from miekg as a code owner April 18, 2023 02:22
@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Apr 18, 2023

/cc @lobeck

@miekg
Copy link
Owner

miekg commented Apr 18, 2023

lgtm

defaults.go Show resolved Hide resolved
defaults.go Show resolved Hide resolved
@lobeck
Copy link

lobeck commented Apr 18, 2023

I've deployed it to one of the environments, will keep you posted about the performance impact

@lobeck
Copy link

lobeck commented Apr 19, 2023

I'm sorry, but I can't provide traces for the moment. I've let it run in one of the environments and didn't see much of a change. But we've found the root cause for the load, so currently we're busy addressing this first. (The root cause is a buggy client doing a query every 10ms)

defaults.go Show resolved Hide resolved
defaults.go Show resolved Hide resolved
@miekg
Copy link
Owner

miekg commented Apr 27, 2023

think this can go in regardless? Perf improvements look impressive enough

@tmthrgd
Copy link
Collaborator Author

tmthrgd commented Apr 27, 2023

think this can go in regardless? Perf improvements look impressive enough

Agreed. Won't be ground breaking, but definitely a nice improvement.

@miekg miekg merged commit 4bdf302 into miekg:master Apr 27, 2023
4 checks passed
@tmthrgd tmthrgd deleted the isfqdn_perf branch April 29, 2023 04:52
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.

IsFqdn performance issues
5 participants