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

feat: calculate path substitutions in RestMethodInfo #1897

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Oct 29, 2024

Parse the url target inside RestMethodInfo, removing the use of regex on function calls.

Re attempt at #1730. Turns out the additional allocations were caused by line 829 creating a closure. Looking at the lowered code C# always created the linq closure, even when round tripping was never used 🤔

In future I could optimise round trip to not use string.Split, either manually splitting or using Span.Split

Benchmarks (on battery power)

Method Mean Error StdDev Median Gen0 Allocated
ConstantRouteAsync 9.332 us 0.6444 us 1.8999 us 9.479 us 0.2899 2.71 KB
DynamicRouteAsync 5.284 us 0.2704 us 0.7449 us 4.957 us 0.3052 3 KB
ComplexDynamicRouteAsync 5.091 us 0.4201 us 1.2387 us 4.433 us 0.3662 3.41 KB
ObjectRequestAsync 5.942 us 0.1163 us 0.2504 us 5.834 us 0.4730 4.41 KB
ComplexRequestAsync 18.005 us 0.4408 us 1.2359 us 17.392 us 1.1292 10.44 KB

Old benchmarks

Method Mean Error StdDev Gen0 Allocated
ConstantRouteAsync 2.072 us 0.0110 us 0.0097 us 0.2937 2.71 KB
DynamicRouteAsync 2.783 us 0.0259 us 0.0202 us 0.3319 3.07 KB
ComplexDynamicRouteAsync 4.060 us 0.0226 us 0.0200 us 0.4044 3.78 KB
ObjectRequestAsync 4.954 us 0.0295 us 0.0262 us 0.4807 4.48 KB
ComplexRequestAsync 14.455 us 0.0686 us 0.0642 us 1.1597 10.67 KB

@TimothyMakkison TimothyMakkison marked this pull request as draft October 29, 2024 22:17
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 94.52055% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.89%. Comparing base (6ebeda5) to head (288ad31).
Report is 156 commits behind head on main.

Files with missing lines Patch % Lines
Refit/RestMethodInfo.cs 94.11% 3 Missing and 2 partials ⚠️
Refit/RequestBuilderImplementation.cs 95.08% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1897      +/-   ##
==========================================
- Coverage   87.73%   84.89%   -2.85%     
==========================================
  Files          33       36       +3     
  Lines        2348     2502     +154     
  Branches      294      361      +67     
==========================================
+ Hits         2060     2124      +64     
- Misses        208      299      +91     
+ Partials       80       79       -1     

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

@TimothyMakkison TimothyMakkison marked this pull request as ready for review October 31, 2024 15:47
@ChrisPulman ChrisPulman enabled auto-merge (squash) November 3, 2024 03:57
@ChrisPulman ChrisPulman merged commit fa3a57b into reactiveui:main Nov 3, 2024
1 check passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants