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: WIP optimize url creation #1730

Closed
wants to merge 1 commit into from

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jun 22, 2024

WIP; Uses the first call to determine the substitutions for the URL.

  • This might be a "breaking change" as the url conversion is done later on and in bulk, in addition to calling UrlParameterFormatter.Format on the same value if it appears several times in the URL.
    • If this is an issue I can use a ValueListBuilder to preserve the order of operations and to cache values,
  • ParameterFragment is my messy attempt to create a tagged union.
    • Instead of having a string Value property it could be an index and count representing the reevant area in the original url.
  • Uses ValueStringBuilder I'll need to rebase this after feat: use ValueStringBuilder adding the query parameters #1719 is merged.
  • This would ideally be done in the source generator.
  • Could do with some benchmarks, especially for startup times.

@ChrisPulman ChrisPulman marked this pull request as draft June 22, 2024 22:15
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 91.72414% with 12 lines in your changes missing coverage. Please review.

Project coverage is 83.54%. Comparing base (6ebeda5) to head (94bf0a5).
Report is 59 commits behind head on main.

Files Patch % Lines
Refit/RestMethodInfo.cs 94.11% 3 Missing and 2 partials ⚠️
Refit/RequestBuilderImplementation.cs 92.98% 2 Missing and 2 partials ⚠️
Refit/UnreachableException.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1730      +/-   ##
==========================================
- Coverage   87.73%   83.54%   -4.20%     
==========================================
  Files          33       37       +4     
  Lines        2348     2455     +107     
  Branches      294      354      +60     
==========================================
- Hits         2060     2051       -9     
- Misses        208      319     +111     
- Partials       80       85       +5     

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

@TimothyMakkison TimothyMakkison changed the title feat: optimize url creation feat: WIP optimize url creation Jun 22, 2024
@TimothyMakkison
Copy link
Contributor Author

Rebased and done some modifications. Added Refit.UnreachableException for cases that should not be able to happen.

Not sure what I'm missing but bizarrely the benchmarks don't show a major change in memory usage. I'll review this a different day but I might abandon this and reuse the logic for the source generator.

Before

Method Mean Error StdDev Gen0 Gen1 Allocated
ConstantRouteAsync 2.045 us 0.0395 us 0.0638 us 0.6828 0.0114 6.28 KB
DynamicRouteAsync 2.601 us 0.0518 us 0.0532 us 0.7172 0.0038 6.6 KB
ComplexDynamicRouteAsync 3.799 us 0.0651 us 0.0891 us 0.7858 - 7.24 KB
ObjectRequestAsync 4.560 us 0.0886 us 0.1021 us 0.8698 - 8.05 KB
ComplexRequestAsync 12.951 us 0.2575 us 0.4643 us 1.5259 - 14.19 KB

Changes

Method Mean Error StdDev Median Gen0 Gen1 Allocated
ConstantRouteAsync 3.037 us 0.0602 us 0.1174 us 3.028 us 0.6828 - 6.3 KB
DynamicRouteAsync 3.195 us 0.2061 us 0.6076 us 3.410 us 0.7172 0.0076 6.63 KB
ComplexDynamicRouteAsync 3.054 us 0.0496 us 0.0464 us 3.033 us 0.7858 0.0076 7.23 KB
ObjectRequestAsync 4.194 us 0.0613 us 0.0512 us 4.180 us 0.8698 - 8.04 KB
ComplexRequestAsync 11.623 us 0.1973 us 0.1749 us 11.590 us 1.5259 0.0153 14.16 KB

Copy link

github-actions bot commented Aug 5, 2024

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 Aug 5, 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.

1 participant