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: remove properties dictionary #1741

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jun 26, 2024

  • Relies on chore: extract methods #1740
  • Options and Properties both internally use dictionaries so propertiesToAdd isn't needed
  • When iterating parameters, refit now checks if PropertyParameterMap contains the key and sets isParameterMappedToRequest to be true.
  • AddPropertiesToRequest iterates the parameter indices, adding them to Options/Properties if they are in AddPropertiesToRequest

Small memory saving of 72 bytes, the size of an empty Dictionary. These benefits are much greater when a property is used.

Original

Method Mean Error StdDev Gen0 Gen1 Allocated
ConstantRouteAsync 2.576 us 0.0511 us 0.0699 us 0.7553 - 6.95 KB
DynamicRouteAsync 3.197 us 0.0410 us 0.0383 us 0.7896 0.0076 7.27 KB
ComplexDynamicRouteAsync 4.635 us 0.0872 us 0.0773 us 0.8545 - 7.9 KB
ObjectRequestAsync 5.250 us 0.0923 us 0.0818 us 0.9460 0.0076 8.76 KB
ComplexRequestAsync 14.130 us 0.2724 us 0.4699 us 1.5869 - 14.8 KB

Changes

Method Mean Error StdDev Median Gen0 Gen1 Allocated
ConstantRouteAsync 3.757 us 0.0736 us 0.0931 us 3.737 us 0.7439 0.0038 6.87 KB
DynamicRouteAsync 4.522 us 0.0899 us 0.1621 us 4.522 us 0.7782 - 7.19 KB
ComplexDynamicRouteAsync 4.083 us 0.0608 us 0.0569 us 4.082 us 0.8469 - 7.82 KB
ObjectRequestAsync 6.217 us 0.3865 us 1.0710 us 5.829 us 0.9384 - 8.68 KB
ComplexRequestAsync 14.342 us 0.2850 us 0.4603 us 14.152 us 1.5869 - 14.72 KB

Copy link

codecov bot commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.89%. Comparing base (6ebeda5) to head (bbaaaa9).
Report is 50 commits behind head on main.

Files Patch % Lines
Refit/RequestBuilderImplementation.cs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
- Coverage   87.73%   83.89%   -3.85%     
==========================================
  Files          33       36       +3     
  Lines        2348     2440      +92     
  Branches      294      329      +35     
==========================================
- Hits         2060     2047      -13     
- Misses        208      309     +101     
- Partials       80       84       +4     

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

@ChrisPulman
Copy link
Member

@TimothyMakkison I guess it's a knock on effect, conflict strikes again

@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jun 26, 2024

Serves me right, I should've been more patient 😅

Had some very broken rebases but the resulting commit looks okay 👍

@ChrisPulman ChrisPulman merged commit f5b1690 into reactiveui:main Jun 27, 2024
1 of 3 checks 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 Jul 12, 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