Cache transformations within a phase #537
Conversation
| @@ -154,3 +159,15 @@ func NewRuleGroup() RuleGroup { | |||
| rules: []*Rule{}, | |||
| } | |||
| } | |||
|
|
|||
| type transformationKey struct { | |||
There was a problem hiding this comment.
@M4tteoP The main difference with your initial approach is using a struct instead of string for the key, which I failed to communicate well offline :) There is significant cost including allocation to concatenate strings, while filling a struct is much faster and all on the stack so no allocation
There was a problem hiding this comment.
Rag, Many thanks for taking over it while explaining how you did it! Very appreciated.
| @@ -104,6 +104,8 @@ type Transaction struct { | |||
| audit bool | |||
|
|
|||
| variables TransactionVariables | |||
|
|
|||
| transformationCache map[transformationKey]transformationValue | |||
There was a problem hiding this comment.
@M4tteoP Though besides that reusing maps is also important, adding to the transaction which is already pooled made it pretty simple
| @@ -109,7 +110,41 @@ func BenchmarkCRSSimplePOST(b *testing.B) { | |||
| tx.AddRequestHeader("Accept", "application/json") | |||
| tx.AddRequestHeader("Content-Type", "application/x-www-form-urlencoded") | |||
| tx.ProcessRequestHeaders() | |||
| if _, err := tx.ResponseBodyWriter().Write([]byte("parameters2=and&other2=Stuff")); err != nil { | |||
There was a problem hiding this comment.
If anyone was trying to use this benchmark and confused like I was, it's because of the typo here preventing post body processing from happening at all (response body processing is disabled by default I guess)
| @@ -280,8 +315,30 @@ func crsWAF(t testing.TB) coraza.WAF { | |||
| if err != nil { | |||
| t.Fatal(err) | |||
| } | |||
| customTestingConfig := ` | |||
There was a problem hiding this comment.
I added this so the benchmark runs the same rules as FTW (mostly for paranoia level), I'm not sure if there is a practical difference though
Codecov ReportBase: 78.28% // Head: 78.41% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v3/dev #537 +/- ##
==========================================
+ Coverage 78.28% 78.41% +0.12%
==========================================
Files 145 145
Lines 6884 6925 +41
==========================================
+ Hits 5389 5430 +41
Misses 1263 1263
Partials 232 232
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -155,11 +158,11 @@ func (r *Rule) Status() int { | |||
| // Evaluate will evaluate the current rule for the indicated transaction | |||
| // If the operator matches, actions will be evaluated, and it will return | |||
| // the matched variables, keys and values (MatchData) | |||
| func (r *Rule) Evaluate(tx rules.TransactionState) []types.MatchData { | |||
| return r.doEvaluate(tx.(*Transaction)) | |||
| func (r *Rule) Evaluate(tx rules.TransactionState, cache map[transformationKey]transformationValue) []types.MatchData { | |||
internal/corazawaf/rule.go
Outdated
| ars, es := r.executeTransformations(arg.Value()) | ||
| args = []string{ars} | ||
| errs = es | ||
| switch { |
There was a problem hiding this comment.
I think 'doEvaluate' method is complex enough already so maybe we could extract this in another method?
This reverts commit d2a0e8e.
We currently compute the same transformation per variable many many times per phase. This adds a cache to reduce this repetitive computation. It notably reduces allocations by half in the CRS benchmarks, which has some performance impact here and will have bigger performance impact where GC is slower like wasm.
After
Before