Skip to content

Optimize dissect parser creation#49334

Closed
Copilot wants to merge 5 commits intomainfrom
copilot/optimize-suffix-extraction-allocations
Closed

Optimize dissect parser creation#49334
Copilot wants to merge 5 commits intomainfrom
copilot/optimize-suffix-extraction-allocations

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

extractKeyParts used FindAllStringSubmatch despite the anchored suffixRE only ever producing a single match. This allocated a redundant [][]string wrapper on every call.

Switch to FindStringSubmatch and index directly into the flat []string:

- m := suffixRE.FindAllStringSubmatch(rawKey, -1)
+ m := suffixRE.FindStringSubmatch(rawKey)
  ...
- if m[0][3] != "" { ordinal, _ = strconv.Atoi(m[0][3]) }
+ if m[3] != "" { ordinal, _ = strconv.Atoi(m[3]) }

Benchmark results (BenchmarkDissectNoConversionMultipleValues / WithConversion):

  • ~12% lower ns/op
  • ~25% lower B/op (5778 → 4324)
  • ~11% fewer allocs/op (56 → 50)
Original prompt

This section details on the original issue you should resolve

<issue_title>[performance-profiler] Optimize dissect parser key suffix extraction allocations</issue_title>
<issue_description>## Hot Path
libbeat/processors/dissect/field.go:337-359 (extractKeyParts) currently uses suffixRE.FindAllStringSubmatch(rawKey, -1) even though only one anchored match is consumed.

Profiling Data

Before:

go test ./libbeat/processors/dissect -run '^$' -bench 'BenchmarkDissect(NoConversionMultipleValues|WithConversionMultipleValues)$' -benchmem -count=5

BenchmarkDissectNoConversionMultipleValues-4   93194  12731 ns/op  5776 B/op  56 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   94039  13255 ns/op  5779 B/op  56 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   93270  12850 ns/op  5778 B/op  56 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   93493  12816 ns/op  5778 B/op  56 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   92097  12883 ns/op  5780 B/op  56 allocs/op

BenchmarkDissectWithConversionMultipleValues-4 82034  14878 ns/op  5777 B/op  56 allocs/op
BenchmarkDissectWithConversionMultipleValues-4 81615  14511 ns/op  5781 B/op  56 allocs/op
BenchmarkDissectWithConversionMultipleValues-4 83312  14649 ns/op  5782 B/op  56 allocs/op
BenchmarkDissectWithConversionMultipleValues-4 79563  14699 ns/op  5777 B/op  56 allocs/op
BenchmarkDissectWithConversionMultipleValues-4 69480  16527 ns/op  5775 B/op  56 allocs/op

Proposed Change

Replace the multi-match regex call with a single-match call and index directly:

- m := suffixRE.FindAllStringSubmatch(rawKey, -1)
+ m := suffixRE.FindStringSubmatch(rawKey)
  if len(m) == 0 { ... }
- if m[0][3] != "" { ordinal, _ = strconv.Atoi(m[0][3]) }
+ if m[3] != "" { ordinal, _ = strconv.Atoi(m[3]) }
  ...
- return m[0][1], dataType, ordinal, length, greedy, nil
+ return m[1], dataType, ordinal, length, greedy, nil

File reference: libbeat/processors/dissect/field.go:337-359.

Results

After:

go test ./libbeat/processors/dissect -run '^$' -bench 'BenchmarkDissect(NoConversionMultipleValues|WithConversionMultipleValues)$' -benchmem -count=5

BenchmarkDissectNoConversionMultipleValues-4   102697 11245 ns/op  4324 B/op  50 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   102717 11443 ns/op  4329 B/op  50 allocs/op
BenchmarkDissectNoConversionMultipleValues-4    95758 11668 ns/op  4327 B/op  50 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   104931 11471 ns/op  4327 B/op  50 allocs/op
BenchmarkDissectNoConversionMultipleValues-4   102726 11642 ns/op  4331 B/op  50 allocs/op

BenchmarkDissectWithConversionMultipleValues-4  81697 13341 ns/op  4326 B/op  50 allocs/op
BenchmarkDissectWithConversionMultipleValues-4  88341 13258 ns/op  4323 B/op  50 allocs/op
BenchmarkDissectWithConversionMultipleValues-4  88832 13203 ns/op  4321 B/op  50 allocs/op
BenchmarkDissectWithConversionMultipleValues-4  89328 13416 ns/op  4324 B/op  50 allocs/op
BenchmarkDissectWithConversionMultipleValues-4  83425 13181 ns/op  4322 B/op  50 allocs/op

Improvement:

  • BenchmarkDissectNoConversionMultipleValues: 10.95% faster (avg 12907 -> 11493.8 ns/op)
  • BenchmarkDissectWithConversionMultipleValues: 11.78% faster (avg 15052.8 -> 13279.8 ns/op)
  • Memory: ~25.15% lower B/op (5778 -> 4325)
  • Allocations: 10.71% lower allocs/op (56 -> 50)

Verification

  • Ran relevant package tests: go test ./libbeat/processors/dissect -> ok.
  • Change is behavior-preserving (same regex/groups, only switching from all-matches to single-match for anchored pattern).

Evidence

  • Bench commands and outputs above.
  • Hot-path code location: libbeat/processors/dissect/field.go:337-359.
  • Candidate is distinct from /tmp/previous-findings.json entries (no existing dissect parser suffix extraction issue listed).

What is this? | From workflow: Performance Profiler

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

  • expires on Mar 13, 2026, 2:29 PM UTC

<agent_instructions>Please make a PR and report on any other benchmarks which have improved with this change</agent_instructions>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 6, 2026
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 6, 2026

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @Copilot? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

…indAllStringSubmatch

Co-authored-by: strawgate <6384545+strawgate@users.noreply.github.com>
Copilot AI changed the title [WIP] Optimize dissect parser key suffix extraction allocations Optimize dissect parser key suffix extraction allocations Mar 6, 2026
@strawgate strawgate marked this pull request as ready for review March 6, 2026 23:33
@strawgate strawgate requested a review from a team as a code owner March 6, 2026 23:33
@strawgate strawgate requested review from mauri870 and orestisfl March 6, 2026 23:33
@strawgate
Copy link
Copy Markdown
Contributor

/ai can you run additional benchmarks on this and see if it helps in any other situations?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9a4162c4-03e5-4472-a9d5-4355340d495c

📥 Commits

Reviewing files that changed from the base of the PR and between 785b203 and 3180fef.

📒 Files selected for processing (1)
  • libbeat/processors/dissect/field.go

📝 Walkthrough

Walkthrough

The change removes the exported MarkGreedy() method from the field interface and modifies extractKeyParts to use suffixRE.FindStringSubmatch(rawKey) instead of FindAllStringSubmatch. The regex result is now a 1D slice, so all match index accesses were adjusted (e.g., ordinal, length, greedy flag, and data type use m[3], m[5], m[6], and m[8] respectively; key uses m[1]). Control flow and error handling were updated to reflect the new match structure.

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive PR removes MarkGreedy() method from the exported field interface, which was not part of the linked issue #49319 optimization objective but addresses a staticcheck lint warning about ineffective field assignment. Clarify whether MarkGreedy() removal is a separate fix or was intended as part of this optimization. Consider splitting into separate PR if unrelated to the primary objective.
✅ Passed checks (1 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR replaces FindAllStringSubmatch with FindStringSubmatch in extractKeyParts, adjusting all index references (m[0][n]→m[n]). Performance benchmarks show ~11% faster execution, ~25% lower memory, and ~11% fewer allocations—meeting the linked issue #49319 objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch copilot/optimize-suffix-extraction-allocations
  • 🛠️ Update Documentation: Commit on current branch
  • 🛠️ Update Documentation: Create PR
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Mar 8, 2026
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 8, 2026
@strawgate
Copy link
Copy Markdown
Contributor

/ai please fix libbeat/processors/dissect/field.go:77:2: SA4005: ineffective assignment to field baseField.greedy (staticcheck) make sure all lint and tests are passing

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@cla-checker-service
Copy link
Copy Markdown

cla-checker-service bot commented Mar 8, 2026

💚 CLA has been signed

Copy link
Copy Markdown
Member

@mauri870 mauri870 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from the code (and investigating a previous bug) is that we cannot ever have multiple matches, so in theory the changes here retain the same exact behavior as before. Still, I would appreciate another pair of eyes on this.

@AndersonQ
Copy link
Copy Markdown
Member

My understanding from the code (and investigating a previous bug) is that we cannot ever have multiple matches, so in theory the changes here retain the same exact behavior as before. Still, I would appreciate another pair of eyes on this.

I have the same understanding, but I never touched this processor. Also I see the code changed is covered by tests, so it looks good.

AndersonQ
AndersonQ previously approved these changes Mar 10, 2026
@AndersonQ AndersonQ dismissed their stale review March 10, 2026 13:29

the tests didn't run

@AndersonQ
Copy link
Copy Markdown
Member

/test

@AndersonQ
Copy link
Copy Markdown
Member

My understanding from the code (and investigating a previous bug) is that we cannot ever have multiple matches, so in theory the changes here retain the same exact behavior as before. Still, I would appreciate another pair of eyes on this.

I have the same understanding, but I never touched this processor. Also I see the code changed is covered by tests, so it looks good.

correction. The tests didn't run. Let's see if MarkGreedy() can be safely removed once the tests run

@strawgate strawgate force-pushed the copilot/optimize-suffix-extraction-allocations branch from 785b203 to dfad542 Compare March 14, 2026 00:47
@strawgate
Copy link
Copy Markdown
Contributor

/test

@strawgate
Copy link
Copy Markdown
Contributor

strawgate commented Mar 14, 2026

Parser creation is faster but this isnt actually a hot path for anything other than config unpacking

Benchmark Main ns/op (avg) PR ns/op (avg) ns/op delta Main B/op (avg) PR B/op (avg) B/op delta Main allocs/op (avg) PR allocs/op (avg) allocs/op delta
BenchmarkDissectNoConversionOneValue 3106.0 2843.4 -8.45% 2629.2 2141.2 -18.56% 29.0 27.0 -6.90%
BenchmarkDissectWithConversionOneValue 3211.0 2908.4 -9.42% 2628.2 2140.2 -18.57% 29.0 27.0 -6.90%
BenchmarkDissectNoConversionMultipleValues 9408.0 8523.8 -9.40% 5791.2 4331.8 -25.20% 56.0 50.0 -10.71%
BenchmarkDissectWithConversionMultipleValues 15494.2 9238.4 -40.38% 5785.0 4333.2 -25.10% 56.0 50.0 -10.71%

@strawgate strawgate changed the title Optimize dissect parser key suffix extraction allocations Optimize dissect parser creation Mar 14, 2026
@strawgate strawgate closed this Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[performance-profiler] Optimize dissect parser key suffix extraction allocations

6 participants