cache Rule ID string version #1039
Conversation
Remove `strconv.Itoa(rid)` from hot path by caching its result
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1039 +/- ##
==========================================
+ Coverage 82.72% 83.07% +0.35%
==========================================
Files 162 162
Lines 9080 7611 -1469
==========================================
- Hits 7511 6323 -1188
+ Misses 1319 1037 -282
- Partials 250 251 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -76,3 +84,14 @@ func (r *RuleMetadata) Raw() string { | |||
| func (r *RuleMetadata) SecMark() string { | |||
| return r.SecMark_ | |||
| } | |||
|
|
|||
| func (r *RuleMetadata) StrID() string { | |||
| if r.cachedStrID_ == "" { | |||
There was a problem hiding this comment.
@jcchavezs is empty comparison as fast as a nil pointer comparison?
There was a problem hiding this comment.
Yeah, they both just compare a value to 0
|
Thank you very much for your contribution, looks great. |
| @@ -76,3 +84,14 @@ func (r *RuleMetadata) Raw() string { | |||
| func (r *RuleMetadata) SecMark() string { | |||
| return r.SecMark_ | |||
| } | |||
|
|
|||
| func (r *RuleMetadata) StrID() string { | |||
| if r.cachedStrID_ == "" { | |||
There was a problem hiding this comment.
There aren't that many rules and I think this is used fairly commonly. Should we just populate it eagerly without the lazy pattern?
There was a problem hiding this comment.
This would be the best solution, however it seems there is no init function for those fields. We will need to introduce a New function & use that in a few places to get this done properly (the tricky bit being determining if the rule is part of a chain or not, in which case we take parent ID)
There was a problem hiding this comment.
Yeah it's a bigger change - I think it's fine to do but if it's too much for now, we can merge as is too
There was a problem hiding this comment.
In the end using lazy pattern leads to a race condition #1083
| @@ -76,3 +84,14 @@ func (r *RuleMetadata) Raw() string { | |||
| func (r *RuleMetadata) SecMark() string { | |||
| return r.SecMark_ | |||
| } | |||
|
|
|||
| func (r *RuleMetadata) StrID() string { | |||
| if r.cachedStrID_ == "" { | |||
There was a problem hiding this comment.
Yeah, they both just compare a value to 0
|
Could you address the feedback @noboruma? |
While testing the library at 1K RPS,
pprofshowed that thisstrconv.Itoaalone is adding a ~2.2% CPU usage.Since this ID is per rule and does not change over time, we can compute it once, cache it and reuse.