Performance improvements: AttributeDataAccessor #2020
Conversation
…ibuteDataAccessor`
latonz
left a comment
latonz
left a comment
There was a problem hiding this comment.
Thank you for this contribution. I am still unsure whether it is worth adopting. I like the idea of moving away from reflection, but I am not yet fully convinced. What are your general thoughts on this?
| Job=InProcess Toolchain=InProcessEmitToolchain | ||
| ``` | ||
| | Method | Mean | Error | StdDev | Gen0 | Gen1 | Allocated | |
There was a problem hiding this comment.
IMO we shouldn't checkin the generated results of the benchmarks...
There was a problem hiding this comment.
I checked in this only so I can show you my results. When I open this PR, I will remove this.
For now, I'm interested in whether you agree with this approach, considering the performance gains.
| public class CachedAttributeDataAccessor(IAttributeDataAccessor attributeDataAccessor) : IAttributeDataAccessor | ||
| { | ||
| private readonly Dictionary<CacheKey, object?> _cache = new(); | ||
| private readonly IAttributeDataAccessor _attributeDataAccessor = attributeDataAccessor; |
There was a problem hiding this comment.
This can be replaced with the primary ctor field, no?
There was a problem hiding this comment.
Done. BTW: Do you prefer using the primary ctor fields directly? I always disliked that it is not read-only.
There was a problem hiding this comment.
IMO it has pros and cons. Often it saves quite a bit of boilerplate, but IMO it results in unnecessary diffs when there is a need to switch back to a regular ctor 🤷
|
|
||
| namespace Riok.Mapperly.Configuration; | ||
|
|
||
| public class CachedAttributeDataAccessor(IAttributeDataAccessor attributeDataAccessor) : IAttributeDataAccessor |
There was a problem hiding this comment.
To what extent do the performance gains result from caching?
There was a problem hiding this comment.
After the AttributeDataAccessor, it is smaller. As mentioned, in some cases, it is pointless; in other cases, it provides benefits.
For me, the large compile had these results:
- Baseline: 93.267 ms and 21185.55 KB
- Without caching: 85.923 ms and 20239.85 KB
- With caching: 82.853 ms and 19992.74 KB
| private static TValue? GetSimpleValue<TValue>(AttributeData attrData, string propertyName) | ||
| where TValue : struct | ||
| { | ||
| if (TryGetTypedConstant(attrData, propertyName, out var typedConstant, out _)) |
There was a problem hiding this comment.
Invert the if and return early.
There was a problem hiding this comment.
I used the conditional operator where it seemed to fit, and early return where it wasn't.
|
Hi @faddiv , Thank you for your effort on this and for providing the detailed performance benchmarks. Thanks again for your contribution! |
2 participants
Performance improvements: AttributeDataAccessor
Description
This PR showcases the performance improvements I had in mind:
The idea was to improve the AttributeDataAccessor since that is one of the most used parts of the code.
I replaced the reflection with a more direct approach. I also used caching on them to save some more work.
I also created a comparison benchmark for each step, so it can be compared to see how much benefit it can give.
My comments about the changes:
ToList-ed. I didn't touch it for consistency.SymbolAccessordoesn't need to be passed around and checked for null. I think this makes this class cleaner.GetBestTypeByMetadataNameinWellKnownTypeswon't find some types if it is not used in the code. It can occur when some tests are run alone. This could cause a problem, and I would look for some way to implement this differently. Or let's useTryGetAttributesinstead.TryGetAttributesat other places, but it caused quite a lot of failures.NotNullIfNotNullConfiguration, since theNotNullIfNotNullAttributeis internal, but theAttributeDataAccessoris public.<ReportAnalyzer>true</ReportAnalyzer>so I can observe how it is changed in practice. This reflected the gain in the benchmark.Fixes # (issue)
Checklist