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: optimize RestMethodInfo, reduce dictionary allocations and li… #1742

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jun 27, 2024

  • Changed ParameterInfoMap to ParameterInfoArray.
    • Not sure why refit was using a dictionary as an array?
  • Changed Headers, HeaderParameterMap, PropertyParameterMap, QueryParameterMap, AttachmentNameMap to only allocate a dictionary when needed.
    • Sharing a mutable Empty dictionary is a little risky but I dont see why we'd ever want to mutate the collection.
  • Addded EmptyDictionary<TKey, TValue>.get() to create static empty dictionaries. I would prefer to use ImmutableDictionary.Empty and change Dictionary to IReadOnlyDictionary but they aren't available in . Net Framework.
    • This is a little messy but It saves 72 bytes per dictionary instance.
  • bodyParamEnumerable and authorizeParamsEnumerable now use a tuple instead of an anonymous class.
  • Used an integer HeaderCollectionParameterIndex instead of the hashset HeaderCollectionParameterMap
    • Another strange one, refit explicitly prevents adding more than 1 item to the hashset, it seemed a little redundant 😄.

We could probably get away with using arrays instead of Dictionary<int, TSomething> they would use less memory in most cases and would have faster lookups for smaller numbers.

Saves 4Kb of memory, I might be able to do more after profiling.

Original

Method Job InvocationCount UnrollFactor Mean Error StdDev Gen0 Gen1 Allocated
CreateService DefaultJob Default 16 71.07 us 1.320 us 1.412 us 5.1270 0.1221 47.17 KB
ConstantRouteAsync DefaultJob Default 16 56.56 us 0.908 us 0.758 us 5.8594 - 55.27 KB
ComplexRequestAsync DefaultJob Default 16 59.39 us 1.143 us 1.122 us 6.1035 0.1221 57.09 KB
FirstCallConstantRouteAsync Job-NYYUCJ 1 1 32.87 us 0.640 us 0.897 us - - 9.08 KB
FirstCallComplexRequestAsync Job-NYYUCJ 1 1 58.53 us 1.669 us 4.843 us - - 11.9 KB

Changes

Method Job InvocationCount UnrollFactor Mean Error StdDev Median Gen0 Gen1 Allocated
CreateService DefaultJob Default 16 46.24 us 0.828 us 0.692 us 46.30 us 4.6387 0.1221 43 KB
ConstantRouteAsync DefaultJob Default 16 51.42 us 0.282 us 0.236 us 51.41 us 5.5542 0.1221 51.1 KB
ComplexRequestAsync DefaultJob Default 16 55.17 us 0.730 us 0.683 us 55.13 us 5.7373 0.1221 52.94 KB
FirstCallConstantRouteAsync Job-OIQPYS 1 1 36.07 us 1.025 us 2.873 us 35.20 us - - 9.08 KB
FirstCallComplexRequestAsync Job-OIQPYS 1 1 50.36 us 1.357 us 3.738 us 49.30 us - - 11.9 KB

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.91%. Comparing base (6ebeda5) to head (1b8282d).
Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
- Coverage   87.73%   83.91%   -3.82%     
==========================================
  Files          33       36       +3     
  Lines        2348     2444      +96     
  Branches      294      340      +46     
==========================================
- Hits         2060     2051       -9     
- Misses        208      309     +101     
- Partials       80       84       +4     

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

@ChrisPulman ChrisPulman reopened this Jun 29, 2024
@ChrisPulman ChrisPulman merged commit ea1cc52 into reactiveui:main Jun 29, 2024
3 of 4 checks passed
@ChrisPulman
Copy link
Member

Now I see what happened before, as you click merge the browser then scrolls after a short delay making you think your clicking confirm, but the scrolling pushes the Close button into the same place 😖

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 14, 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