perf: TestExtensions eliminate closure with equality#4350
perf: TestExtensions eliminate closure with equality#4350thomhurst merged 1 commit intothomhurst:mainfrom
TestExtensions eliminate closure with equality#4350Conversation
b90c565 to
7ebd1c1
Compare
SummaryEliminates closure allocations in Critical IssuesNone found ✅ SuggestionsMemory Retention ConsiderationThe PR changes the cache key from Potential impact:
Questions:
Looking at Alternative Approach (Optional)If memory retention becomes an issue, you could achieve the same allocation elimination while keeping strings as keys: private static CachedTestNodeProperties GetOrCreateCachedProperties(TestContext testContext)
{
var testId = testContext.TestDetails.TestId;
return TestNodePropertiesCache.GetOrAdd(
testId,
static (_, context) => CreateProperties(context),
testContext // passed as state parameter
);
}
private static CachedTestNodeProperties CreateProperties(TestContext testContext)
{
// ... existing implementation
}This uses the Verdict✅ APPROVE - The optimization is sound and correctly eliminates allocations. The memory retention concern is minor and likely acceptable given the cache is cleared during cleanup. The performance improvement shown in the screenshots is valuable. |
Damn, hadn't thought of that good catch. I don't think
Didn't think this was available for .NET standard 2.0 but it looks like the polyfill has added it, I'll update my PR |
7ebd1c1 to
f89a376
Compare
|
Updated this PR Just noticed that my original changes got the |
f89a376 to
9924eff
Compare
SummaryThis PR eliminates a closure allocation in the hot path of test node property caching by using the Critical IssuesNone found ✅ SuggestionsNone - this is a clean performance optimization that:
The PR description mentions a custom Verdict✅ APPROVE - No critical issues. Clean performance improvement in a hot path. |
Each time
GetOrCreateCachedPropertiesis called aFunc<string, CachedTestNodeProperties>and display class would be created forGetOrAddfunction.I changed
TestNodePropertiesCachetoConcurrentDictionary<TestContext, CachedTestNodeProperties>and used a customIEqualityComparerto useTestIdas the actual key. This way thetestContextwould be passed to thevalueFactoryand no closure would be created.Before
After
This trick can probably be used in a lot of other places