Skip to content

perf: remove unused value and early return in DedThreadExe#4517

Merged
thomhurst merged 1 commit intothomhurst:mainfrom
TimothyMakkison:return_ded
Feb 3, 2026
Merged

perf: remove unused value and early return in DedThreadExe#4517
thomhurst merged 1 commit intothomhurst:mainfrom
TimothyMakkison:return_ded

Conversation

@TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jan 20, 2026

  • Remove unsued DedicatedThreadTaskScheduler _taskScheduler field
  • Make ExecuteAsync sync
  • Make Task.Factory.StartNew sync

Before

image

After

image

@TimothyMakkison TimothyMakkison marked this pull request as draft January 20, 2026 18:11
@thomhurst
Copy link
Owner

Looks like ya posted the same image for before and after

@TimothyMakkison
Copy link
Contributor Author

TimothyMakkison commented Jan 21, 2026

Looks like ya posted the same image for before and after

Yeah, I've fixed it, but semi abadoned this PR when I realised the savings weren't adding up and I had no idea how to prove that line 82 did anything.

@TimothyMakkison
Copy link
Contributor Author

This should help a little, don't know how much though

@claude
Copy link
Contributor

claude bot commented Feb 3, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.


Review Summary:

This PR makes clean performance optimizations that align well with TUnit's "Performance First" principle:

  1. Removes unused field - The _taskScheduler field was stored but never used
  2. Eliminates async overhead - Converting ExecuteAsync from async to sync by returning ValueTask directly avoids unnecessary state machine allocations
  3. Optimizes Task.Factory.StartNew - Replacing the async lambda with a sync lambda + .AsTask() reduces allocations while maintaining identical semantics

All changes are semantically equivalent to the original code while reducing memory allocations in hot-path test execution code.

@thomhurst thomhurst merged commit d7b5b33 into thomhurst:main Feb 3, 2026
9 of 11 checks passed
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