contrib/google.golang.org/api: lazy init regexp#2197
Conversation
BenchmarksBenchmark execution time: 2023-09-07 10:35:48 Comparing candidate commit 4101e36 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics. |
| pathMatcher, err := regexp.Compile(e.PathRegex) | ||
| if err != nil { | ||
| log.Warn("contrib/google.golang.org/api: failed to create regex: %s: %v", e.PathRegex, err) | ||
| return Endpoint{}, false |
There was a problem hiding this comment.
I think in this case it's preferable to skip this endpoint and keep trying with the rest of the endpoints instead of directly returning false.
Also, we can add a very simple unit test that checks the existing regexes compile properly, so we make sure this situation never happens at runtime.
There was a problem hiding this comment.
I was just hacking this together quickly 😅. Testing all the regex with a unit test and then assuming they won't fail (or trying the next one) sounds good to me.
rarguelloF
changed the title
ajgajg1134
left a comment
ajgajg1134
left a comment
There was a problem hiding this comment.
Looks good! before merging can you include a benchstat comparison for the new benchmarks to show the improved memory?
There was a problem hiding this comment.
LGTM. benchstat output for future reference:
goos: darwin
goarch: arm64
pkg: gopkg.in/DataDog/dd-trace-go.v1/contrib/google.golang.org/api
│ a.txt │ b.txt │
│ sec/op │ sec/op vs base │
InitApiEndpointsTree-10 239.89m ± ∞ ¹ 56.65m ± ∞ ¹ -76.38% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95
│ a.txt │ b.txt │
│ B/op │ B/op vs base │
InitApiEndpointsTree-10 378.25Mi ± ∞ ¹ 27.31Mi ± ∞ ¹ -92.78% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95
│ a.txt │ b.txt │
│ allocs/op │ allocs/op vs base │
InitApiEndpointsTree-10 2918.0k ± ∞ ¹ 177.6k ± ∞ ¹ -93.91% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95
3 participants
What does this PR do?
Make regexp initialization lazy. Significantly reduce memory usage caused by importing this package.
Note: There is probably more room for improvement here:
gen_endpoints.jsoncould be gzipped which would reduce binary sizes by ~8MB (in exchange for a small startup cost of decompression) (this is probably offset by saving the regexp compilation startup cost eliminated in this PR)Motivation
Problem reported by user in public slack.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!