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

Use string for methods instead of IMethodSymbol #549

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

TimothyMakkison
Copy link
Collaborator

@TimothyMakkison TimothyMakkison commented Jul 8, 2023

Use string for methods instead of IMethodSymbol

Description

Inspired by Sytem.Text.Json to use the known string of a method instead of looking it up via the compilation.

  • CollectionInfoBuilder - used SpecialType.System_Int32 instead of types.Get<int>()
  • EnumerableMappingBuilder - use method string instead of retrieving the type and looking for a matching method, this has been done for ImmutableCollections, Collect methods and Select
  • DictionaryMappingBuilder
    • Use switch expression in ResolveImmutableCollectMethod to use CollectionType
    • Use method string instead of retrieving the type and looking for a matching method.
    • Fix typo LinqDicitonaryMapping

Related to #530

Note that I've used a branch where #537 and #542 are added. This shouldn't be merged until after they are added.
By using a known method string it might be possible to remove the qualified type signature and use a using statement. See #507

Benchmarks

Saves 0.5MB with a small 5% speed boost

Original

Method Mean Error StdDev Gen0 Gen1 Allocated
Compile 1.653 ms 0.0307 ms 0.0301 ms 93.7500 23.4375 180.8 KB
LargeCompile 58.961 ms 0.9728 ms 1.6253 ms 1571.4286 571.4286 9712.91 KB

Changed

Method Mean Error StdDev Gen0 Gen1 Allocated
Compile 1.618 ms 0.0317 ms 0.0412 ms 93.7500 23.4375 174.31 KB
LargeCompile 55.762 ms 0.8799 ms 1.2620 ms 1500.0000 500.0000 9284.46 KB

Checklist

  • The existing code style is followed
  • The commit message follows our guidelines
  • Performed a self-review of my code
  • Hard-to-understand areas of my code are commented
  • The documentation is updated (as applicable)
  • Unit tests are added/updated
  • Integration tests are added/updated (as applicable, especially if feature/bug depends on roslyn or framework version in use)

@TimothyMakkison TimothyMakkison changed the title Use string methods IMethodSymbol Use string for methods instead of IMethodSymbol Jul 8, 2023
@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Merging #549 (7e33cc1) into main (85347e4) will decrease coverage by 0.30%.
The diff coverage is 91.13%.

❗ Current head 7e33cc1 differs from pull request most recent head 24ae157. Consider uploading reports for the commit 24ae157 to get more accurate results

@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
- Coverage   91.96%   91.67%   -0.30%     
==========================================
  Files         147      158      +11     
  Lines        4656     5417     +761     
  Branches      588      687      +99     
==========================================
+ Hits         4282     4966     +684     
- Misses        254      305      +51     
- Partials      120      146      +26     
Impacted Files Coverage Δ
...numerables/EnsureCapacity/EnsureCapacityBuilder.cs 45.94% <42.10%> (-32.32%) ⬇️
...ok.Mapperly/Configuration/AttributeDataAccessor.cs 68.88% <64.58%> (-7.93%) ⬇️
...lders/NewInstanceObjectMemberMappingBodyBuilder.cs 79.58% <78.94%> (+0.79%) ⬆️
.../Descriptors/MappingBuilders/SpanMappingBuilder.cs 80.00% <80.00%> (ø)
src/Riok.Mapperly/Helpers/SymbolExtensions.cs 86.02% <83.33%> (-5.29%) ⬇️
...scriptors/InlineExpressionMappingBuilderContext.cs 95.91% <85.71%> (+0.79%) ⬆️
...Mapperly/Descriptors/UserMethodMappingExtractor.cs 93.77% <85.71%> (+0.08%) ⬆️
...escriptors/MappingBuilders/MemoryMappingBuilder.cs 88.46% <88.46%> (ø)
...rc/Riok.Mapperly/Configuration/StringMemberPath.cs 88.88% <88.88%> (ø)
src/Riok.Mapperly/Descriptors/WellKnownTypes.cs 92.00% <88.88%> (+3.11%) ⬆️
... and 47 more

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TimothyMakkison TimothyMakkison force-pushed the remove_Imethod branch 4 times, most recently from 7e33cc1 to fe4e1a9 Compare July 11, 2023 08:13
Copy link
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 😊 Ready to merge after a rebase.

@latonz latonz merged commit 7f717dd into riok:main Jul 11, 2023
12 checks passed
@github-actions
Copy link

🎉 This PR is included in version 2.9.0-next.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@TimothyMakkison TimothyMakkison deleted the remove_Imethod branch August 2, 2023 10:55
@github-actions
Copy link

github-actions bot commented Aug 7, 2023

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants