Skip to content

Avoid RangeSet node reallocations for right-growing merges - #36

Closed
Majed-allababedi wants to merge 2 commits into
Jetsku:mainfrom
Majed-allababedi:perf/rangeset-inplace-merge
Closed

Majed-allababedi wants to merge 2 commits into
Jetsku:mainfrom
Majed-allababedi:perf/rangeset-inplace-merge

Conversation

@Majed-allababedi

@Majed-allababedi Majed-allababedi commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Avoid erasing and reallocating the first std::map node when RangeSet::Add grows an existing interval without changing its start.

  • Keep the existing contained-range fast path.
  • Extend the retained node's endpoint and erase only absorbed successors.
  • Leave new-start/left-growing/disjoint insertion, End() validation, synchronization, and all renderer/coherence ordering unchanged.
  • Add a standalone range_set_tests target, register it with CTest/kyty_tests, and explicitly include it in the Windows U59 CI build/test lists.

This targets repeated dirty-range/scissor-row accumulation. It introduces no game-specific behavior, graphics-setting changes, shader changes, or quality reductions.

Why the merge is equivalent

The existing lower_bound/predecessor selection guarantees that an anchor with it->first <= address touches or overlaps the incoming interval. The preceding containment check guarantees end > it->second on the new path. The key can therefore stay fixed while the endpoint grows; successor absorption uses the same <= touching rule and max endpoint as before. Erasing successors does not invalidate the retained iterator.

Tests against this PR's actual base

Base: f9e19582f0deac0d1226a7ce3faae30a9ab408ab (main), not the newer release branch used for playtesting.

Windows, clang-cl 22.1.3, Release:

  • Before the production change: --semantics-only passed; --allocations-only failed on the intended replacement-node allocation/free assertions. The contained control was already allocation-free.
  • After the change: range_set, memory_tracker, and page_manager passed (3/3 CTest entries).
  • Seven targeted allocation cases include right-touching/overlapping growth, same-start growth, containment, successor absorption, and the maximum valid endpoint. Eligible merges allocate zero nodes; absorbed successors are still freed.
  • The unchanged test source exercises 60,000 randomized mutations and 240,000 range queries against an independent byte model, including high addresses and UINT64_MAX boundaries.
  • The identical production headers were additionally reviewed with exhaustive prior/current oracles and MSVC checked iterators. This is component verification, not a whole-program race or graphics-correctness claim.

Reproduce the focused suite after normal project configuration:

cmake --build build --target range_set_tests memory_tracker_tests page_manager_tests
ctest --test-dir build --output-on-failure -R "^(range_set|memory_tracker|page_manager)$"

The standalone executable also accepts --semantics-only, --allocations-only, and --bench.

Component measurements — not game FPS

Measured on an i5-14600K / Windows using the byte-identical before/after RangeSet headers and test source in the AMD5b-based development build. The PR-base correctness tests above were rebuilt separately. Both measured variants used the same compiler/Release flags and allocation wrappers. Setup, destruction, and output validation were outside timing; counting was a separate pass. Ten A-B-B-A cycles yielded 20 measured samples per variant, excluding warmups.

Representative medians, 64 seed groups and 32 adds per group:

Workload Before, ns/add After, ns/add Component speedup
Touching right growth 63.316 15.007 4.22x
Same-start growth 56.772 13.166 4.31x
Bridging successors 116.491 57.078 2.04x
Already contained control 11.396 11.447 approximately unchanged
Left-growing control 56.939 57.124 approximately unchanged
Disjoint control 105.967 103.948 approximately unchanged

A slower control is retained in the results: contained/one-group increased from 3.433 to 3.771 ns/add (about 9.86%, or 0.338 ns). Seed groups are not always map-node counts: the bridge case initially has 33 nodes per group. Benchmark allocation/free columns describe one counting pass; timing summaries describe run averages, not single-operation latency.

Playtesting and limits

I noticed a good improvement while playing Astro Bot on my i5-14600K / RX 6800 with this change on u59-windows-20261011-int18.1-amd5b-pre. This is a qualitative observation, not a controlled same-scene FPS measurement or a claimed FPS percentage. The local builds use IPO with PGO disabled, so comparing them with an official release is not a source-only performance comparison. Existing local non-tiled-lighting/GI-probe workarounds were retained; none are part of this PR.

The broader AMD5b test run was not all-green: GPU harness runs were blocked by rasterization-feature/validation-layer prerequisites, and the unrelated thread_service precise-wait assertion remained failing. draw_prep exceeded an initial 60-second cap but passed with a 120-second allowance. These are separately recorded release-based results, not a claim that the full main test suite passed. No renderer barriers or test assertions were weakened.

@Majed-allababedi

Copy link
Copy Markdown
Contributor Author

I investigated the failed Windows check on this PR. The important finding is that the same five failures reproduce on unmodified upstream 28bc20f146a276479acf42d2e405fb46a4491b8e, without the RangeSet change, using the same local clang-cl 22.1.3 / Release / IPO configuration. This is an existing upstream test-expectation mismatch, not a demonstrated RangeSet regression or a newly discovered runtime defect.

Failure and root cause

PR run 38451080651 fails all five virtual_memory_allocation* variants at:

VirtualMemoryAllocationTests: PackedBitFieldInsert failed:
SSE4a bitfield result or destination lanes differ

Commit c42b55f94 deliberately changed INSERTQ emulation to zero bits 127:64, based on your Zen 4 measurements. Two Windows test expectations in tests/VirtualMemoryAllocationTests.cpp still require the old preserved upper quadword:

  1. TestPackedBitField(bool insert), around line 4238: output.destination[1] == (insert ? input[1] : 0).
  2. The mixed INSERTQ/CPU-extension fixture in TestCpuExtensionPatches(), around line 4745: (host.sse4a || output[3] == input[1]).

Instrumenting the first failure showed that the destination matched the expected result except for this quadword: actual 0x0000000000000000, stale expected 0xdeadbeef12345678. Updating that expectation exposed the same mismatch in the mixed fixture. Your Linux follow-up bf756628 already addresses an analogous stale expectation in the Linux-only test path.

Minimal local correction

- output.destination[1] == (insert ? input[1] : 0) &&
+ output.destination[1] == 0 &&

- (host.sse4a || output[3] == input[1]) &&
+ (host.sse4a || output[3] == 0) &&

The low-lane result oracle, upper YMM preservation, source/state checks, and existing native-host exception are unchanged. No production source changes, disabled tests, weakened masks, or added skips are needed.

To be precise, the ISA documents these upper bits as undefined. This correction matches the behavior the project intentionally selected and your reported AMD measurement; it is not a claim that zero is universally mandated by the ISA or newly measured on AMD here. The local host is Intel i5-14600K.

Verification

  • Clean official base without RangeSet: all five VM variants reproduce the failure.
  • Corrected candidate: 8/8 selected suites pass, repeated three times.
  • Correction plus RangeSet: 9/9 pass, repeated three times, followed by a separate parent rerun of all nine. These include all five VM variants, range_set, memory_tracker, page_manager, and amd_cpu_emulation.
  • Sensitivity check: temporarily restoring the old production behavior (preserving bits 127:64) makes the corrected VM test fail. The independent AMD suite also detects it: 12,000 synthetic CONTEXT mismatches and 800 real-#UD mismatches. That mutation was removed, production source restored exactly, and the corrected tests rebuilt.
  • Independent review reran preserved baseline/final/mutation executables and confirmed fail/pass/fail, plus the final AMD suite with zero mismatches.

The same-base upstream main CI run passed 20/20, so I am not claiming it failed everywhere. The test has native-SSE4a-dependent paths/skips, which could explain different runner outcomes, but I have not established the runner CPU identity or proven that explanation. Remote CI has not been rerun with this correction yet.

The correction is currently local and review-verified; this comment does not change the PR branch. It can be applied independently of the RangeSet optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant