Improve hot-paths for openMW - #2
Conversation
|
It doesn't matter for OpenMW, as our minimum MSVC version is now 2022, but MSVC 2019 is now installed on the 2022 runners, so if upstream wants MSVC 2019 coverage, it can still be done. For OpenMW, it'd be best to have both 2022 and 2026 coverage in CI, and then if upstream have a flurry of merges, which they do once in a while, it'll be more tempting to merge a PR with these changes if it doesn't just reduce how much CI and instead updates it. As for the actual maths and algorithms, it's not my area, so I'll ping @elsid to take a look. |
|
Unrelated to the changes themselves but we should not use |
| freeHeightfieldSpans(heightfield); | ||
| } | ||
|
|
||
| SECTION("Random sample") |
There was a problem hiding this comment.
How this single test could cover so many changes? It's also possible to add a benchmark to show in which case performance is improved.
There was a problem hiding this comment.
Yeah fair, it couldnt in isolation, I relied on the openMW side too much. I split the proof by mechanism instead, pushed in 2033b32.
Tests_RecastRasterization.cpp keeps the pre-optimization generic clipper in test code and checks each of the three overloads against reference clipping followed by public incremental rcAddSpan insertion, over the full ordered (column, smin, smax, area) stream. LCG triangles cross tile and height bounds, cells and clip planes, counts 1, 63, 64, 65 and 192 for both sides of the threshold, all six area bits, merge thresholds 0-4, plus a single-voxel golden case and a pre-populated heightfield that has to stay incremental. 4,958 assertions.
Tests_RecastFilter.cpp gets a case that keeps the old linked-list ledge scan as oracle: 32 deterministic 12×12 heightfields with 1-8 spans per column, gaps, null areas and variable walkable height/climb, comparing every span's column, min, max and area. 18,752 assertions. Also fixed the cleanup helper while in there, it only freed each column head -_-.
Bench_RecastHotPaths.cpp is hidden behind [.benchmark][recast], public API only, returns a span checksum. Release -O3, GCC 16.1.1, i7-13700KF, 100 samples, 500 ms warmup, two interleaved runs per side, same source on both sides (03259f3 vs 13e95d6):
| workload | base | fixed | speedup |
|---|---|---|---|
| 1,024 overlapping triangles | 31.33 / 31.24 ms | 23.65 / 23.63 ms | 1.32× |
| 4,096 single-cell triangles | 192.8 / 192.8 µs | 157.5 / 158.1 µs | 1.22× |
rcFilterLedgeSpans, 32×32 columns × 24 spans |
3.484 / 3.485 ms | 0.755 / 0.752 ms | 4.62× |
Overlapping triangles is the repeated linked-list insert/merge shape the bulk path targets, single-cell isolates the voxel bypass (the smaller of the two), tall columns is the neighbor scan the filter change is for. They say where each mechanism helps, equivalence comes from the tests above and from the OpenMW side: openmw-navmeshtool on one Garden cell with only the two Recast objects swapped goes 48.21 → 31.92 s CPU with all 803 prepared tiles byte-identical, at +72 MiB peak RSS since the batched paths hold per-tile scratch (multiplied per concurrent worker, +401 MiB at 15).
Suite total is 52 cases / 25,065 assertions, also green under ASan+UBSan and on Actions for that head. If there's a shape you'd want on the bench list I'll add it.
13e95d6 to
2033b32
Compare
95dfae5 to
fb2b445
Compare
@AnyOldName3 I asked out dear buddy Claude to add the 2026 coverage. LGTM but please take another critical look just in case, I haven't touched MSVC stuff in ages. The new gmake build didnt ship binaries with the +x flag so it needed a few tries to get it right :D |
420e865 to
6423620
Compare
Both Windows jobs now build with VS 2022 on windows-2022 and VS 2026 on
windows-2025 (Debug and Release each).
Premake is bumped to 5.0.0-beta8 on every platform: it is the first
release with a vs2026 action, and upstream already uses it. beta8 drops
`flags { "FatalCompileWarnings" }`, so the premake script uses
`fatalwarnings { "All" }` as upstream does, and the Linux job moves to
the `gmake` action, since beta8's `gmake2` turns that setting into an
invalid `-Werror=All`. Xcode is excluded from it: the old flag never
reached Xcode, and enabling it there fails on Contrib/fastlz.
6423620 to
9e961ca
Compare
It looks plausible to me. |
| { | ||
| span->area = RC_NULL_AREA; | ||
| span.area = RC_NULL_AREA; | ||
| span.source->area = RC_NULL_AREA; |
There was a problem hiding this comment.
If this is the only use case for source. FilterSpan could just store a pointer to it.
| { | ||
| FilterSpan& filterSpan = spans[spanIndex++]; | ||
| filterSpan.source = span; | ||
| filterSpan.smin = (unsigned short)span->smin; |
|
|
||
| const int floor = (int)(span->smax); | ||
| const int ceiling = span->next ? (int)(span->next->smin) : MAX_HEIGHTFIELD_HEIGHT; | ||
| const int floor = (int)(span.smax); |
There was a problem hiding this comment.
Even though this code pretty much looks like C it's C++ and it has static_cast.
| // heightfield is empty lets each column be merged in contiguous memory and | ||
| // emits the final linked lists column-by-column. The per-column input order is | ||
| // retained, so area merging has the same semantics as repeated addSpan calls. | ||
| struct RasterizedSpan |
There was a problem hiding this comment.
This should be in the unnamed namespace since it's not exposed in the header.
|
I think it worth to split this into PRs changing CI and and separate PRs for different parts of the library and send to the upstream for review. |
Will do once I've addressed your feedback ! 🫡 |
|
My feedback does not matter much because I'm not that familiar with this codebase. You could check correctness with tests but then you need a commit with only tests and then with changes without functional changes in tests. Running them on both with coverage report and separately with asan and ubsan should give some confidence in correctness. |
|
If upstream does what upstream typically does and ignores this for months, or outright rejects it because it's too big a change for a library that has users but isn't being especially actively maintained, then you're the only person whose feedback we're going to get, so the only person whose feedback matters in the short term. |
|
Already doing both - tests against master w/asan and ubsan + checking out the output remains identical on broad sets like the launcher's prebake. I'll split off things nicely in coherent commits and PRs and address comments on code correctness 👍 |
3 participants
OpenMW profiles put linked-list span insertion,
rcFilterLedgeSpansand triangle clipping on the cold navmesh path. Three changes, all inRecastRasterization.cppandRecastFilter.cpp.On an empty heightfield with at least 64 triangles,
rcRasterizeTrianglescollects packed spans, stable-scatters them by column, merges in contiguous arrays with a bitset over active heights, then emits each linked list once. Per-column input order andaddSpanmerge/area semantics are preserved. Smaller batches, non-empty heightfields andrcRasterizeTrianglestay on the incremental path.rcFilterLedgeSpanssnapshots the columns into a flat array with column offsets and walks neighbors with monotonic cursors instead of re-scanning the linked lists.A triangle whose bounds sit inside one cell skips the polygon clipper.
Public API, allocator hooks and C++98 compatibility are unchanged. The trade is temporary scratch memory per tile build for locality: it is released after the build, and it multiplies per concurrently built tile.
Performance
Tests/Recast/Bench_RecastHotPaths.cpp, hidden behind[.benchmark][recast], public API only, returns a span checksum. Release-O3, GCC 16.1.1, i7-13700KF, 100 samples, 500 ms warmup, two interleaved runs per side, same source on both SHAs (03259f3vs this branch):rcFilterLedgeSpans, 32×32 columns × 24 spansRun with
build/Tests/Tests [.benchmark] --benchmark-samples 100 --benchmark-warmup-time 500.Real workload,
openmw-navmeshtoolon^The Garden, Tower$(Garden of Dreams mod), same OpenMW objects at846be7a137, onlyRecastFilter.cpp.oandRecastRasterization.cpp.oswapped, fresh DB, 1,061 tile attempts, 803 stored,/usr/bin/time -v:Correctness
The pre-optimization generic clipper and linked-list ledge scan are kept in test code as oracles.
Tests_RecastRasterization.cppruns each of the threercRasterizeTrianglesoverloads against reference clipping followed by public incrementalrcAddSpaninsertion and compares the full ordered(column, smin, smax, area)stream, over triangle counts on both sides of the batch threshold, all six area bits and merge thresholds 0-4.Tests_RecastFilter.cppcompares every span of 32 deterministic 12×12 heightfields against the frozen scan. The OpenMW grand tour normalizes only database-local IDs before comparing every remaining prepared-navmesh byte.2033b32The workflow cleanup removes the retired VS2019 jobs, keeps VS2022 Debug and Release, and adds VS2026.
Companion OpenMW MR: https://gitlab.com/OpenMW/openmw/-/merge_requests/5517