|
| 1 | +# Porting Review: box2d ddfd9df727a06940af34b5bc2ef79bcaba287d50 |
| 2 | + |
| 3 | +Original commit: `ddfd9df727a06940af34b5bc2ef79bcaba287d50` (`Threading cleanup (#1049)`) |
| 4 | + |
| 5 | +## Summary |
| 6 | + |
| 7 | +This port applies the runtime meaning of the threading cleanup to Box2D.NET: |
| 8 | + |
| 9 | +- `b2TaskCallback` and `b2EnqueueTaskCallback` no longer expose parallel-for ranges to user task systems. |
| 10 | +- Box2D.NET now owns range partitioning through `B2ParallelFors.b2ParallelFor`. |
| 11 | +- A built-in managed scheduler with long-lived worker threads is used when `workerCount > 1` and no external task callbacks are supplied. |
| 12 | +- `B2_MAX_WORKERS` is reduced to 32 and `B2_MAX_TASKS` is added. |
| 13 | +- Worker contexts can be recreated through `b2World_SetWorkerCount` / `b2World_GetWorkerCount`. |
| 14 | +- Solver task block initialization and comments were updated to preserve the original cleanup/documentation intent. |
| 15 | + |
| 16 | +## File Mapping |
| 17 | + |
| 18 | +| Original file | C# counterpart | Status | Meaning difference / notes | Remaining risk | |
| 19 | +|---|---|---:|---|---| |
| 20 | +| `include/box2d/types.h` | `src/Box2D.NET/B2Delegates.cs`, `src/Box2D.NET/B2WorldDef.cs` | Ported | Task delegates simplified to one task context; worker-count documentation updated. | Public API break matches upstream. | |
| 21 | +| `include/box2d/constants.h` | `src/Box2D.NET/B2Constants.cs` | Ported | `B2_MAX_WORKERS = 32`, `B2_MAX_TASKS = 256`. XML/doc-comment style remains C# style. | Existing users expecting 64 workers will be clamped to 32. | |
| 22 | +| `include/box2d/box2d.h` | `src/Box2D.NET/B2Worlds.cs` | Ported | Added `b2World_SetWorkerCount` and `b2World_GetWorkerCount`. | External task callbacks keep ownership of their own scheduler. | |
| 23 | +| `src/parallel_for.c`, `src/parallel_for.h` | `src/Box2D.NET/B2ParallelFor.cs` | Ported | Same atomic block-claiming algorithm; C# class contexts preserve stable worker index. | Managed task scheduling can differ in physical thread affinity. | |
| 24 | +| `src/scheduler.c`, `src/scheduler.h`, `src/core.h`, `src/timer.c` thread primitives | `src/Box2D.NET/B2Scheduler.cs`, .NET `Thread`/`SemaphoreSlim` infrastructure | Ported with .NET equivalent | Native thread/semaphore APIs are not exposed publicly in Box2D.NET; the built-in scheduler mirrors the fixed task array, worker threads, semaphore wakeup, and main-thread helping model. | OS-level details differ from C, but scheduler ownership and task lifetime now match closely. | |
| 25 | +| `src/physics_world.c`, `src/physics_world.h` | `src/Box2D.NET/B2World.cs`, `src/Box2D.NET/B2Worlds.cs` | Ported | Worker context create/destroy helpers added; built-in scheduler selected when no external callbacks are provided. | C# `B2TaskContext`/`B2SensorTaskContext` are classes, so recreation deliberately destroys and reallocates per-worker bitsets. | |
| 26 | +| `src/broad_phase.c` | `src/Box2D.NET/B2BroadPhases.cs` | Ported | Pair finding uses `b2ParallelFor`; tree rebuild uses simplified task callback; move-pair capacity changed from `8x` to `32x`. | Tree rebuild task falls back inline if `B2_MAX_TASKS` is exceeded. | |
| 27 | +| `src/sensor.c`, `src/sensor.h` | `src/Box2D.NET/B2Sensors.cs`, existing sensor context types | Ported | Sensor overlap work uses `b2ParallelFor` and internal worker indexes. | None known. | |
| 28 | +| `src/island.c`, `src/island.h` | `src/Box2D.NET/B2Islands.cs` | Ported | `b2SplitIslandTask` uses single task context callback. | None known. | |
| 29 | +| `src/solver.c`, `src/solver.h` | `src/Box2D.NET/B2Solvers.cs`, `src/Box2D.NET/B2SolverBlock.cs` | Ported | Solver block documentation preserved; block/stage initialization helpers added; finalize/bullet tasks use `b2ParallelFor`; solver tasks use simplified callback. | Managed runtime placement can still differ from native cache affinity. | |
| 30 | +| `src/contact_solver.c`, `src/contact_solver.h` | `src/Box2D.NET/B2ContactSolvers.cs` | Ported | `b2GetContactConstraintSIMDByteCount` renamed to `b2GetWideContactConstraintByteCount`. | C# allocation is element-count based, so byte count is kept as semantic/API counterpart, not used for pointer arithmetic. | |
| 31 | +| `src/container.h`, `src/array.h` | `src/Box2D.NET/B2Arrays.cs` | Already represented / partially ported | Existing generic arrays already distinguish class vs struct allocation; worker context recreation handles zero-initialization semantics explicitly. | No new generic resize-zero helper added because class field ownership must be destroyed deliberately. | |
| 32 | +| `test/test_determinism.c` | `test/Box2D.NET.Test/B2DeterminismTest.cs` | Ported | Determinism now exercises built-in scheduler over wider worker-count range. | More worker-count coverage increases runtime. | |
| 33 | +| `test/test_world.c` | `test/Box2D.NET.Test/B2WorldTest.cs` | Ported | Added `TestSetWorkerCount` with Junkyard stepping and clamp checks. | None known. | |
| 34 | +| `test/test_thread.c` | Existing .NET runtime / `B2Scheduler.cs`; no direct public core API | Not directly ported | Native `b2Thread`/`b2Semaphore` APIs do not exist in Box2D.NET. The relevant scheduler behavior is covered by determinism and worker-count tests. | Lower direct coverage of semaphore/thread wrappers because they are not public C# wrappers. | |
| 35 | +| `test/test_container.c`, `test/test_shape.c`, `test/main.c`, `test/CMakeLists.txt` | Existing NUnit tests / `.csproj` discovery | No direct code changes | Original changes are C test harness/build registration. NUnit auto-discovers tests. | None. | |
| 36 | +| `samples/*.cpp`, `samples/*.c`, `samples/*.h`, `samples/CMakeLists.txt` | `src/Box2D.NET.Samples/Samples/Sample.cs` | Ported where relevant | Samples no longer keep or pass task callbacks into worlds; built-in scheduler is used. | None known. | |
| 37 | +| `benchmark/main.c`, benchmark CSV files | `tools/Box2D.NET.Benchmark`, `src/Box2D.NET.Shared/*Data.cs` | Not directly ported | Original benchmark CSV and C harness updates do not map to the current .NET benchmark data flow in this branch. | Benchmark baselines may not reflect upstream m2air_neon CSV updates. | |
| 38 | +| `shared/random.c`, `shared/random.h`, `shared/utils.c`, `shared/utils.h`, `shared/human.c` | `src/Box2D.NET.Shared/RandomSupports.cs`, `src/Box2D.NET.Shared/Humans.cs` | Already represented / no runtime change needed | C rename from random to utils is build-organization cleanup; C# shared helpers are already split by domain. | No known runtime risk. | |
| 39 | +| `CMakeLists.txt`, `.gitignore`, `README.md`, `docs/foundation.md`, `docs/samples.md`, `src/CMakeLists.txt`, `benchmark/CMakeLists.txt`, `shared/CMakeLists.txt` | `.csproj`, `.sln`, `README.md`, `docs/*` | Reviewed, mostly not applicable | CMake/enkiTS removal has no direct .NET build counterpart. README/docs wording was not broadly rewritten because Box2D.NET docs are not 1:1 with upstream C docs. | Documentation may still mention old task callback details if present outside updated API comments. | |
| 40 | + |
| 41 | +## Special Handling |
| 42 | + |
| 43 | +- `B2TaskContext` and `B2SensorTaskContext` are C# classes. The port does not rely on shallow value-copy semantics; worker context lifetime is managed by destroy/recreate helpers. |
| 44 | +- `b2World_SetWorkerCount` switches a serial world to the built-in scheduler when the count rises above 1. This avoids a C#-specific deadlock where the serial callback would run worker 0 inline before other solver workers are enqueued. |
| 45 | +- Parallel-for worker identity is assigned internally by `B2ParallelForTask.workerIndex`, matching the upstream rule that user task systems no longer provide worker indexes. |
| 46 | +- The built-in scheduler uses managed `Thread` and `SemaphoreSlim` rather than native `b2Thread`/`b2Semaphore` wrappers, but keeps the upstream fixed worker and task-claiming structure. |
| 47 | + |
| 48 | +## Verification |
| 49 | + |
| 50 | +- `dotnet test test/Box2D.NET.Test/Box2D.NET.Test.csproj --framework net10.0 --no-restore`: passed, 163 tests. |
| 51 | +- `dotnet build Box2D.NET.sln --no-restore`: passed with pre-existing warnings. |
0 commit comments