perf(catalog): split exact matches & pattern - #7340
Merged
Mzack9999 merged 4 commits intoApr 29, 2026
Merged
Conversation
Contributor
WalkthroughIntroduces a sync.Once-guarded Filter.Compile() that precomputes exact-match maps and wildcard pattern lists for include/exclude IDs; matching now uses cached helpers. Adds benchmarks exercising compiled matching and updates tests to use the new compiled matching paths. Filter.String() includes exclude-ids when set. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
mikhail5555
commented
Apr 13, 2026
Contributor
Author
There was a problem hiding this comment.
can be removed after review, let me know if you want me to do it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/catalog/index/filter.go (1)
187-189: Address TODO before merging.The comment indicates this deprecated function should be removed before the PR is merged. Ensure this is cleaned up along with its usage in
filter_bench_test.go(theBenchmarkFilterMatches_Mixed_Oldbenchmark).Would you like me to help track this as an issue, or should the old benchmark be retained for future regression comparisons?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/index/filter.go` around lines 187 - 189, Remove the deprecated helper and its benchmark: delete the matchesID function and any references to it, and remove the BenchmarkFilterMatches_Mixed_Old benchmark from filter_bench_test.go; replace callers (if any) with the supported matchesIncludeID implementation or its test/bench variants, and ensure tests/benches compile by updating imports/bench names accordingly so no dead/unused symbols remain (targets: matchesID, matchesIncludeID, BenchmarkFilterMatches_Mixed_Old).pkg/catalog/index/filter_test.go (1)
8-11: Non-standard import grouping.Go convention groups imports as: stdlib, blank line, external packages, blank line, internal packages. Currently
testify/requireis placed between stdlib and internal with an inconsistent blank line.Suggested grouping
import ( "os" "path/filepath" "testing" - "github.com/stretchr/testify/require" - "github.com/projectdiscovery/nuclei/v3/pkg/model/types/severity" "github.com/projectdiscovery/nuclei/v3/pkg/templates/types" + "github.com/stretchr/testify/require" )Or run
go fmt/goimportsto auto-fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/index/filter_test.go` around lines 8 - 11, The import block in pkg/catalog/index/filter_test.go is mis-grouped: move the external package "github.com/stretchr/testify/require" into the external imports group (after the stdlib imports and separated by a blank line) so imports follow the stdlib | external | internal grouping and ensure a blank line between groups; you can also run gofmt/goimports to automatically fix the ordering and spacing in the imports block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/catalog/index/filter_bench_test.go`:
- Around line 40-41: The benchmark comment above BenchmarkFilterMatches_Mixed is
inconsistent (says 7000 while templateCount is 10,000); update the comment to
match the actual templateCount value (or change templateCount to 7000 if
intended) so the description and the variable (templateCount) are aligned—edit
the comment text referring to the benchmark and/or adjust templateCount in the
benchmark setup to reflect the intended corpus size.
In `@pkg/catalog/index/filter.go`:
- Around line 22-23: Fix the typo in the comment for the struct field once in
pkg/catalog/index/filter.go: change "firs ttime" to "first time" so the comment
reads "once ensures that IDs and ExcludedIDs are compiled the first time Matches
is called." and keep the reference to the once sync.Once field unchanged.
- Around line 204-207: The comment on Filter.Compile is misleading about calling
Compile() manually; update the docs to state that Compile() is called
automatically on first Matches() and that Filter (including IDs and ExcludeIDs)
must be treated as immutable once compiled or when used concurrently—do not
suggest callers can safely mutate and re-run Compile concurrently. Specifically
mention Filter.Compile, Filter.Matches, and matchesIncludeID, and replace the
"If IDs or ExcludeIDs are modified after calling Matches, this must be called
manually" guidance with a clear concurrency-safe directive: callers must either
call Compile() before sharing the Filter between goroutines or never modify
IDs/ExcludeIDs after the Filter is in use.
- Around line 212-216: The pattern-detection logic in the filtering code
incorrectly treats patterns with character classes like "[...]" as exact IDs;
update the checks that currently call strings.ContainsAny(idOrPattern, "*?") to
also include '[' so character-class patterns are detected (do this where
includeIDPatterns/ includeIDs are set and likewise for excludeIDPatterns/
excludeIDs), e.g., in the branches that append to f.includeIDPatterns or
f.excludeIDPatterns instead of inserting into f.includeIDs or f.excludeIDs.
---
Nitpick comments:
In `@pkg/catalog/index/filter_test.go`:
- Around line 8-11: The import block in pkg/catalog/index/filter_test.go is
mis-grouped: move the external package "github.com/stretchr/testify/require"
into the external imports group (after the stdlib imports and separated by a
blank line) so imports follow the stdlib | external | internal grouping and
ensure a blank line between groups; you can also run gofmt/goimports to
automatically fix the ordering and spacing in the imports block.
In `@pkg/catalog/index/filter.go`:
- Around line 187-189: Remove the deprecated helper and its benchmark: delete
the matchesID function and any references to it, and remove the
BenchmarkFilterMatches_Mixed_Old benchmark from filter_bench_test.go; replace
callers (if any) with the supported matchesIncludeID implementation or its
test/bench variants, and ensure tests/benches compile by updating imports/bench
names accordingly so no dead/unused symbols remain (targets: matchesID,
matchesIncludeID, BenchmarkFilterMatches_Mixed_Old).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cbfa84b-3c36-403b-98e9-4ae6fbd6a5fd
📒 Files selected for processing (3)
pkg/catalog/index/filter.gopkg/catalog/index/filter_bench_test.gopkg/catalog/index/filter_test.go
… modifying filter was always not-thread safe
Neo - PR Security ReviewCaution Neo couldn't finish analyzing this pull request during this run. Please run the review again. Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/catalog/index/filter.go (1)
187-202: Address the TODO before merging.The comment explicitly states "Remove before PR merged." If this function is only needed for benchmark comparison during review, consider either:
- Removing it now along with the
*_Oldbenchmark variant- If keeping the old benchmark for historical comparison, update the comment to reflect the actual intent (e.g., "Retained for benchmark comparison")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/index/filter.go` around lines 187 - 202, The matchesID function is flagged for removal but still present; either remove matchesID and any associated *_Old benchmark variants (so no deprecated helpers remain), or if you intentionally keep it for benchmark comparison, update the TODO/comment to a clear retention note (e.g., "Retained for benchmark comparison") and mark it with Deprecated and reason; locate the matchesID function and any *_Old benchmark names in the repo and apply the chosen change consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/catalog/index/filter.go`:
- Around line 187-202: The matchesID function is flagged for removal but still
present; either remove matchesID and any associated *_Old benchmark variants (so
no deprecated helpers remain), or if you intentionally keep it for benchmark
comparison, update the TODO/comment to a clear retention note (e.g., "Retained
for benchmark comparison") and mark it with Deprecated and reason; locate the
matchesID function and any *_Old benchmark names in the repo and apply the
chosen change consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5014b7b-0c50-49b0-a625-f187ee2ad91f
📒 Files selected for processing (1)
pkg/catalog/index/filter.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/catalog/index/filter_bench_test.go (1)
39-46: Deduplicate benchmark case tables to avoid drift.The same cases are declared twice. Extracting them once will prevent future divergence between old/new benchmark suites.
♻️ Suggested refactor
+var benchmarkCases = []struct{ exact, wildcards int }{ + {10, 0}, + {100, 0}, + {1000, 0}, + {100, 1}, + {100, 3}, + {100, 5}, +} + func BenchmarkFilterMatches(b *testing.B) { corpus := makeMetadataCorpus(templateCount) - - cases := []struct{ exact, wildcards int }{ - {10, 0}, - {100, 0}, - {1000, 0}, - {100, 1}, - {100, 3}, - {100, 5}, - } - - for _, tc := range cases { + for _, tc := range benchmarkCases { b.Run(fmt.Sprintf("ids=%d,patterns=%d", tc.exact, tc.wildcards), func(b *testing.B) { ... }) } } func BenchmarkFilterMatches_Old(b *testing.B) { corpus := makeMetadataCorpus(templateCount) - - cases := []struct{ exact, wildcards int }{ - {10, 0}, - {100, 0}, - {1000, 0}, - {100, 1}, - {100, 3}, - {100, 5}, - } - - for _, tc := range cases { + for _, tc := range benchmarkCases { b.Run(fmt.Sprintf("ids=%d,patterns=%d", tc.exact, tc.wildcards), func(b *testing.B) { ... }) } }Also applies to: 66-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/index/filter_bench_test.go` around lines 39 - 46, The benchmark table "cases" is duplicated; consolidate it into a single shared declaration and reuse it in both places to avoid drift. Replace the two inline slices with one extracted variable (e.g., var cases = []struct{ exact, wildcards int }{...}) placed at the top of the test or package scope, and update both benchmark usages to reference that single "cases" variable so both suites use the same data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/catalog/index/filter_bench_test.go`:
- Around line 39-46: The benchmark table "cases" is duplicated; consolidate it
into a single shared declaration and reuse it in both places to avoid drift.
Replace the two inline slices with one extracted variable (e.g., var cases =
[]struct{ exact, wildcards int }{...}) placed at the top of the test or package
scope, and update both benchmark usages to reference that single "cases"
variable so both suites use the same data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29d4b527-e289-4dca-856a-265a5b1166e2
📒 Files selected for processing (1)
pkg/catalog/index/filter_bench_test.go
dwisiswant0
changed the title
dwisiswant0
self-requested a review
April 14, 2026 22:59
Member
|
@CodeRabbit ai create an issue for this PR |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 55
👉 #7367 |
Mzack9999
approved these changes
Apr 29, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/catalog/index/filter.go (1)
187-190:⚠️ Potential issue | 🟠 MajorDocument
Filteras immutable once compiled/in use.The current comment still suggests mutating
IDs/ExcludeIDsand rerunningCompile(), but that is unsafe onceMatches()may be running concurrently against the cached maps/pattern slices. Please document the safe contract instead: callCompile()before sharing if you want eager initialization, and treat the filter as immutable after the firstCompile()/Matches().Suggested doc fix
// Compile pre-processes IDs and ExcludeIDs into fast lookup structures. // The first time Matches is called, this is called automatically. -// If IDs or ExcludeIDs are modified after calling Matches, this must be called manually. -// This method is not thread-safe, so make sure noone is using the filter when calling it. +// Call Compile before sharing the filter if you want eager initialization. +// After Compile or the first Matches call, treat IDs and ExcludeIDs as immutable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/catalog/index/filter.go` around lines 187 - 190, Update the comment for Filter to state that once Compile() or Matches() has been called (which initializes internal lookup maps/slices), the Filter must be treated as immutable and must not be mutated (do not change IDs or ExcludeIDs) while Matches() may be called concurrently; also advise callers to call Compile() before sharing the Filter for concurrent use if they want eager initialization rather than relying on the first Matches() to lazily compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/catalog/index/filter.go`:
- Around line 187-190: Update the comment for Filter to state that once
Compile() or Matches() has been called (which initializes internal lookup
maps/slices), the Filter must be treated as immutable and must not be mutated
(do not change IDs or ExcludeIDs) while Matches() may be called concurrently;
also advise callers to call Compile() before sharing the Filter for concurrent
use if they want eager initialization rather than relying on the first Matches()
to lazily compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a735dfb7-0fc9-4ce6-bccf-1d9a3f91ec2d
📒 Files selected for processing (2)
pkg/catalog/index/filter.gopkg/catalog/index/filter_bench_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for change:
Proposed changes
Instead of treating IDs (and ExcludedIDs) as both 'exact' matches and pattern matches, I now pre-compile a list and split them out.
Especially for a high list of 'exact matches' the performance with the current implementation is extremely taxing, for the following reasons:
O(N*M). Given that N is in the order of 10_000, this make it very expensive if you want to run on a small subset of them (say 1_000 IDs).Fixes:
Proof
Especially when using many 'exact' IDs (aka not patterns), the improvement is drastic (3000x).
BenchmarkFilterMatches_Mixed/ids=1000,patterns=0245167 ns/opBenchmarkFilterMatches_Mixed_Old/ids=1000,patterns=0879078188 ns/opChecklist
Disclaimer
(Logic) Code is written by human, benchmark is written with AI assistance.
Summary by CodeRabbit
New Features
Tests