Skip to content
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

Optimize/new route regexp allocations #732

Merged

Conversation

titpetric
Copy link
Contributor

@titpetric titpetric commented Aug 24, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This optimizes NewRouter creation. The change introduces strings.Builder for more efficient strings concatenation, and replaces SplitN with Index, avoiding the allocations for the slice.

Related Tickets & Documents

Test Group Condition Average Time (ns/op) Average Memory (B/op) Average Allocations
BenchmarkNewRouter Before 37041 26553 376
After 32046 25800 347
BenchmarkNewRouterRegexpFunc Before 4713 2385 75
After 2792 1640 46

BenchmarkNewRouterRegexpFunc:

Time Improvement: Significant improvement in execution time from 4,713 ns/op to 2,792 ns/op.
Memory Usage: Memory usage per operation dropped from 2,385 bytes to 1,640 bytes.
Allocations: Allocations per operation decreased substantially from 75 to 46.

Added/updated tests?

  • Yes

Run verifications and test

  • make verify is passing
  • make test is passing

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 78.18%. Comparing base (7d9a6f7) to head (f80a252).
Report is 1 commits behind head on main.

❗ Current head f80a252 differs from pull request most recent head b684b38. Consider uploading reports for the commit b684b38 to get more accurate results

Files Patch % Lines
regexp.go 90.62% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
- Coverage   79.58%   78.18%   -1.40%     
==========================================
  Files           5        5              
  Lines         911      903       -8     
==========================================
- Hits          725      706      -19     
- Misses        131      141      +10     
- Partials       55       56       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@titpetric
Copy link
Contributor Author

Rebased off #731

@titpetric titpetric force-pushed the optimize/newRouteRegexp-allocations branch from bd66659 to ed62916 Compare April 2, 2024 07:14
@pull-request-size pull-request-size bot added size/M and removed size/L labels Apr 2, 2024
@titpetric
Copy link
Contributor Author

@AlexVulaj thanks for #731 merge; rebased this one for review. Benchmark still the same as in description.

@titpetric
Copy link
Contributor Author

@coreydaley is there anything i can/should do to get this reviewed?

@apoorvajagtap apoorvajagtap self-assigned this May 6, 2024
regexp.go Outdated Show resolved Hide resolved
@apoorvajagtap
Copy link
Member

Hi @titpetric, thanks for working on this!
Also, thanks for being patient while we were adapting to the changes :). I have looked through & added some inputs, please feel free to share your thoughts.

@titpetric
Copy link
Contributor Author

@apoorvajagtap took your suggestion, added test to cover the expectation of the error return contents, PTAL

@apoorvajagtap
Copy link
Member

The lint & security scan errors are not related to the current changes, so I'll go ahead with squash & merge.

@apoorvajagtap apoorvajagtap merged commit de7178d into gorilla:main May 22, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants