Use ForAttributeWithMetadataName

[TimothyMakkison]

ForAttributeWithMetadataName was added to target types with attached attributes. Should have a performance boost - (not familiar with source generators).

[codecov-commenter]

Codecov Report

Merging #327 (00db61a) into main (dce4b42) will increase coverage by 0.22%.
The diff coverage is 93.67%.

❗ Current head 00db61a differs from pull request most recent head ad36d3c. Consider uploading reports for the commit ad36d3c to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
+ Coverage   91.71%   91.94%   +0.22%     
==========================================
  Files         109      118       +9     
  Lines        3379     3710     +331     
  Branches      438      501      +63     
==========================================
+ Hits         3099     3411     +312     
- Misses        189      205      +16     
- Partials       91       94       +3     

Impacted Files	Coverage Δ
...appings/NewInstanceObjectFactoryPropertyMapping.cs	100.00% <ø> (ø)
...pperly/Descriptors/Mappings/NullDelegateMapping.cs	83.33% <ø> (ø)
...escriptors/Mappings/ObjectPropertyMethodMapping.cs	90.00% <ø> (ø)
...riptors/MappingBuilders/QueryableMappingBuilder.cs	68.42% <68.42%> (ø)
...Descriptors/Mappings/EnumFromStringParseMapping.cs	73.68% <73.68%> (ø)
...criptors/Mappings/PropertyMappings/PropertyPath.cs	89.06% <80.00%> (-3.60%)	⬇️
src/Riok.Mapperly/Helpers/SymbolExtensions.cs	86.56% <81.81%> (+0.85%)	⬆️
...iptors/MappingBuilders/DictionaryMappingBuilder.cs	79.51% <83.33%> (+0.50%)	⬆️
src/Riok.Mapperly/Descriptors/WellKnownTypes.cs	87.23% <83.87%> (-8.92%)	⬇️
...ers/NewInstanceObjectPropertyMappingBodyBuilder.cs	80.43% <91.66%> (+2.83%)	⬆️
... and 32 more

... and 4 files with indirect coverage changes

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

[TimothyMakkison]

Seems like the pipeline can't find GeneratorAttributeSyntaxContext. I successfully ran the .Net Framework 48 integration test so I'm not sure what's failing

[latonz]

Seems like the pipeline can't find GeneratorAttributeSyntaxContext. I successfully ran the .Net Framework 48 integration test so I'm not sure what's failing

This depends on the Roslyn version which is used to run the source generator. AFAIK this depends whether the source generator runs in Visual Sutdio (which uses the Roslyn version which is shipped with VS), via cli (which uses the Roslyn version bundled by de sdk in use), or via other tools. Mapperly already multi targets Roslyn 4.0, 4.4 and 4.5 and provides a constant ROSLYN4_4_OR_GREATER. However, I'm not sure if the added complexity is worth it here, wdyt?

[TimothyMakkison]

This depends on the Roslyn version which is used to run the source generator.

Makes sense forgot about that 😆

I'm not sure if the added complexity is worth it here, wdyt?

I'd hoped this was a quick small change so hadn't given this much thought (haven't benchmarked a SG before). This PR claims that roslyn may do up to x40 times less work with this change. The PR did raise some concerns, although I'm not sure if they apply here.

I'm not familiar with source generators so I can't say how beneficial this change would be, I've updated this PR to use ROSLYN4_4_OR_GREATER, I'll leave the final decision to you. You could revisit this if performance becomes a problem.

[latonz]

I'm happy to accept this change 👍

The biggest performance improvement could be achieved by implementing a "real" incremental source generator, as the current implementation is not really incremental (tracked in #72). I started working in that quite some time ago, but didn't find the time to complete it.

Do you use Mapperly in a project where Mapperly becomes a performance bottleneck?

[TimothyMakkison]

The biggest performance improvement could be achieved by implementing a "real" incremental source generator, as the current implementation is not really incremental (tracked in #72). I started working in that quite some time ago, but didn't find the time to complete it.

My knowledge of roslyn and source generators are limited to partially rewriting codemaid and reading these articles. I'm not sure if its possible to do memoisation when passing Compilation in a pipeline. I wonder how the dotnet team solve this for their source generators.

You could move the distinct call from Execute to the pipeline, assuming it caches it would probably only yield a minor improvement. 🤷

var distincted = mapperClassDeclarations.Collect().Select((myObjects, _) => myObjects.Distinct().ToImmutableArray());
var compilationAndMappers = context.CompilationProvider.Combine(distincted);

Do you use Mapperly in a project where Mapperly becomes a performance bottleneck?

Only in a small personal project, so I wouldn't be aware of issues if they existed. Do you use mapperly at scale at riok?

[latonz]

To address #72 I think the compilation needs to get out of the pipeline as soon as possible, as it changes each time and therefore disables caching. But Mapperly for example uses ClassifyConversion and I currently don't have an idea how to get rid of it. As soon as I find the time I'll look into it 😊

[TimothyMakkison]

For what it's wortht the System.Text.Json does the exact same thing. It looks incredibly difficult to remove compilation while remaining correct. Perhaps a full rewrite could use compilation in the pipeline, get the type symbols, largely building most of the final syntax in type symbol form and then using a custom converter to memoise it. Good luck 😁

I did think that most of the logic could be removed from Execute and turned into a pipeline step. If the resulting CompilationUnitSyntax is the same execute wouldn't run for that class node. I'm not sure how much of an improvement this would be and there would still be the problem of SourceProductionContext. It looks like it's only used for reporting diagnostics so it could be removed from DescriptorBuilder and some some alternative way of passing diagnostics from the pipeline into execute be used.

[latonz]

IMO the filenames of the newly added files could be improved. Suggestions: SyntaxProvider.cs (this file may not be needed anymore...), SyntaxProvider.Roslyn4.0.cs and SyntaxProvider.Roslyn4.4.cs (Mapperly already uses similar naming conventions (eg. Riok.Mapperly.Roslyn4.0.props).

[TimothyMakkison]

Thanks for the help, I'll rebase when the tests pass.

[github-actions]

🎉 This PR is included in version 2.8.0-next.2 🎉

The release is available on:

• GitHub release
• v2.8.0-next.2

Your semantic-release bot 📦🚀