Skip to content

Conversation

@TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jan 13, 2026

  • Make async methods return the Task
  • Fix issue where DataTypesProduced would create a new array each time it was called
    • I assume this wasn't intentional and not a hidden part of IDataProducer

Again returning ValueTask here makes the callers state machine larger. I don't think non-generic ValueTask are useful here

Before

image

After

image

@thomhurst
Copy link
Owner

Summary

Performance optimization that caches the DataTypesProduced array and wraps PublishAsync calls in ValueTask to avoid async state machine overhead.

Critical Issues

None found ✅

Suggestions

Consider consistency in early return patterns:
The changes maintain the existing pattern of early returns before wrapping in ValueTask (lines 26-29 and 47-50 in the new code). This is correct and consistent.

Verify PublishAsync return type:
The PR wraps Task PublishAsync(...) calls with new ValueTask(Task). This is valid and aligns with the TUnit guideline "Performance First" - avoiding async state machine overhead when the methods don't need to await. The interface requires ValueTask return type, so this wrapper is appropriate.

DataTypesProduced caching is a clear win:
Moving from a collection expression that allocates on every property access to a cached static array eliminates ~15MB of allocations as shown in the metrics. This follows TUnit's "minimize allocations in hot paths" principle.

Previous Review Status

Unable to retrieve previous comments due to GitHub token scope limitations.

Verdict

APPROVE - No critical issues. The changes are well-motivated performance improvements that align with TUnit's "Performance First" principle. The 15MB savings on TUnit.ExampleProject demonstrates meaningful impact.

@thomhurst thomhurst merged commit 0f27b69 into thomhurst:main Jan 13, 2026
9 of 10 checks passed
This was referenced Jan 13, 2026
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