Skip to content

Conversation

@TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jan 14, 2026

  • Remove async keyword making methods synchronous
  • Use ValueListBuilder in RegisterArgumentsAsync, using the newer Task.WhenAll(ReadOnlySpan<Task>) overload where possible.
    • Older versions of .NET either return a single Task or create an array.

Before

image

After

image

@thomhurst
Copy link
Owner

Summary

This PR optimizes ObjectLifecycleService by removing unnecessary async keywords and using ValueListBuilder for better performance in RegisterArgumentsAsync.

Critical Issues

None found ✅

Suggestions

Consider Task.WhenAll behavior on older .NET versions

In RegisterArgumentsAsync, the fallback for pre-NET9 targets allocates a new array via .ToArray(), which reduces the performance benefit of using ValueListBuilder. However, this is only for older .NET targets where the span-based Task.WhenAll overload does not exist, so it is a reasonable trade-off. The NET9+ path gets the full benefit.

Performance validation

The PR description shows allocation improvements in a screenshot. Since this touches a hot path (object registration), the performance improvement is welcome and aligns with TUnits Performance First principle.

Previous Review Status

No previous comments found.

Verdict

✅ APPROVE - No critical issues. The changes are clean, correct, and improve performance:

  • Methods that were unnecessarily async are now synchronous (eliminating state machine overhead)
  • ValueListBuilder reduces allocations on the happy path
  • ValueTask.AsTask() is correctly used for the cleanup method
  • Unused using directive correctly removed
  • All changes preserve existing behavior while improving efficiency

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