Implement Scheduler: stage execution, conflict batching, debug enforcement
Schedule gains real execution on top of the registration bookkeeping
from the PluginHost pass: RunStage(stage, world) runs every system
registered for a stage, and Reads<>/Writes<>() declarations are
enforced live via SystemAccessScope — a system touching a component it
didn't declare throws immediately, with a message naming the
violation, from GameWorld.Query<T>() and GameObject.GetComponent<T>()/
AddComponent<T>()/RemoveComponent<T>(). Outside of a running system
(editor code, tests, scene construction) nothing is enforced.
Scope cut made deliberately, not by accident: systems are grouped into
conflict-free batches by declared access (ComputeBatches, tested
directly), but batches run sequentially rather than on real threads.
Actually parallelizing them needs GameWorld's structural changes
(Create/Destroy/AddComponent/RemoveComponent) deferred to a command
buffer first — without that, two systems with disjoint *declared*
types can still race on shared storage, since AddComponent<T>() on a
GameObject mutates that object's own component list regardless of T.
Building real concurrency on top of a known thread-safety hole would
be worse than not building it yet. Noted as a TODO on RunStage.
Two real bugs found and fixed while wiring this up, not designed in
from the start:
- ISchedule.Add took `Delegate`, and a lambda passed there doesn't
reliably compile down to `Action<IWorld>` at runtime — the
compiler's natural-type inference for lambdas (as opposed to method
groups, which do work this way) can synthesize a different, private
delegate type instead, so `is Action<IWorld>` silently failed for
every lambda-registered system. Changed Add's parameter type to
Action<IWorld> directly, which sidesteps the inference question
entirely — found by ScheduleTests actually using lambdas, which
EchoPlugin's method-group-based Tick had been masking.
- AlcUnloadTests started failing intermittently ("ALC survived unload
cycle 3") once PluginSystemTests existed alongside it — xUnit
parallelizes across test classes by default, and ALC-unload tests
are sensitive to any concurrent activity in the process. Added
[CollectionBehavior(DisableTestParallelization = true)] to the
harness assembly; stable across 5+ repeated runs since.
EchoPlugin.Tick is no longer a stub — it increments every Ping.Count
in World, which two new integration tests in
Engine.ConformanceHarness/PluginSystemTests.cs exercise end to end: a
plugin loaded from a real collectible ALC registers a system, Schedule
actually invokes that cross-ALC delegate, and it correctly mutates a
component owned by the Default-ALC World. This only works because
PluginLoadContext resolves Sandbox.Echo.Contracts to the copy this
test project references directly, rather than loading a second,
type-incompatible one — the harness's new normal ProjectReference to
Sandbox.Echo.Contracts.csproj makes that a live assertion, not just an
implementation detail no test would notice breaking.
27 tests total now (21 in Engine.Kernel.Tests, 6 in
Engine.ConformanceHarness), all green on a clean build, harness
verified stable across repeated runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N1qPfzq8TDCUMFMV3UwV5N
This commit is contained in:
@@ -1,3 +1,5 @@
|
||||
using Engine.Kernel.World;
|
||||
|
||||
namespace Engine.Kernel.Scheduling;
|
||||
|
||||
/// <summary>
|
||||
@@ -5,7 +7,18 @@ namespace Engine.Kernel.Scheduling;
|
||||
/// </summary>
|
||||
public interface ISchedule
|
||||
{
|
||||
ISystemBuilder Add(Stage stage, Delegate system);
|
||||
/// <summary>
|
||||
/// <c>Action<IWorld></c> specifically, not <c>Delegate</c> — a
|
||||
/// lambda passed where the target type is exactly <c>Delegate</c>
|
||||
/// doesn't reliably compile down to <c>System.Action<IWorld></c>
|
||||
/// at runtime (the compiler's natural-type inference for lambdas can
|
||||
/// synthesize a different, unspeakable delegate type instead, so a
|
||||
/// runtime <c>is Action<IWorld></c> check silently fails). Method
|
||||
/// groups and lambdas both convert to <c>Action<IWorld></c>
|
||||
/// correctly when it's the actual parameter type — see §3 for both
|
||||
/// forms in use.
|
||||
/// </summary>
|
||||
ISystemBuilder Add(Stage stage, Action<IWorld> system);
|
||||
|
||||
/// <summary>Called from a plugin's Shutdown() — must remove everything
|
||||
/// Configure() added, or the ALC it lives in will never unload. See §4.</summary>
|
||||
|
||||
@@ -1,13 +1,10 @@
|
||||
using System.Reflection;
|
||||
using Engine.Kernel.World;
|
||||
|
||||
namespace Engine.Kernel.Scheduling;
|
||||
|
||||
/// <summary>
|
||||
/// Bookkeeping only for now — no stage execution, no parallelism, no
|
||||
/// enforcement of declared access. That's the real Scheduler work described
|
||||
/// in docs/kernel-contract.md §2 and §7; this pass exists to make
|
||||
/// <see cref="RemoveAllFrom"/> actually correct, because it's the one thing
|
||||
/// PluginHost's reload correctness depends on.
|
||||
/// Registration plus execution. See docs/kernel-contract.md §2 and §7.
|
||||
///
|
||||
/// Ownership is tracked by the registering delegate's declaring assembly,
|
||||
/// not by a string tag on each entry — a plugin can only accidentally
|
||||
@@ -24,7 +21,7 @@ public sealed class Schedule : ISchedule
|
||||
internal void RegisterPlugin(string pluginId, Assembly implementationAssembly)
|
||||
=> _pluginAssemblies[pluginId] = implementationAssembly;
|
||||
|
||||
public ISystemBuilder Add(Stage stage, Delegate system)
|
||||
public ISystemBuilder Add(Stage stage, Action<IWorld> system)
|
||||
{
|
||||
var entry = new SystemEntry(stage, system);
|
||||
_systems.Add(entry);
|
||||
@@ -39,4 +36,64 @@ public sealed class Schedule : ISchedule
|
||||
if (_pluginAssemblies.Remove(pluginId, out var assembly))
|
||||
_systems.RemoveAll(e => e.System.Method.DeclaringType?.Assembly == assembly);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Runs every system registered for <paramref name="stage"/>, grouped
|
||||
/// into conflict-free batches by declared Reads/Writes (see
|
||||
/// <see cref="ComputeBatches"/>) and, in DEBUG builds, enforced against
|
||||
/// what each one actually touches (see SystemAccessScope).
|
||||
///
|
||||
/// TODO: batches are computed but run sequentially, not on separate
|
||||
/// threads — real parallel dispatch needs structural changes
|
||||
/// (Create/Destroy/AddComponent/RemoveComponent) deferred to a command
|
||||
/// buffer flushed after the batch first. Without that, two systems with
|
||||
/// disjoint *declared* types can still race on shared, non-thread-safe
|
||||
/// storage: AddComponent<T>() on a GameObject mutates that
|
||||
/// GameObject's own component list regardless of T, and GameWorld's
|
||||
/// index/roots collections aren't safe for concurrent mutation either.
|
||||
/// See the Scheduler row in docs/kernel-contract.md §2.
|
||||
/// </summary>
|
||||
public void RunStage(Stage stage, IWorld world)
|
||||
{
|
||||
var stageSystems = _systems.Where(e => e.Stage == stage).ToList();
|
||||
|
||||
foreach (var batch in ComputeBatches(stageSystems))
|
||||
{
|
||||
foreach (var entry in batch)
|
||||
Invoke(entry, world);
|
||||
}
|
||||
}
|
||||
|
||||
private static void Invoke(SystemEntry entry, IWorld world)
|
||||
{
|
||||
using var _ = SystemAccessScope.Enter(entry.Reads, entry.Writes);
|
||||
entry.System(world);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Groups systems into the fewest sequential batches such that no two
|
||||
/// systems in the same batch conflict — greedy first-fit, preserving
|
||||
/// registration order. Not yet consumed for real parallelism (see the
|
||||
/// TODO on RunStage), but exercised directly by ScheduleTests so the
|
||||
/// conflict logic itself is validated independent of that.
|
||||
/// </summary>
|
||||
internal static List<List<SystemEntry>> ComputeBatches(IReadOnlyList<SystemEntry> systems)
|
||||
{
|
||||
var batches = new List<List<SystemEntry>>();
|
||||
|
||||
foreach (var entry in systems)
|
||||
{
|
||||
var batch = batches.FirstOrDefault(b => b.TrueForAll(other => !Conflicts(entry, other)));
|
||||
|
||||
if (batch is not null)
|
||||
batch.Add(entry);
|
||||
else
|
||||
batches.Add([entry]);
|
||||
}
|
||||
|
||||
return batches;
|
||||
}
|
||||
|
||||
private static bool Conflicts(SystemEntry a, SystemEntry b) =>
|
||||
a.Writes.Overlaps(b.Reads) || a.Writes.Overlaps(b.Writes) || b.Writes.Overlaps(a.Reads);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
namespace Engine.Kernel.Scheduling;
|
||||
|
||||
/// <summary>
|
||||
/// Ambient, per-thread record of which component types the currently
|
||||
/// running system declared via Reads<T>()/Writes<T>(). GameWorld
|
||||
/// and GameObject consult this — when one is active — to enforce
|
||||
/// docs/kernel-contract.md §7's rule that an undeclared access fails
|
||||
/// loudly instead of silently working by accident.
|
||||
///
|
||||
/// No scope is active outside of Schedule.RunStage's invocation of a
|
||||
/// system — editor code, tests, and initial scene construction are all
|
||||
/// unconstrained by design; enforcement exists for the frame loop, not for
|
||||
/// every touch of a GameObject anywhere in the process.
|
||||
///
|
||||
/// ThreadLocal rather than a plain static field: batches run sequentially
|
||||
/// today (see the TODO on Schedule.RunStage), but this is already correct
|
||||
/// for when a batch's systems run on separate threads instead.
|
||||
/// </summary>
|
||||
internal static class SystemAccessScope
|
||||
{
|
||||
private static readonly ThreadLocal<(IReadOnlySet<Type> Reads, IReadOnlySet<Type> Writes)?> Current = new();
|
||||
|
||||
public static IDisposable Enter(IReadOnlySet<Type> reads, IReadOnlySet<Type> writes)
|
||||
{
|
||||
var previous = Current.Value;
|
||||
Current.Value = (reads, writes);
|
||||
return new Restore(previous);
|
||||
}
|
||||
|
||||
/// <summary>Querying or fetching a component counts as a read — either
|
||||
/// Reads<T>() or Writes<T>() satisfies it.</summary>
|
||||
public static void CheckRead(Type componentType)
|
||||
{
|
||||
var scope = Current.Value;
|
||||
if (scope is null)
|
||||
return;
|
||||
|
||||
if (!scope.Value.Reads.Contains(componentType) && !scope.Value.Writes.Contains(componentType))
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"A system read '{componentType.Name}' without declaring Reads<{componentType.Name}>() " +
|
||||
$"or Writes<{componentType.Name}>() — see docs/kernel-contract.md §7.");
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Structurally changing a GameObject's components — adding or
|
||||
/// removing one — requires Writes<T>() specifically. Mutating a
|
||||
/// component's own fields after GetComponent<T>() isn't
|
||||
/// interceptable this way; see the note on GameObject.AddComponent.</summary>
|
||||
public static void CheckWrite(Type componentType)
|
||||
{
|
||||
var scope = Current.Value;
|
||||
if (scope is null)
|
||||
return;
|
||||
|
||||
if (!scope.Value.Writes.Contains(componentType))
|
||||
{
|
||||
throw new InvalidOperationException(
|
||||
$"A system structurally changed '{componentType.Name}' without declaring " +
|
||||
$"Writes<{componentType.Name}>() — see docs/kernel-contract.md §7.");
|
||||
}
|
||||
}
|
||||
|
||||
private sealed class Restore((IReadOnlySet<Type> Reads, IReadOnlySet<Type> Writes)? previous) : IDisposable
|
||||
{
|
||||
public void Dispose() => Current.Value = previous;
|
||||
}
|
||||
}
|
||||
@@ -7,10 +7,10 @@ using Engine.Kernel.World;
|
||||
/// Scheduler can build the conflict graph described in docs/kernel-contract.md
|
||||
/// §2 — nothing consumes them yet; that's real Scheduler work, not this pass.
|
||||
/// </summary>
|
||||
internal sealed class SystemEntry(Stage stage, Delegate system)
|
||||
internal sealed class SystemEntry(Stage stage, Action<IWorld> system)
|
||||
{
|
||||
public Stage Stage { get; } = stage;
|
||||
public Delegate System { get; } = system;
|
||||
public Action<IWorld> System { get; } = system;
|
||||
public string? After { get; set; }
|
||||
public HashSet<Type> Reads { get; } = [];
|
||||
public HashSet<Type> Writes { get; } = [];
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
using System.Numerics;
|
||||
using Engine.Kernel.Scheduling;
|
||||
|
||||
namespace Engine.Kernel.World;
|
||||
|
||||
@@ -77,6 +78,8 @@ public sealed class GameObject
|
||||
|
||||
public T? GetComponent<T>() where T : Component
|
||||
{
|
||||
SystemAccessScope.CheckRead(typeof(T));
|
||||
|
||||
foreach (var component in _components)
|
||||
{
|
||||
if (component is T match)
|
||||
@@ -86,8 +89,19 @@ public sealed class GameObject
|
||||
return null;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Adding a component requires <c>Writes<T>()</c> — checked here,
|
||||
/// at the structural change. What isn't and can't be checked: mutating
|
||||
/// a component's own fields after the fact, e.g.
|
||||
/// <c>go.GetComponent<T>()!.Value = 5</c>. That's a plain field
|
||||
/// write on a plain object, with nothing to intercept it — see
|
||||
/// docs/kernel-contract.md §7's note on why components stay plain
|
||||
/// classes rather than something that could enforce this fully.
|
||||
/// </summary>
|
||||
public T AddComponent<T>() where T : Component, new()
|
||||
{
|
||||
SystemAccessScope.CheckWrite(typeof(T));
|
||||
|
||||
var component = new T();
|
||||
_components.Add(component);
|
||||
Owner?.IndexComponentAdded(this, component);
|
||||
@@ -96,6 +110,8 @@ public sealed class GameObject
|
||||
|
||||
public void RemoveComponent<T>() where T : Component
|
||||
{
|
||||
SystemAccessScope.CheckWrite(typeof(T));
|
||||
|
||||
for (var i = 0; i < _components.Count; i++)
|
||||
{
|
||||
if (_components[i] is not T match)
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
using Engine.Kernel.Scheduling;
|
||||
|
||||
namespace Engine.Kernel.World;
|
||||
|
||||
/// <summary>
|
||||
@@ -56,8 +58,11 @@ public sealed class GameWorld : IWorld
|
||||
/// expected to queue structural changes to a stage boundary rather than
|
||||
/// let a running system trigger this — see the Scheduler row in §2.
|
||||
/// </summary>
|
||||
public IEnumerable<GameObject> Query<T>() where T : Component =>
|
||||
_index.TryGetValue(typeof(T), out var set) ? set : [];
|
||||
public IEnumerable<GameObject> Query<T>() where T : Component
|
||||
{
|
||||
SystemAccessScope.CheckRead(typeof(T));
|
||||
return _index.TryGetValue(typeof(T), out var set) ? set : [];
|
||||
}
|
||||
|
||||
internal void IndexComponentAdded(GameObject go, Component component)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user