more improvements - #26
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves MinimalWorker’s developer-facing documentation and optimizes generated worker/metrics plumbing in the source generator.
Changes:
- Expanded XML documentation for
IWorkerBuilderandRun*BackgroundWorkerAPIs (naming, error handling, scoping, scheduling semantics). - Optimized generated metrics gauge enumeration by introducing a cached snapshot of worker states.
- Replaced generated switch-based worker initializer dispatch with a dictionary-based O(1) lookup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/MinimalWorker/BackgroundWorkerExtensions.cs | Adds/expands XML docs for worker registration APIs and registration metadata types. |
| src/MinimalWorker.Generators/WorkerEmitter.cs | Updates generated code to cache worker-state snapshots for gauges and to use dictionary-based signature dispatch for initializer selection. |
| sb.AppendLine(" var cached = _cachedSnapshot;"); | ||
| sb.AppendLine(" if (cached != null && _snapshotVersion == version)"); | ||
| sb.AppendLine(" {"); | ||
| sb.AppendLine(" return cached;"); | ||
| sb.AppendLine(" }"); | ||
| sb.AppendLine(" // Version changed or no cache - create new snapshot"); | ||
| sb.AppendLine(" var snapshot = _workerStates.Values.ToArray();"); | ||
| sb.AppendLine(" _cachedSnapshot = snapshot;"); | ||
| sb.AppendLine(" _snapshotVersion = version;"); |
There was a problem hiding this comment.
The generated snapshot cache uses plain reads/writes of _cachedSnapshot and _snapshotVersion. Because these fields aren't accessed via Volatile.Read/Write (or another memory barrier), the JIT is allowed to reorder writes, which can lead to returning an out-of-date snapshot even when _currentVersion has advanced. Consider storing (version, snapshot) as a single immutable reference updated via Volatile.Write, or make both fields volatile and use Volatile.Read/Write consistently (or lock) to ensure correctness under concurrent metrics reads/worker registration.
| sb.AppendLine(" var cached = _cachedSnapshot;"); | |
| sb.AppendLine(" if (cached != null && _snapshotVersion == version)"); | |
| sb.AppendLine(" {"); | |
| sb.AppendLine(" return cached;"); | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" // Version changed or no cache - create new snapshot"); | |
| sb.AppendLine(" var snapshot = _workerStates.Values.ToArray();"); | |
| sb.AppendLine(" _cachedSnapshot = snapshot;"); | |
| sb.AppendLine(" _snapshotVersion = version;"); | |
| sb.AppendLine(" var cached = Volatile.Read(ref _cachedSnapshot);"); | |
| sb.AppendLine(" var cachedVersion = Volatile.Read(ref _snapshotVersion);"); | |
| sb.AppendLine(" if (cached != null && cachedVersion == version)"); | |
| sb.AppendLine(" {"); | |
| sb.AppendLine(" return cached;"); | |
| sb.AppendLine(" }"); | |
| sb.AppendLine(" // Version changed or no cache - create new snapshot"); | |
| sb.AppendLine(" var snapshot = _workerStates.Values.ToArray();"); | |
| sb.AppendLine(" Volatile.Write(ref _cachedSnapshot, snapshot);"); | |
| sb.AppendLine(" Volatile.Write(ref _snapshotVersion, version);"); |
| sb.AppendLine(" }"); | ||
| sb.AppendLine(); | ||
| sb.AppendLine(" /// <summary>"); | ||
| sb.AppendLine(" /// Gets a cached snapshot of worker states, only re-allocating when workers are added/removed."); |
There was a problem hiding this comment.
The emitted XML doc says the snapshot is only re-allocated when workers are added/removed, but the generated code invalidates the cache on DeactivateWorker (a state change, not an add/remove). Either update the comment to match the actual invalidation behavior, or avoid invalidating on deactivation if the goal is strictly “add/remove” (the snapshot contains references, so IsActive changes are already observable without reallocation).
| sb.AppendLine(" /// Gets a cached snapshot of worker states, only re-allocating when workers are added/removed."); | |
| sb.AppendLine(" /// Gets a cached snapshot of worker states. The snapshot is re-allocated whenever the worker state version changes (e.g., workers are added, removed, or their activation state changes)."); |
| // Emit worker initialization methods - one for each unique signature | ||
| caseNum = 1; | ||
| foreach (var kvp in workerMap) | ||
| { | ||
| EmitWorkerInitializer(sb, kvp.Value, caseNum); |
There was a problem hiding this comment.
workerMap is a Dictionary, and later you generate initializer method numbers by enumerating workerMap while the signature→method mapping is produced from a different dictionary (signatureToMethod). Since Dictionary enumeration order is not guaranteed by contract, it’s possible for the emitted InitializeWorker_{n} methods to not align with the numbers referenced in _workerInitializers, resulting in the wrong initializer being invoked for a signature (and likely invalid casts at runtime). Use a deterministic ordered sequence (e.g., a List of signatures in insertion order) to drive both the _workerInitializers entries and the emitted InitializeWorker_{n} methods.
| // Build signature from worker parameters - strip global:: prefix and normalize spacing to match runtime format | ||
| // Runtime uses FormatTypeName which joins generic args with "," (no space), so we must do the same | ||
| var paramTypes = string.Join(",", worker.Parameters.Select(p => p.Type.Replace("global::", "").Replace(", ", ","))); | ||
| var signature = $"{worker.Type}:{paramTypes}"; |
There was a problem hiding this comment.
Signature normalization here may not match the runtime BackgroundWorkerExtensions.FormatTypeName output for nested types: Type.FullName uses + between declaring and nested types, while Roslyn’s FullyQualifiedFormat typically uses .. That would produce a registration.Signature that never matches any generated initializer, and the worker won’t start. Consider normalizing nested type separators consistently on both sides (e.g., replace + with . in FormatTypeName, or adjust generator signature formatting to match Type.FullName).
| /// </para> | ||
| /// <para> | ||
| /// <b>Important:</b> Do NOT add your own loop - the framework handles repetition automatically. | ||
| /// The interval starts <i>after</i> each execution completes. |
There was a problem hiding this comment.
The remarks claim “The interval starts after each execution completes”, but the generated implementation uses PeriodicTimer(schedule, ...), which ticks on a fixed period independent of execution duration (if an iteration runs long, the next tick may be immediately available). Either adjust the docs to describe PeriodicTimer semantics, or change the implementation to a post-execution delay loop if “delay after completion” is the intended behavior.
| /// The interval starts <i>after</i> each execution completes. | |
| /// The interval is measured on a fixed schedule; if an execution runs longer than the interval, | |
| /// the next run may start immediately after the previous one completes. |
No description provided.