Optimize Network.Check single-IP path to reduce allocations#49588
Optimize Network.Check single-IP path to reduce allocations#49588
Conversation
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Avoid per-call slice construction in the common single-IP path by handling concrete types directly in Check via a type switch: - Fast-path string and net.IP without building []net.IP - Keep loop semantics for []string and []net.IP - Preserve behavior for invalid/empty values Remove now-unused extractIP function and slices import. Benchmark improvement: ~22% faster, 60% less memory, 50% fewer allocs. Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com> Agent-Logs-Url: https://github.com/elastic/beats/sessions/6a9f4491-418d-484d-a4d0-ac8777846040
📝 WalkthroughWalkthroughThe PR modifies the 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libbeat/conditions/network.go`:
- Around line 166-176: The net.IP branch in the matcher (inside Check) does not
guard against nil or zero-length IPs, so calls like network.Contains(v) can
treat invalid IPs as matches; update the net.IP case to first reject nil or
len(v) == 0 and return false, and similarly ensure containsAny (and
containsAnyStr for string slices) skips or rejects empty/invalid entries in the
[]net.IP and []string branches before calling network.Contains (i.e., add a
nil/len==0 check for v in the net.IP case and ensure containsAny handles
zero-length net.IP elements).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9740e2e9-20f7-424e-83c6-9136c930797e
📒 Files selected for processing (1)
libbeat/conditions/network.go
| case net.IP: | ||
| if !network.Contains(v) { | ||
| return false | ||
| } | ||
| case []net.IP: | ||
| if len(v) == 0 || !containsAny(v, network.Contains) { | ||
| return false | ||
| } | ||
| case []string: | ||
| if len(v) == 0 || !containsAnyStr(v, network.Contains) { | ||
| return false |
There was a problem hiding this comment.
Invalid net.IP values can incorrectly match public and pass Check.
On Line 166 and Line 188, net.IP(nil) / zero-length IPs are not rejected before network.Contains(...). For the public matcher, invalid IPs can evaluate as a match, causing false positives.
Proposed fix
func (c *Network) Check(event ValuesMap) bool {
@@
case net.IP:
- if !network.Contains(v) {
+ if v == nil || v.To16() == nil || !network.Contains(v) {
return false
}
@@
func containsAny(ips []net.IP, match func(net.IP) bool) bool {
for _, ip := range ips {
+ if ip == nil || ip.To16() == nil {
+ continue
+ }
if match(ip) {
return true
}
}
return false
}Also applies to: 188-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libbeat/conditions/network.go` around lines 166 - 176, The net.IP branch in
the matcher (inside Check) does not guard against nil or zero-length IPs, so
calls like network.Contains(v) can treat invalid IPs as matches; update the
net.IP case to first reject nil or len(v) == 0 and return false, and similarly
ensure containsAny (and containsAnyStr for string slices) skips or rejects
empty/invalid entries in the []net.IP and []string branches before calling
network.Contains (i.e., add a nil/len==0 check for v in the net.IP case and
ensure containsAny handles zero-length net.IP elements).
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Network.CheckcalledextractIPon every event value, unconditionally allocating a[]net.IPslice even for the dominant single-IP case. This adds ~40 B and 2 allocs per op on a hot path.Changes:
extractIP+slices.ContainsFuncwith a type switch inCheckthat fast-pathsstringandnet.IPwithout slice constructioncontainsAny/containsAnyStrhelpers for[]net.IPand[]stringslice casesextractIPfunction andslicesimportBenchmark (10 runs): ~181 → ~139 ns/op, 40 → 16 B/op, 2 → 1 allocs/op.
Original prompt
This section details on the original issue you should resolve
<issue_title>[performance-profiler] Optimize Network.Check single-IP path to reduce allocations and CPU</issue_title>
<issue_description>## Hot Path
libbeat/conditions/network.goin(*Network).Checkcurrently callsextractIPfor every event value and then scans the returned slice.Relevant code paths:
libbeat/conditions/network.go:153-209(Check)libbeat/conditions/network.go:208-225(extractIP)libbeat/conditions/network_test.go:301-323(BenchmarkNetworkCondition)Profiling Data
Before:
Proposed Change
Avoid per-call slice construction in the common single-IP path by handling concrete types directly in
Check:stringandnet.IPwithout building[]net.IP.[]stringand[]net.IP.Representative diff:
Results
After:
Improvement:
Verification
go test ./libbeat/conditions -run '^TestNetwork'string,net.IP,[]string, and[]net.IP; invalid values still fail.Evidence
Commands run:
go test ./libbeat/conditions -run '^$' -bench '^BenchmarkNetworkCondition$' -benchmem -count=10(before)go test ./libbeat/conditions -run '^TestNetwork'go test ./libbeat/conditions -run '^$' -bench '^BenchmarkNetworkCondition$' -benchmem -count=10(after)Duplicate check:
/tmp/previous-findings.json; this hot path is distinct from prior filed items (e.g., dissect suffix allocations, diskqueue decoder buffer reuse, memqueue producer allocations).What is this? | From workflow: Performance Profiler
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
Comments on the Issue (you are @copilot in this section)
@strawgate /ai how frequently is this called? is this a useful optimization? what is it in the hot path for📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.