perf: pre-compute lowercase keys to avoid redundant ToLower calls#1531
perf: pre-compute lowercase keys to avoid redundant ToLower calls#1531
Conversation
|
@copilot comment on this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1531 +/- ##
==========================================
- Coverage 86.68% 86.67% -0.02%
==========================================
Files 179 179
Lines 8790 8812 +22
==========================================
+ Hits 7620 7638 +18
- Misses 911 913 +2
- Partials 259 261 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Optimizes transaction variable exception filtering by precomputing lowercase keys to avoid repeated strings.ToLower calls in hot paths.
Changes:
- Added precomputed lowercase key fields (
MatchData.LowerKey_,ruleVariableException.lowerKeyStr) and populated them during map lookups / exception creation. - Introduced
Map.AddWithLowerKeyand updated header insertion to reuse already-lowercased keys. - Updated
GetFieldexception filtering to use precomputed lowercase values.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/corazawaf/transaction_test.go | Updates test data to include lowerKeyStr for exceptions. |
| internal/corazawaf/transaction.go | Uses precomputed lowercase values and AddWithLowerKey in header paths and exception filtering. |
| internal/corazawaf/rule.go | Adds/stores lowerKeyStr on exceptions at creation time. |
| internal/corazarules/rule_match.go | Adds LowerKey_ to match data to carry precomputed lowercase keys. |
| internal/collections/named.go | Populates LowerKey_ from stored entries when returning matches. |
| internal/collections/map.go | Stores lowerKey per entry and adds AddWithLowerKey; wires LowerKey_ into returned matches. |
Store pre-computed lowercase keys in keyValue.lowerKey and surface them via MatchData.LowerKey_. Pre-compute lowerKeyStr on ruleVariableException at construction time. Add AddWithLowerKey to Map for callers that already have the lowercase key. This eliminates per-match strings.ToLower calls in GetField's exception filtering loop and per-exception ToLower calls, both of which are on the hot path during rule evaluation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jptosso
force-pushed
the
perf/precompute-lower-key
branch
from
96d11b9 to
e022a78
Compare
|
@copilot Can you solve conflicts in this PR? |
…-key # Conflicts: # internal/corazawaf/rule.go # internal/corazawaf/transaction_test.go Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Resolved in d53b2c4. Two conflicts in
|
| // Add adds a new key-value pair to the map. | ||
| func (c *Map) Add(key string, value string) { | ||
| aVal := keyValue{key: key, value: value} | ||
| lk := key | ||
| if !c.isCaseSensitive { | ||
| key = strings.ToLower(key) | ||
| lk = strings.ToLower(key) | ||
| } | ||
| aVal := keyValue{key: key, value: value, lowerKey: lk} | ||
| c.data[lk] = append(c.data[lk], aVal) | ||
| } |
There was a problem hiding this comment.
keyValue.lowerKey is currently set to the storage key (lk), which for case-sensitive maps equals the original key casing. Transaction.GetField now trusts MatchData.LowerKey_ to already be lowercase, so in case-sensitive collections (e.g. ARGS_* with build tag coraza.rule.case_sensitive_args_keys) exception filtering becomes case-sensitive and can fail to match. Consider always storing a lowercase form (e.g. strings.ToLower(key)) in keyValue.lowerKey, while using a separate storageKey (original key vs lowercased key) for the map index.
| // Sets the value of a key with the array of strings passed. If the key already exists, it will be overwritten. | ||
| func (c *Map) Set(key string, values []string) { | ||
| originalKey := key | ||
| lk := key | ||
| if !c.isCaseSensitive { | ||
| key = strings.ToLower(key) | ||
| lk = strings.ToLower(key) | ||
| } | ||
| dataSlice, exists := c.data[key] | ||
| dataSlice, exists := c.data[lk] | ||
| if !exists || cap(dataSlice) < len(values) { | ||
| dataSlice = make([]keyValue, len(values)) | ||
| } else { | ||
| dataSlice = dataSlice[:len(values)] // Reuse existing slice with the same length | ||
| } | ||
| for i, v := range values { | ||
| dataSlice[i] = keyValue{key: originalKey, value: v} | ||
| dataSlice[i] = keyValue{key: originalKey, value: v, lowerKey: lk} | ||
| } | ||
| c.data[key] = dataSlice | ||
| c.data[lk] = dataSlice | ||
| } | ||
|
|
||
| // SetIndex sets the value of a key at the specified index. If the key already exists, it will be overwritten. | ||
| func (c *Map) SetIndex(key string, index int, value string) { | ||
| originalKey := key | ||
| lk := key | ||
| if !c.isCaseSensitive { | ||
| key = strings.ToLower(key) | ||
| lk = strings.ToLower(key) | ||
| } | ||
| values := c.data[key] | ||
| av := keyValue{key: originalKey, value: value} | ||
| values := c.data[lk] | ||
| av := keyValue{key: originalKey, value: value, lowerKey: lk} | ||
|
|
||
| switch { | ||
| case len(values) == 0: | ||
| c.data[key] = []keyValue{av} | ||
| c.data[lk] = []keyValue{av} | ||
| case len(values) <= index: | ||
| c.data[key] = append(c.data[key], av) | ||
| c.data[lk] = append(c.data[lk], av) | ||
| default: | ||
| c.data[key][index] = av | ||
| c.data[lk][index] = av | ||
| } |
There was a problem hiding this comment.
Same issue as Add: Set/SetIndex assign keyValue.lowerKey to lk, which is not lowercased for case-sensitive maps. Since MatchData.LowerKey_ is used as the pre-lowercased key in GetField exception filtering, this can change behavior under case-sensitive collections. Suggest computing lowerKey := strings.ToLower(originalKey) unconditionally for the stored keyValue, and using storageKey (lowerKey vs originalKey) only for indexing into c.data.
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| tx.GetField(rvp) | ||
| } |
There was a problem hiding this comment.
This benchmark includes tx.Close() while the benchmark timer is still running, which can skew ns/op and alloc measurements for GetFieldWithExceptions. Consider calling b.StopTimer() before tx.Close() (and optionally restarting if needed) so the benchmark measures only GetField.
| } | |
| } | |
| b.StopTimer() |
| func TestFindAllPopulatesLowerKey(t *testing.T) { | ||
| m := NewMap(variables.ArgsGet) | ||
| m.Add("Content-Type", "text/html") | ||
|
|
||
| results := m.FindAll() | ||
| if len(results) != 1 { | ||
| t.Fatalf("expected 1 result, got %d", len(results)) | ||
| } | ||
|
|
||
| // Access the MatchData to check LowerKey_ is populated | ||
| md, ok := results[0].(*corazarules.MatchData) | ||
| if !ok { | ||
| t.Fatal("expected *corazarules.MatchData") | ||
| } | ||
| if md.LowerKey_ != "content-type" { | ||
| t.Errorf("expected LowerKey_ 'content-type', got %q", md.LowerKey_) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new LowerKey_ behavior is only asserted for the default (case-insensitive) Map. Since ARGS collections can be case-sensitive under build tag coraza.rule.case_sensitive_args_keys, it would be useful to add a test that FindAll/FindString still populate LowerKey_ with the lowercase form even when the map itself is case-sensitive (to prevent regressions in exception filtering).
|
@jptosso Can you follow on copilot's comments? |
Summary
Eliminates redundant `strings.ToLower` calls from the hot path in `GetField` exception filtering:
Benchmark
```
main:
BenchmarkTxGetField-10 5461011 215.3 ns/op 400 B/op 8 allocs/op
BenchmarkTxGetFieldWithExceptions-10 2476045 467.5 ns/op 704 B/op 14 allocs/op
branch:
BenchmarkTxGetField-10 5714665 211.3 ns/op 448 B/op 8 allocs/op
BenchmarkTxGetFieldWithExceptions-10 5073336 237.9 ns/op 720 B/op 9 allocs/op
```
Test plan