Simplify DynamicData reflection lookups using FlattenHierarchy#7535
Simplify DynamicData reflection lookups using FlattenHierarchy#7535Evangelink merged 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves DynamicData reflection performance in MSTest’s DynamicDataOperations by caching inheritance-walking member lookups (fields/properties/methods) so repeated calls don’t traverse the full type hierarchy every time.
Changes:
- Added per-member-kind
ConcurrentDictionarycaches keyed by(Type, memberName)(via a customTypeMemberKeystruct). - Refactored the existing hierarchy-walk implementations into
Lookup*InHierarchyhelpers and routedGet*ConsideringInheritancethrough the caches.
You can also share your feedback on Copilot code review. Take the survey.
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
- Have we seen real perf issues here that warrants adding a cache?
- I feel like walking the inheritance hierarchy is already not the right approach. Instead, we should avoid
BindingFlags.DeclaredOnlyand let the .NET runtime do its thing. The .NET reflection implementation has some caching already, so if we are able to avoid walking the hierarchy ourselves and let the caching behavior of reflection do its job, I think that's the best.
…l hierarchy walk Replace the manual type hierarchy walk with DeclaredOnly + loop with a single reflection call using BindingFlags.FlattenHierarchy. This lets the .NET runtime handle inheritance search for static members with its own built-in caching, removing the need for custom ConcurrentDictionary caches and the TypeMemberKey struct.
|
@Youssef1313 Great suggestion — replaced the manual hierarchy walk + caching with a single reflection call using |
Evangelink
force-pushed
the
perf/cache-reflection-in-dynamic-data-operations
branch
from
96f1ad1 to
28dac23
Compare
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
LGTM, with two small comments.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
…-in-dynamic-data-operations
…to disallow private base members - Remove trailing blank line in DynamicDataOperations.cs (Thread 5) - Update DynamicDataShouldBeValidAnalyzer.TryGetMemberCore to filter out private members from base types, matching the FlattenHierarchy runtime behavior (Thread 4) - Add test WhenPrivateMemberIsFromBase_Diagnostic to cover the new behavior
Evangelink
changed the title
Replace the manual
DeclaredOnly+BaseTypewalk inDynamicDataOperationswith direct reflection lookups usingBindingFlags.FlattenHierarchy, and inline the helper methods at their call sites.Behavior change:
FlattenHierarchydoes not return private static members from base types, while the previous loop-based approach did. This is accepted as a reasonable tradeoff — accessing private members of a base class via[DynamicData]is an unusual pattern.The
DynamicDataShouldBeValidAnalyzerhas been updated to match:TryGetMemberCorenow filters out private members when looking at base types.