-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RegexpCompileFunc to override regexp.Compile #731
Add RegexpCompileFunc to override regexp.Compile #731
Conversation
Codecov Report
@@ Coverage Diff @@
## main #731 +/- ##
=======================================
Coverage 78.04% 78.04%
=======================================
Files 5 5
Lines 902 902
=======================================
Hits 704 704
Misses 142 142
Partials 56 56
|
@titpetric Thank you for the pull request! We will need a little bit of time to digest this and review it, but it is definitely towards the top of our queue. |
@coreydaley Let me know if there's absolutely anything I can do to improve it. I've considered adding a .SetRegexpCompile(func(...)...) as an API addition, but wen't with the 'less is more' approach. It would be possible to update this in runtime with some mutex protections. That sounds fine until you consider how many extra bugs can rise up when conccurency is at play. Explicit control of the regex compile func behaviour is a single shot - if someone wants to control that during the router lifetime, just implementing the compile func signature enables it. It's expected there would be some sort of smarter eviction implementations behind it. Overthinking it just leads to more unnecessary problems, keeping things simple is hard sometimes. |
@coreydaley This looks good to me, though I'll defer to you for a merge since I know you were looking at it first. While I'm in favor of giving consumers the freedom to optimize their route creation, I believe such an action comes with the implicit disclaimer that if someone is going to play with around with it, they should know what they're doing and are on their own if it breaks something. |
Another note - all of the changes from this PR seem to also be present in #732 |
Correct, regexp is it's own optimization scope (this PR). The second PR needs a rebase when this is merged, that addresses actual memory pressure, while this one just gives us an ability to use a cached/compiled *regexp.Regexp. |
7784a7c
to
9efbf04
Compare
|
What type of PR is this? (check all applicable)
Description
Route creation is slow as it always invokes regexp.Compile, increasing memory and cpu pressure in cases where route creation is common (e.g. an api gateway reloading changes). This PR adds a exported
RegexpCompileFunc
variable, that's possible to override with other implementations, adding caches. Added a benchmark.Related Tickets & Documents
Benchmark: NewRouter
Average runtime per iteration: ~34450 ns/op
Average memory usage per iteration: ~26327 B/op
Average allocations per iteration: ~374 allocs/op
Benchmark: NewRouterRegexpFunc
Average runtime per iteration: ~4184 ns/op (approx. 87.8% improvement)
Average memory usage per iteration: ~2161 B/op (approx. 91.8% reduction)
Average allocations per iteration: ~73 allocs/op (approx. 80.5% reduction)
As per go doc, using *regexp.Regexp concurrently is safe.
Added/updated tests?
have not been included
Run verifications and test
make verify
is passingmake test
is passing