-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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)
/cc @lobeck |
lgtm |
I've deployed it to one of the environments, will keep you posted about the performance impact |
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) |
think this can go in regardless? Perf improvements look impressive enough |
Agreed. Won't be ground breaking, but definitely a nice improvement. |
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.Fixes #1452