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: use private static fields to store constant typeParameters where possible #1606

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Nov 16, 2023

Modify source generator to use private static readonly fields to store the typeParameter arrays, saving the allocations for each request.

  • Only generates when there are actual arguments, otherwise Array.Empty is used - feat: generate code that uses Array.Empty where possible #1599
  • private static fields are generated beside its respective method, this could be moved to the start of the class if requested.
  • I've used a HashSet of each member in scope to accidental name collisions. Fields will have names like _typeParameters3 incrementing for each field generated.
  • Uses nullable coalescing assignment if one of the method parameters uses type arguments. This is done as we may not be able to access it from outside the method scope making the array initialization invalid.
  • It should be safe to initialize and set the static arrays in multithreaded contexts as setting references are thread safe in C#.

Edit: typeof(T) will be different for each usage of T so it isn't possible to have a reusable _genericType array. This can also cause issues for _typeParameter. GenerateTypeParameterExpression ensures that each parameter doesn't contain a generic type parameter, falling back to the current solution if it detects one.

  • Tests for the changes have been added (for bug fixes / features)

See #1598

@TimothyMakkison TimothyMakkison marked this pull request as draft November 16, 2023 21:54
@TimothyMakkison TimothyMakkison changed the title feat: use private static fields to store constant typeParameters where possible feat: use private static fields to store constant typeParameters & genericTypes where possible Nov 16, 2023
@TimothyMakkison TimothyMakkison changed the title feat: use private static fields to store constant typeParameters & genericTypes where possible feat: use private static fields to store constant typeParameters where possible Nov 16, 2023
@TimothyMakkison TimothyMakkison marked this pull request as ready for review November 16, 2023 23:13
@TimothyMakkison TimothyMakkison force-pushed the type_array branch 3 times, most recently from c5b222b to 680f3b9 Compare November 17, 2023 14:45
@anaisbetts
Copy link
Member

Seems legit, @clairernovotny?

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (52151a2) 87.81% compared to head (a54d24e) 87.81%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1606   +/-   ##
=======================================
  Coverage   87.81%   87.81%           
=======================================
  Files          33       33           
  Lines        2347     2347           
  Branches      294      294           
=======================================
  Hits         2061     2061           
  Misses        207      207           
  Partials       79       79           

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

@TimothyMakkison TimothyMakkison force-pushed the type_array branch 3 times, most recently from 1e659da to 33f669b Compare November 21, 2023 19:25
@TimothyMakkison TimothyMakkison force-pushed the type_array branch 3 times, most recently from 8173af0 to d912119 Compare November 27, 2023 23:04
@TimothyMakkison TimothyMakkison marked this pull request as draft November 27, 2023 23:04
@TimothyMakkison TimothyMakkison marked this pull request as ready for review November 27, 2023 23:36
@TimothyMakkison TimothyMakkison marked this pull request as draft November 27, 2023 23:43
@TimothyMakkison TimothyMakkison marked this pull request as ready for review November 28, 2023 18:36
@anaisbetts
Copy link
Member

@ChrisPulman Any objections to this PR?

@ChrisPulman
Copy link
Member

@ChrisPulman Any objections to this PR?

I am currently in India, but I will check it this evening and merge it if it's fitting to our requirements, thank you.

@ChrisPulman
Copy link
Member

All looks good, thank you Tim

@ChrisPulman ChrisPulman merged commit 4055e7a into reactiveui:main Dec 2, 2023
3 checks passed
@TimothyMakkison TimothyMakkison deleted the type_array branch December 2, 2023 15:22
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 Dec 17, 2023
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.

3 participants