refactor: Reduce reflection usage with compiled delegates and static abstracts

[thomhurst]

Summary

Addresses #1424 - Reduce reflection usage and consider compile-time alternatives.

This PR replaces runtime reflection patterns with compile-time alternatives for improved performance:

• ParallelLimitProvider: Use static abstract interface members (.NET 7+) instead of Activator.CreateInstance. IParallelLimit.Limit is now a static abstract property
• ModuleRunner: Create compiled delegate factories using expression trees for ModuleExecutionContext<T> creation, ExecuteAsync<T> calls, ModuleResult<T> creation, and result retrieval
• ModuleLoggerProvider: Use AsyncLocal<Type?> to track current module type, avoiding expensive stack trace inspection in most logging scenarios
• OptionsProvider: Cache compiled property accessors using expression trees, replacing GetProperty("Value") reflection on the hot path

New Factory Classes

Factory	Purpose
ExecutionContextFactory	Cached delegates for ModuleExecutionContext<T> creation
ModuleExecutionDelegateFactory	Cached delegates for ExecuteAsync<T> calls
ModuleResultFactory	Cached delegates for ModuleResult<T> creation
ResultRepositoryDelegateFactory	Cached delegates for result retrieval

Breaking Change

IParallelLimit.Limit changed from instance property to static abstract. Implementations must update:

// Before
public int Limit => 3;

// After  
public static int Limit => 3;

Test plan

• All 442 unit tests pass (437 succeeded, 5 skipped)
• Verify performance improvement with benchmarks (future work)
• Test on .NET 8+ to ensure static abstract interface members work correctly

Closes #1424

🤖 Generated with Claude Code

[github-actions]

Summary

This PR reduces reflection usage by replacing runtime patterns with compile-time alternatives using static abstract interface members (.NET 7+) and compiled expression tree delegates.

Critical Issues

1. IParallelLimitProvider visibility change

In src/ModularPipelines/Helpers/IParallelLimitProvider.cs:7, the interface changed from internal to public. This is a public API surface expansion.

Question: Was this intentional? If this is meant to remain internal infrastructure, it should stay internal. If it's meant to be public for extensibility, this should be documented in the PR description as a public API addition.

2. Potential issue with record types and static properties

In the test file (test/ModularPipelines.UnitTests/ParallelLimiterTests.cs:169), MyParallelLimit is defined as a record with a static property. Records with static abstract interface members can have subtle issues in some scenarios. While this should work fine in .NET 7+, consider adding a note in documentation that both class and record implementations are supported.

Suggestions

1. Documentation of performance gains

The PR description mentions improved performance but notes benchmarks are future work. Consider adding at least rough estimates of the performance improvement to help reviewers understand the value proposition.

2. Expression tree compilation cost

The new factory classes compile expression trees on first use and cache the results. The ConcurrentDictionary cache grows unbounded (one entry per unique result type). For typical usage this is fine, but be aware of this if you have thousands of unique module result types.

3. Error messages in factory methods

Several factory methods throw InvalidOperationException when constructors/methods aren't found. These errors would only occur if the ModularPipelines internal types changed. Consider whether these should be Debug.Assert instead, or whether the detailed error messages are valuable for future maintainability.

Previous Review Status

No previous comments.

Verdict

⚠️ REQUEST CHANGES - Need clarification on IParallelLimitProvider visibility change (critical issue #1).

If the visibility change to public was intentional and you want to expose this as public API, this PR can be approved (with the understanding that it's a public API addition). If it was accidental, it should be reverted to internal.

[github-actions]

Summary

This PR reduces reflection usage by replacing runtime patterns with compile-time alternatives using static abstract interface members and compiled expression tree delegates for improved performance.

Critical Issues

None found ✅

Suggestions

1. IParallelLimitProvider remains internal

The previous review mentioned IParallelLimitProvider visibility change to public. After reviewing the actual diff, I can confirm it remains internal - no public API expansion occurred. The interface signature changed but this is an internal implementation detail.

2. Consider documenting .NET version requirement

The PR uses static abstract interface members which require .NET 7+. While this is likely already a requirement for ModularPipelines, consider explicitly documenting this in the PR description or release notes since the IParallelLimit breaking change will affect users.

3. Memory efficiency of unbounded caches

Several new factories use ConcurrentDictionary with unbounded growth. For typical pipelines with dozens or hundreds of unique module types, this is fine. However, if someone dynamically generates thousands of unique module types at runtime, these caches would grow unbounded. Consider adding a comment noting this trade-off.

4. ParallelLimiterAttribute constraint removed

The new() constraint was removed because it is no longer needed. This is correct and intentional.

5. AsyncLocal cleanup pattern is solid

The AsyncLocal cleanup in ModuleRunner properly uses try/finally to prevent context leaks. The addition of CurrentModuleType tracking follows the same safe pattern.

6. Expression tree compilation performance

The expression tree compilation happens once per type on first use. For a typical build with 50-100 module types, this one-time cost is negligible compared to the reflection savings on every module execution. Well done!

Previous Review Status

The github-actions bot flagged concern about IParallelLimitProvider visibility change to public. After reviewing the actual code, this was a false alarm - the interface remains internal.

Verdict

✅ APPROVE - No critical issues. The refactoring successfully eliminates reflection in hot paths while maintaining thread safety and proper cleanup semantics. The breaking change to IParallelLimit is clearly documented and necessary for the static abstract pattern.

[github-actions]

Summary

This PR reduces reflection usage by replacing runtime patterns with compile-time alternatives using static abstract interface members (.NET 7+) and compiled expression tree delegates for improved performance.

Critical Issues

None found ✅

Suggestions

1. Missing IParallelLimit interface change in diff

The PR description mentions changing IParallelLimit.Limit from an instance property to a static abstract property, but I don't see this change in the provided diff. The diff shows:

• New GetLock<TParallelLimit>() generic method added to IParallelLimitProvider
• Updated ParallelLimiterAttribute<TParallelLimit> to call GetLock without reflection
• Documentation updated to show public static int Limit => 2;

However, the actual IParallelLimit interface definition change is not visible in the diff. Could you verify that the interface was updated to:

public interface IParallelLimit
{
    static abstract int Limit { get; }
}

2. ModuleLogger.CurrentModuleType added but not shown in diff

In ModuleRunner.cs:156, the code sets ModuleLogger.CurrentModuleType.Value = moduleType; and clears it in the finally block at line 162. However, the declaration of CurrentModuleType is not shown in the diff for ModuleLogger.cs. This is likely just a diff display issue, but verify that this static field was added:

internal static readonly AsyncLocal<Type?> CurrentModuleType = new();

3. Expression tree compilation is efficient

The new factory classes (ExecutionContextFactory, ModuleExecutionDelegateFactory, ModuleResultFactory, ResultRepositoryDelegateFactory) all use expression trees with ConcurrentDictionary caching. This is the correct approach - compile once per type, use forever. Well done.

4. Thread safety of AsyncLocal cleanup

The try/finally pattern for AsyncLocal cleanup in ModuleRunner.ExecuteModuleWithPipeline (lines 153-163) is correct and prevents context leaks. The pattern properly handles the case where an exception occurs immediately after assignment.

5. ParallelLimiterAttribute now abstract

The change to make ParallelLimiterAttribute abstract (line 29) with an abstract GetLock method is correct. This allows the generic version to call provider.GetLock<TParallelLimit>() without reflection, while the base class enforces the contract.

6. Documentation clarity on breaking change

The PR description clearly documents the breaking change to IParallelLimit. Users upgrading will need to change implementations from public int Limit => 3; to public static int Limit => 3;. Consider also noting the minimum .NET version requirement (.NET 7+) in the release notes.

Previous Review Status

Two previous automated reviews exist from github-actions bot:

1. First review raised concern about IParallelLimitProvider visibility change to public - this was a false alarm, the interface remains internal
2. Second review approved the PR, noting that all critical concerns were addressed

Verdict

✅ APPROVE - No critical issues found. The refactoring successfully eliminates reflection in hot paths with proper thread safety, caching, and cleanup semantics. The breaking change to IParallelLimit is well-documented and necessary for the static abstract pattern. Excellent performance optimization work.