diff --git a/.github/scripts/fetch-trace-fixture-lfs.sh b/.github/scripts/fetch-trace-fixture-lfs.sh index ac0be72c..4834b4cf 100644 --- a/.github/scripts/fetch-trace-fixture-lfs.sh +++ b/.github/scripts/fetch-trace-fixture-lfs.sh @@ -1,6 +1,10 @@ #!/usr/bin/env bash set -euo pipefail +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=trace-fixture-lib.sh +. "${script_dir}/trace-fixture-lib.sh" + if [ "$#" -lt 1 ] || [ "$#" -gt 2 ]; then echo "usage: $0 [fixture-dir]" >&2 exit 2 @@ -62,57 +66,6 @@ if [ "${case_name}" = "OpenRA" ]; then exit 0 fi -get_lfs_metadata() { - local file="$1" - local pointer - local expected_oid - local expected_size - - if ! pointer="$(git show "HEAD:${file}" 2>/dev/null)"; then - echo "failed to read tracked fixture metadata: ${file}" >&2 - return 1 - fi - if ! grep -q '^version https://git-lfs.github.com/spec/v1$' <<< "${pointer}"; then - echo "tracked fixture is not a Git LFS pointer: ${file}" >&2 - return 1 - fi - - expected_oid="$(awk '$1 == "oid" && $2 ~ /^sha256:/ { sub(/^sha256:/, "", $2); print $2 }' <<< "${pointer}")" - expected_size="$(awk '$1 == "size" { print $2 }' <<< "${pointer}")" - if ! [[ "${expected_oid}" =~ ^[0-9a-f]{64}$ ]] || ! [[ "${expected_size}" =~ ^[0-9]+$ ]]; then - echo "invalid Git LFS pointer metadata: ${file}" >&2 - return 1 - fi - - printf '%s %s\n' "${expected_oid}" "${expected_size}" -} - -verify_fixture_file() { - local downloaded_file="$1" - local display_name="$2" - local expected_oid="$3" - local expected_size="$4" - local actual_oid - local actual_size - - if [ ! -f "${downloaded_file}" ]; then - echo "fixture file is missing: ${display_name}" >&2 - return 1 - fi - - actual_size="$(wc -c < "${downloaded_file}" | tr -d '[:space:]')" - if [ "${actual_size}" != "${expected_size}" ]; then - echo "fixture size mismatch for ${display_name}: expected ${expected_size}, got ${actual_size}" >&2 - return 1 - fi - - actual_oid="$(sha256sum "${downloaded_file}" | awk '{ print $1 }')" - if [ "${actual_oid}" != "${expected_oid}" ]; then - echo "fixture SHA-256 mismatch for ${display_name}: expected ${expected_oid}, got ${actual_oid}" >&2 - return 1 - fi -} - fetch_file_from_mirror() { local file="$1" local url="$2" diff --git a/.github/scripts/trace-fixture-cache.sh b/.github/scripts/trace-fixture-cache.sh new file mode 100644 index 00000000..11b525bd --- /dev/null +++ b/.github/scripts/trace-fixture-cache.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# Cache-side helper for trace fixtures. +# +# key [fixture-dir] derive the actions/cache key and path list +# verify [fixture-dir] check restored fixtures against their pointers +# reset [fixture-dir] drop restored fixtures, leaving the pointers +# +# The cache key is content-addressed on the Git LFS pointer oids tracked at +# HEAD, which are readable from a plain checkout without smudging. Fixture +# content therefore maps 1:1 onto a key: unchanged content hits, changed +# content is a new key and thus a miss, and the download path handles it. The +# key deliberately carries no restore-keys prefix in the workflow - a fixture +# that does not match the pointer exactly must never be restored. +set -euo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=trace-fixture-lib.sh +. "${script_dir}/trace-fixture-lib.sh" + +# Bump when the key derivation changes in a way that must invalidate old +# entries; the content digest alone would not notice a format change. +key_schema="v1" + +if [ "$#" -lt 2 ] || [ "$#" -gt 3 ]; then + echo "usage: $0 [fixture-dir]" >&2 + exit 2 +fi + +command_name="$1" +case_name="$2" +fixture_dir="${3:-tools/trace_replay/fixtures}" +python_bin="${PYTHON:-python3}" + +if ! command -v "${python_bin}" >/dev/null 2>&1 && command -v python >/dev/null 2>&1; then + python_bin=python +fi + +mapfile -t files < <(trace_fixture_files "${case_name}" "${fixture_dir}" "${python_bin}") +if [ "${#files[@]}" -eq 0 ]; then + echo "no fixture files declared for trace case: ${case_name}" >&2 + exit 1 +fi + +# Writes "name=value" to $GITHUB_OUTPUT when running under Actions, and to +# stdout otherwise so the script stays runnable (and testable) off-CI. +emit_output() { + local name="$1" + local value="$2" + if [ -n "${GITHUB_OUTPUT:-}" ]; then + if [[ "${value}" == *$'\n'* ]]; then + local delimiter="ghadelim_$(date +%s%N)_$$" + { + printf '%s<<%s\n' "${name}" "${delimiter}" + printf '%s\n' "${value}" + printf '%s\n' "${delimiter}" + } >> "${GITHUB_OUTPUT}" + else + printf '%s=%s\n' "${name}" "${value}" >> "${GITHUB_OUTPUT}" + fi + fi + printf '%s=%s\n' "${name}" "${value}" +} + +sanitize_case() { + printf '%s' "$1" | sed 's/[^A-Za-z0-9._-]/_/g' +} + +case "${command_name}" in + key) + manifest="" + for file in "${files[@]}"; do + # A case whose fixtures are committed directly rather than through Git LFS + # (OpenRA) has no pointer oid to key on, and nothing to download either. + # Report it as uncacheable so the workflow skips the cache entirely. + if ! metadata="$(get_lfs_metadata "${file}" 2>/dev/null)"; then + echo "trace case ${case_name} is not stored in Git LFS; skipping fixture cache" >&2 + emit_output "cacheable" "false" + emit_output "key" "" + exit 0 + fi + read -r expected_oid expected_size <<< "${metadata}" + manifest+="$(basename "${file}") ${expected_oid} ${expected_size}"$'\n' + done + + digest="$(printf '%s' "${manifest}" | sha256sum | awk '{ print substr($1, 1, 16) }')" + safe_case="$(sanitize_case "${case_name}")" + + emit_output "cacheable" "true" + emit_output "key" "trace-fixture-${key_schema}-${safe_case}-${digest}" + emit_output "paths" "$(printf '%s\n' "${files[@]}")" + ;; + + verify) + for file in "${files[@]}"; do + metadata="$(get_lfs_metadata "${file}")" + read -r expected_oid expected_size <<< "${metadata}" + verify_fixture_file "${file}" "${file}" "${expected_oid}" "${expected_size}" + done + echo "Verified ${#files[@]} fixture file(s) for ${case_name} against the tracked Git LFS pointers." + ;; + + reset) + # Put the working tree back to the pointer files a fresh checkout would + # have, so that a rejected cache entry falls through to exactly the same + # download path a cache miss takes. + for file in "${files[@]}"; do + rm -f "${file}" "${file}.tmp" + done + git checkout -- "${files[@]}" + echo "Reset ${#files[@]} fixture file(s) for ${case_name} to their tracked Git LFS pointers." + ;; + + *) + echo "unknown command: ${command_name}" >&2 + exit 2 + ;; +esac diff --git a/.github/scripts/trace-fixture-lib.sh b/.github/scripts/trace-fixture-lib.sh new file mode 100644 index 00000000..0e3b971b --- /dev/null +++ b/.github/scripts/trace-fixture-lib.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# Shared helpers for trace-fixture handling: reading the in-tree Git LFS pointer +# metadata and verifying a fixture file against it. Sourced by +# fetch-trace-fixture-lfs.sh (verify after download) and by +# trace-fixture-cache.sh (cache key derivation and verify after cache restore), +# so both paths agree on what a valid fixture is. + +# Reads the Git LFS pointer tracked at HEAD for a fixture path and prints +# " ". Fails if the tracked blob is not a well-formed LFS pointer. +get_lfs_metadata() { + local file="$1" + local pointer + local expected_oid + local expected_size + + if ! pointer="$(git show "HEAD:${file}" 2>/dev/null)"; then + echo "failed to read tracked fixture metadata: ${file}" >&2 + return 1 + fi + if ! grep -q '^version https://git-lfs.github.com/spec/v1$' <<< "${pointer}"; then + echo "tracked fixture is not a Git LFS pointer: ${file}" >&2 + return 1 + fi + + expected_oid="$(awk '$1 == "oid" && $2 ~ /^sha256:/ { sub(/^sha256:/, "", $2); print $2 }' <<< "${pointer}")" + expected_size="$(awk '$1 == "size" { print $2 }' <<< "${pointer}")" + if ! [[ "${expected_oid}" =~ ^[0-9a-f]{64}$ ]] || ! [[ "${expected_size}" =~ ^[0-9]+$ ]]; then + echo "invalid Git LFS pointer metadata: ${file}" >&2 + return 1 + fi + + printf '%s %s\n' "${expected_oid}" "${expected_size}" +} + +# Checks an on-disk fixture against the size and SHA-256 from its LFS pointer. +verify_fixture_file() { + local downloaded_file="$1" + local display_name="$2" + local expected_oid="$3" + local expected_size="$4" + local actual_oid + local actual_size + + if [ ! -f "${downloaded_file}" ]; then + echo "fixture file is missing: ${display_name}" >&2 + return 1 + fi + + actual_size="$(wc -c < "${downloaded_file}" | tr -d '[:space:]')" + if [ "${actual_size}" != "${expected_size}" ]; then + echo "fixture size mismatch for ${display_name}: expected ${expected_size}, got ${actual_size}" >&2 + return 1 + fi + + actual_oid="$(sha256sum "${downloaded_file}" | awk '{ print $1 }')" + if [ "${actual_oid}" != "${expected_oid}" ]; then + echo "fixture SHA-256 mismatch for ${display_name}: expected ${expected_oid}, got ${actual_oid}" >&2 + return 1 + fi +} + +# Prints the fixture file paths of a trace case, one per line. Strips CR so the +# result is usable when python emits CRLF (Git Bash on Windows). +trace_fixture_files() { + local case_name="$1" + local fixture_dir="$2" + local python_bin="${3:-python3}" + + "${python_bin}" tools/trace_replay/trace_cases.py \ + --format fixture-files \ + --case "${case_name}" \ + --fixture-root "${fixture_dir}" | tr -d '\r' +} diff --git a/.github/workflows/apk.yml b/.github/workflows/apk.yml index 2460094c..4ec2e6af 100644 --- a/.github/workflows/apk.yml +++ b/.github/workflows/apk.yml @@ -201,9 +201,41 @@ jobs: - name: Checkout repo uses: actions/checkout@v6 + - name: Derive trace fixture cache key + id: fixture-key + run: bash .github/scripts/trace-fixture-cache.sh key '${{ matrix.case }}' + + - name: Restore trace fixture cache + id: fixture-cache + if: steps.fixture-key.outputs.cacheable == 'true' + uses: actions/cache/restore@v5 + with: + path: ${{ steps.fixture-key.outputs.paths }} + key: ${{ steps.fixture-key.outputs.key }} + + - name: Verify restored trace fixture + id: fixture-verify + if: steps.fixture-cache.outputs.cache-hit == 'true' + run: | + if bash .github/scripts/trace-fixture-cache.sh verify '${{ matrix.case }}'; then + echo "ok=true" >> "$GITHUB_OUTPUT" + else + echo "ok=false" >> "$GITHUB_OUTPUT" + echo "::warning::Cached fixture for ${{ matrix.case }} failed verification; falling back to the download path" + bash .github/scripts/trace-fixture-cache.sh reset '${{ matrix.case }}' + fi + - name: Fetch trace fixture + if: steps.fixture-verify.outputs.ok != 'true' run: bash .github/scripts/fetch-trace-fixture-lfs.sh '${{ matrix.case }}' + - name: Save trace fixture cache + if: steps.fixture-key.outputs.cacheable == 'true' && steps.fixture-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v5 + with: + path: ${{ steps.fixture-key.outputs.paths }} + key: ${{ steps.fixture-key.outputs.key }} + - name: Stage trace fixture run: | safe_case="$(printf '%s' '${{ matrix.case }}' | sed 's/[^A-Za-z0-9._-]/_/g')" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4be96a3c..210bb132 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -470,9 +470,41 @@ jobs: - name: Checkout repo uses: actions/checkout@v6 + - name: Derive trace fixture cache key + id: fixture-key + run: bash .github/scripts/trace-fixture-cache.sh key '${{ matrix.case }}' + + - name: Restore trace fixture cache + id: fixture-cache + if: steps.fixture-key.outputs.cacheable == 'true' + uses: actions/cache/restore@v5 + with: + path: ${{ steps.fixture-key.outputs.paths }} + key: ${{ steps.fixture-key.outputs.key }} + + - name: Verify restored trace fixture + id: fixture-verify + if: steps.fixture-cache.outputs.cache-hit == 'true' + run: | + if bash .github/scripts/trace-fixture-cache.sh verify '${{ matrix.case }}'; then + echo "ok=true" >> "$GITHUB_OUTPUT" + else + echo "ok=false" >> "$GITHUB_OUTPUT" + echo "::warning::Cached fixture for ${{ matrix.case }} failed verification; falling back to the download path" + bash .github/scripts/trace-fixture-cache.sh reset '${{ matrix.case }}' + fi + - name: Fetch trace fixture + if: steps.fixture-verify.outputs.ok != 'true' run: bash .github/scripts/fetch-trace-fixture-lfs.sh '${{ matrix.case }}' + - name: Save trace fixture cache + if: steps.fixture-key.outputs.cacheable == 'true' && steps.fixture-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v5 + with: + path: ${{ steps.fixture-key.outputs.paths }} + key: ${{ steps.fixture-key.outputs.key }} + - name: Stage trace fixture run: | safe_case="$(printf '%s' '${{ matrix.case }}' | sed 's/[^A-Za-z0-9._-]/_/g')" diff --git a/.gitmodules b/.gitmodules index 5bebb9c2..3ae968ea 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,9 +7,6 @@ [submodule "3rdparty/SPIRV-Cross"] path = 3rdparty/SPIRV-Cross url = https://github.com/KhronosGroup/SPIRV-Cross.git -[submodule "include/FastSTL"] - path = include/FastSTL - url = https://github.com/MobileGL-Dev/FastSTL.git [submodule "3rdparty/tracy"] path = 3rdparty/tracy url = https://github.com/wolfpld/tracy.git @@ -34,3 +31,6 @@ [submodule "3rdparty/asio"] path = 3rdparty/asio url = https://github.com/chriskohlhoff/asio.git +[submodule "include/ska"] + path = include/ska + url = https://github.com/MobileGL-Dev/flat_hash_map.git diff --git a/MobileGL/Includes.h b/MobileGL/Includes.h index d3a3eff5..50c3d9d6 100644 --- a/MobileGL/Includes.h +++ b/MobileGL/Includes.h @@ -49,8 +49,8 @@ #include #endif -// Include FastSTL -#include +// Include ska::flat_hash_map +#include // Include xxHash #include diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 444833bb..a47406b5 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -605,10 +605,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // Cached address of g_xfbObjects[g_currentXfbName]: PrepareForDraw consults // CurrentXfb on EVERY draw (StartPendingTransformFeedback) and the map // lookup was pure per-draw overhead for the overwhelmingly common no-capture - // case. FastSTL's open addressing keeps values in the bucket array, so ANY - // insert can rehash and move them (and erase/clear can too): every site that - // mutates the map or rebinds the current name resets this to null instead of - // reasoning about stability, and CurrentXfb re-resolves lazily. + // case. Open addressing keeps values in the bucket array, so ANY insert can + // rehash and move them - and erase moves them too, by shifting the rest of the + // probe cluster into the hole, which reaches entries other than the erased one. + // Every site that mutates the map or rebinds the current name resets this to + // null instead of reasoning about stability, and CurrentXfb re-resolves lazily. XfbObjectState* g_currentXfbState = nullptr; XfbObjectState& CurrentXfb() { @@ -883,7 +884,7 @@ namespace MobileGL::MG_Backend::DirectGLES { if (it->second.esId != 0 && g_GLESFuncs.glDeleteTransformFeedbacks != nullptr) { g_GLESFuncs.glDeleteTransformFeedbacks(1, &it->second.esId); } - g_currentXfbState = nullptr; // erase can move values (open addressing) + g_currentXfbState = nullptr; // erase shifts the probe cluster, moving other entries g_xfbObjects.erase(it); // The frontend reverts to the default object when the bound one is deleted. if (g_currentXfbName == name) { @@ -5172,8 +5173,19 @@ namespace MobileGL::MG_Backend::DirectGLES { const SharedPtr& dstTexture, GLenum dstTarget, GLint dstLevel, GLint dstX, GLint dstY, GLint dstZ, GLsizei srcWidth, GLsizei srcHeight, GLsizei srcDepth) { - auto& srcBackendTexture = TextureImpl::SyncTextureObjectToBackend(srcTexture); - auto& dstBackendTexture = TextureImpl::SyncTextureObjectToBackend(dstTexture); + // BY VALUE, not by reference. SyncTextureObjectToBackend hands back a reference to a + // slot inside the backend texture registry, and the second call mutates that very map: + // GetOrCreate indexes it (an insert relocates entries - by rehashing, and also by + // robin-hood displacement well under the load factor), and Find drops any + // entry whose state object has expired - which, with the map open-addressed and erasing + // by shifting the probe cluster backwards, relocates entries other than the erased one. + // Either way a reference taken by the first call is stale by the time the second returns, + // and it is read four more times below. Copying the SharedPtr costs two refcount bumps on + // a path that is already doing a texture copy. + const SharedPtr srcBackendTexture = + TextureImpl::SyncTextureObjectToBackend(srcTexture); + const SharedPtr dstBackendTexture = + TextureImpl::SyncTextureObjectToBackend(dstTexture); const Bool srcIsDepth = MG_Util::IsDepthFormatInternalFormat(srcTexture->GetFormat()); const Bool dstIsDepth = MG_Util::IsDepthFormatInternalFormat(dstTexture->GetFormat()); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 594f8cc7..ac13dfc1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -129,6 +129,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // Null when no live state object owns this key. The result points into the map, so // it stays valid only until the next GetOrCreate/Find/CollectGarbage on this registry. + // Take that literally, including for Find: the map is open-addressed and erases by + // shifting the rest of the probe cluster into the hole, so an erase relocates entries + // OTHER than the erased one - and Find erases, whenever it lands on a key whose state + // object has expired. Callers that need the twin across another registry call must copy + // the BackendPtr out (or keep only the pointee, which is heap-allocated and never moves). BackendPtr* Find(StateObject* stateObj) { const auto entryIt = m_entries.find(stateObj); if (entryIt == m_entries.end()) { diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp index 046c8a9e..a870db33 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp @@ -805,8 +805,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { // Resolve the index BEFORE taking the reference, and bounds-check the way the // sibling getter does. GetShaderStorageBlockIndex re-enters GetProgramResourceCache, // which indexes g_programResourceCaches and can therefore insert - and that map is - // FastSTL's open-addressed unordered_map, whose rehash MOVES its buckets, so a - // reference taken before the call is left dangling. Binding a program's storage block + // open-addressed, so a rehash MOVES its entries and a reference taken before the + // call is left dangling. Binding a program's storage block // while another program's entry was still absent from the cache was a reproducible // segfault (ProgramPipelineScenario's two storage-block cases, in one process). const GLuint blockIndex = GetShaderStorageBlockIndex(*programObject, storageBlockName); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index 7f19be6d..d942a0ac 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -138,6 +138,14 @@ namespace MobileGL::MG_Backend::DirectVulkan { hash = other.hash; stages = std::move(other.stages); modules = std::move(other.modules); + // Must travel with `modules`: these digests name the SPIR-V those exact + // shader modules were built from, and the pipeline-failure diagnostics + // print the two together. Leaving it behind used to merely lose the + // digests on a rehash; now that the cache is a robin-hood table, insertion + // SWAPS two entries, and a field that no move touches stays behind in the + // slot - pairing one program's modules with another program's digests, so + // a pipeline failure would be reported against the wrong SPIR-V. + stageSpirvDigests = std::move(other.stageSpirvDigests); descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); @@ -189,6 +197,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { hash = other.hash; stages = std::move(other.stages); modules = std::move(other.modules); + stageSpirvDigests = std::move(other.stageSpirvDigests); // travels with `modules` - see the move ctor descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); @@ -257,6 +266,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { } modules.clear(); stages.clear(); + stageSpirvDigests.clear(); // the modules they describe are gone } }; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h index 6d307eee..e5642fbd 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h @@ -111,10 +111,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { const VulkanRendererConfig& m_config; VkPhysicalDevice m_physicalDevice = VK_NULL_HANDLE; - // Values are heap-allocated: FastSTL::unordered_map is open-addressing, - // so INSERT invalidates references to stored values. The draw path (and - // the VAOs' state-pointer memos) hold entry pointers across inserts; - // only the unique_ptr cell moves, never the pointee. + // Values are heap-allocated: UnorderedMap is open-addressing, so INSERT + // invalidates references to stored values - and so does ERASE, which shifts + // the rest of the probe cluster into the hole and therefore moves entries + // other than the erased one. The draw path (and the VAOs' state-pointer + // memos) hold entry pointers across both; only the unique_ptr cell moves, + // never the pointee. UnorderedMap> m_cache; // Monotonic frame-boundary counter (bumped in OnFrameBoundary) for cache aging. Uint64 m_frameBoundaryCounter = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h index 0077d918..531a46be 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h @@ -101,6 +101,42 @@ namespace MobileGL::MG_Backend::DirectVulkan { std::swap(layers, that.layers); std::swap(lastUsedFrame, that.lastUsedFrame); } + // Move ASSIGNMENT, not just construction. The move constructor above and the + // destructor below each independently suppress the implicit one, which left the + // type move-constructible but not move-assignable - and therefore not swappable, + // which std::swap(pair&, pair&) requires. That was invisible while UnorderedMap + // only ever move-CONSTRUCTED an element into a fresh slot. ska::flat_hash_map + // probes robin-hood: inserting swaps the entry being placed against the one + // already sitting in the slot whenever it has travelled further from its desired + // position, so the mapped type has to be swappable or the table fails to + // instantiate at all. + // + // SWAP SEMANTICS, exactly like the move constructor: this does not release the + // destination's handles, it parks them in `that`, which destroys them when it + // dies. That is correct for the only caller - std::swap, whose temporary expires + // immediately - and it is what keeps the three-move sequence from destroying a + // live render pass. It is NOT correct for a hand-written `a = std::move(b)` where + // `a` held live handles and `b` outlives the statement: those handles would then + // survive until `b` dies. There is no such caller; add a destroy-then-steal + // assignment before writing one. + RenderPassEntry& operator=(RenderPassEntry&& that) noexcept { + if (this != &that) { + std::swap(hash, that.hash); + std::swap(renderPass, that.renderPass); + std::swap(framebuffer, that.framebuffer); + std::swap(compatibilityHash, that.compatibilityHash); + std::swap(pendingClearAttachments, that.pendingClearAttachments); + std::swap(trackedAttachmentLayouts, that.trackedAttachmentLayouts); + std::swap(attachmentCount, that.attachmentCount); + std::swap(colorAttachmentCount, that.colorAttachmentCount); + std::swap(hasDepthStencilAttachment, that.hasDepthStencilAttachment); + std::swap(sampleCount, that.sampleCount); + std::swap(extent, that.extent); + std::swap(layers, that.layers); + std::swap(lastUsedFrame, that.lastUsedFrame); + } + return *this; + } RenderPassEntry( Uint64 hash, VkRenderPass renderpass, @@ -315,26 +351,30 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint64 deferredAtFrame = 0; }; - // Node-based std::unordered_map, deliberately not FastSTL's open-addressing UnorderedMap: + // Node-based std::unordered_map, deliberately NOT the open-addressing UnorderedMap: // callers cache a RenderbufferResource* - or a bare &resource->layout - and then make further // calls that touch this map. BlitFramebuffer is the one that bit: it resolves the source and // destination colour bindings (ResolveColorBlitBinding caches &rbResource->layout), then - // materializes the source's pending clear, which looks that same resource up again. FastSTL's - // operator[] runs its load-factor check before find_key and reallocates the whole bucket array - // when occupancy crosses it, so even a plain lookup relocates every element; erase only - // tombstones and never decrements the occupancy, so the doubling keeps firing. After a - // relocation the cached pointer names freed storage still holding the pre-clear - // VK_IMAGE_LAYOUT_UNDEFINED, and BlitFramebuffer bails out at "source image layout is - // undefined", silently dropping the blit - renderbuffers_storage_multisample read back zero - // instead of the clear colour on exactly the iterations that grew the table. + // materializes the source's pending clear, which looks that same resource up again. Growing + // an open-addressed table relocates every element, so the cached pointer went on to name + // freed storage still holding the pre-clear VK_IMAGE_LAYOUT_UNDEFINED; BlitFramebuffer bailed + // out at "source image layout is undefined", silently dropping the blit - + // renderbuffers_storage_multisample read back zero instead of the clear colour on exactly the + // iterations that grew the table. // // Reordering the materialize ahead of the resolves - the fix ReadPixels got - does not cover // this: the destination resolve still runs after the source pointer is taken. The depth blit, // GetOrCreateRenderPass's depthRenderbufferResource and ReadDepthStencilPixels cache the same // kind of pointer, so the invariant belongs in the container rather than in a per-call-site - // ordering rule. m_textureResources is node-based for the same reason. This buys stability - // across rehash and insert only - erase still invalidates the erased element, which is safe - // here because a renderbuffer that is an FBO attachment is held alive by that attachment. + // ordering rule. m_textureResources is node-based for the same reason. + // + // The case for keeping this node-based got STRONGER with ska::flat_hash_map, so do not read + // the paragraph above as merely historical: ska erases by shifting the rest of the probe + // cluster backwards into the hole, so erasing one renderbuffer relocates OTHER renderbuffers' + // entries - a cached pointer can now be invalidated by a key it has nothing to do with, which + // no call-site ordering rule can defend against. (What did change: ska's operator[] returns on + // a hit before it runs its grow check, so a plain lookup of a PRESENT key no longer relocates. + // That narrows the insert hazard; it does not touch the erase one.) std::unordered_map m_renderbufferResources; UnorderedMap m_pendingRenderbufferClears; Vector m_deferredRenderbufferReleases; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 14281e58..62f812c0 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -8660,7 +8660,7 @@ void main() { // blit binding below: for a renderbuffer/texture that has never been part of any // render pass yet (e.g. a GL_NONE draw buffer slot whose attachment is only ever // touched via an explicit glReadBuffer), materializing lazily creates its backing - // Vulkan resource for the first time. UnorderedMap (FastSTL, open-addressing) may + // Vulkan resource for the first time. UnorderedMap is open-addressing and may // rehash on that insertion, invalidating any RenderbufferResource*/TextureResource* // obtained beforehand - so ResolveColorBlitBinding's cached `trackedLayout` pointer // must be taken AFTER this, never before it. diff --git a/MobileGL/MG_Benchmark/CMakeLists.txt b/MobileGL/MG_Benchmark/CMakeLists.txt index 51d95017..f9ca958c 100644 --- a/MobileGL/MG_Benchmark/CMakeLists.txt +++ b/MobileGL/MG_Benchmark/CMakeLists.txt @@ -42,4 +42,5 @@ set_tests_properties(SanityBench PROPERTIES LABELS benchmark) add_subdirectory(Program) add_subdirectory(Buffer) -add_subdirectory(Driver) \ No newline at end of file +add_subdirectory(Driver) +add_subdirectory(Container) \ No newline at end of file diff --git a/MobileGL/MG_Benchmark/Container/CMakeLists.txt b/MobileGL/MG_Benchmark/Container/CMakeLists.txt new file mode 100644 index 00000000..b98e5d86 --- /dev/null +++ b/MobileGL/MG_Benchmark/Container/CMakeLists.txt @@ -0,0 +1,20 @@ +cmake_minimum_required(VERSION 3.24) + +add_executable( + UnorderedMapBench + UnorderedMapBench.cpp +) + +target_include_directories(UnorderedMapBench PRIVATE + ${MGL_ROOT}/include + ${MGL_ROOT}/MobileGL +) + +target_link_libraries( + UnorderedMapBench PRIVATE + benchmark::benchmark + ${LINK_LIBRARIES} +) + +add_test(NAME UnorderedMapBench COMMAND UnorderedMapBench --benchmark_counters_tabular=true) +set_tests_properties(UnorderedMapBench PROPERTIES LABELS benchmark) diff --git a/MobileGL/MG_Benchmark/Container/UnorderedMapBench.cpp b/MobileGL/MG_Benchmark/Container/UnorderedMapBench.cpp new file mode 100644 index 00000000..13cbdb80 --- /dev/null +++ b/MobileGL/MG_Benchmark/Container/UnorderedMapBench.cpp @@ -0,0 +1,248 @@ +// MobileGL - MobileGL/MG_Benchmark/Container/UnorderedMapBench.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// The standing performance observatory for MobileGL::UnorderedMap. +// +// This benchmarks the ALIAS, never a concrete table, so whatever UnorderedMap +// names today is what gets measured - swap the container in MG_Util/Types.h and +// re-run this same binary to get a directly comparable set of numbers. That is +// the point of it: the container sits on per-draw paths, so a change to it needs +// evidence, and the evidence should be produced the same way every time. +// +// The workloads are the shapes the tree actually exercises, not generic hash-map +// microbenchmarks. Four key shapes, because they stress a hash function very +// differently: +// * SEQUENTIAL dense small integers - GL object names from the index generator +// (buffer/texture/framebuffer/sampler registries). +// * POINTER real heap addresses - StateBackendObjectRegistry keys on +// StateObject*. These are aligned, so their low bits are the +// least random part of the key; a table that indexes on raw low +// bits clusters badly here and one that mixes first does not. +// Taken from the real allocator rather than a synthetic stride, +// which would flatter whichever table mixes its bits. +// * DIGEST already well-mixed 64-bit values - the XXH64 pipeline, +// vertex-input-state and program memos. +// * NAME short strings - uniform/attribute name to location maps. +// +// Sizes sweep from 8 upward because the per-draw memos are usually SMALL; a table +// that only wins at 4096 entries has not won anything that matters here. +// +// Run: build-linux/MobileGL/MG_Benchmark/Container/UnorderedMapBench +// or: ctest -R UnorderedMapBench (label: benchmark) + +#include +#include +#include +#include +#include +#include + +#include "MG_Util/Types.h" + +using namespace MobileGL; + +namespace { + + constexpr Int64 kMinSize = 8; + constexpr Int64 kMaxSize = 4096; + + // Keep the real allocations alive for the whole process: the POINTER shape is + // only honest if the keys are addresses the allocator actually handed out, and + // they have to stay unique (a freed address can be handed out twice). + std::vector>& PointerKeyStorage() { + static std::vector> storage; + return storage; + } + + Vector SequentialKeys(SizeT n) { + Vector keys; + keys.reserve(n); + for (SizeT i = 0; i < n; ++i) keys.push_back(static_cast(i) + 1); + return keys; + } + + Vector PointerKeys(SizeT n) { + auto& storage = PointerKeyStorage(); + Vector keys; + keys.reserve(n); + std::mt19937_64 rng(0xBEEF); + std::vector> churn; + for (SizeT i = 0; i < n; ++i) { + // State objects are not all one size, and the allocator sees other + // traffic between them - a single uniform stride is not what this + // registry ever sees. + const SizeT sz = 96 + (rng() % 192); + auto p = std::make_unique(sz); + keys.push_back(reinterpret_cast(p.get())); + storage.push_back(std::move(p)); + if ((rng() & 3) == 0) churn.push_back(std::make_unique(32 + (rng() % 128))); + } + return keys; + } + + Vector DigestKeys(SizeT n) { + Vector keys; + keys.reserve(n); + std::mt19937_64 rng(0xC0FFEE); + for (SizeT i = 0; i < n; ++i) keys.push_back(rng()); + return keys; + } + + Vector NameKeys(SizeT n) { + static const char* kPrefixes[] = {"u_", "a_", "mc_", "iris_", "gl_", "v_"}; + Vector keys; + keys.reserve(n); + for (SizeT i = 0; i < n; ++i) { + keys.push_back(String(kPrefixes[i % 6]) + "Uniform" + std::to_string(i) + "_xyz"); + } + return keys; + } + + // Key sets are built once per size and shared: generating them inside the timed + // loop would measure the generator (and, for POINTER, the allocator) instead of + // the table. + template + const KeyVec& CachedKeys(SizeT n) { + static UnorderedMap cache; + auto it = cache.find(n); + if (it != cache.end()) return it->second; + return cache.emplace(n, Make(n)).first->second; + } + + template + UnorderedMap Populated(const Vector& keys) { + UnorderedMap map; + for (SizeT i = 0; i < keys.size(); ++i) map[keys[i]] = i; + return map; + } + + // ---- the workloads ---------------------------------------------------- + + // The dominant per-draw operation by a wide margin: a populated cache that is + // read far more often than it is written. + template + void LookupHit(benchmark::State& state) { + const auto& keys = CachedKeys(static_cast(state.range(0))); + auto map = Populated(keys); + for (auto _ : state) { + for (const auto& k : keys) { + auto it = map.find(k); + benchmark::DoNotOptimize(it->second); + } + } + state.SetItemsProcessed(state.iterations() * static_cast(keys.size())); + } + + // "Is this resource cached yet?" answered NO - the probe length on a miss is a + // different cost from a hit, and resource caches ask this constantly. + template + void LookupMiss(benchmark::State& state) { + const SizeT n = static_cast(state.range(0)); + const auto& keys = CachedKeys(n); + auto map = Populated(keys); + const KeyVec absent = Make(n); // same shape, never inserted + for (auto _ : state) { + for (const auto& k : absent) { + benchmark::DoNotOptimize(map.find(k) != map.end()); + } + } + state.SetItemsProcessed(state.iterations() * static_cast(absent.size())); + } + + // Building a cache from empty, rehashes included. + template + void InsertGrow(benchmark::State& state) { + const auto& keys = CachedKeys(static_cast(state.range(0))); + for (auto _ : state) { + UnorderedMap map; + for (SizeT i = 0; i < keys.size(); ++i) map[keys[i]] = i; + benchmark::DoNotOptimize(map.size()); + } + state.SetItemsProcessed(state.iterations() * static_cast(keys.size())); + } + + // Cache eviction and refill: erase half by key, put them back. This is the + // aged-out-entry sweep the pipeline and vertex-input caches do. + template + void EraseChurn(benchmark::State& state) { + const auto& keys = CachedKeys(static_cast(state.range(0))); + for (auto _ : state) { + state.PauseTiming(); + auto map = Populated(keys); + state.ResumeTiming(); + for (SizeT i = 0; i < keys.size(); i += 2) benchmark::DoNotOptimize(map.erase(keys[i])); + for (SizeT i = 0; i < keys.size(); i += 2) map[keys[i]] = i; + benchmark::DoNotOptimize(map.size()); + } + state.SetItemsProcessed(state.iterations() * static_cast(keys.size())); + } + + // Mass eviction: erase-while-iterating across the whole table. This is the loop + // shape that a container's erase()-return contract can get wrong, and the one + // that fed garbage handles to vkDestroyPipeline when it was wrong before. + template + void EraseSweep(benchmark::State& state) { + const auto& keys = CachedKeys(static_cast(state.range(0))); + for (auto _ : state) { + state.PauseTiming(); + auto map = Populated(keys); + state.ResumeTiming(); + for (auto it = map.begin(); it != map.end();) it = map.erase(it); + benchmark::DoNotOptimize(map.size()); + } + state.SetItemsProcessed(state.iterations() * static_cast(keys.size())); + } + + // Whole-table walks: the per-frame sweeps that age entries out, and the + // teardown loops that destroy every Vulkan object a cache owns. + template + void Iterate(benchmark::State& state) { + const auto& keys = CachedKeys(static_cast(state.range(0))); + auto map = Populated(keys); + for (auto _ : state) { + Uint64 acc = 0; + for (const auto& entry : map) acc += entry.second; + benchmark::DoNotOptimize(acc); + } + state.SetItemsProcessed(state.iterations() * static_cast(keys.size())); + } + +} // namespace + +#define MGL_MAP_BENCH(WORKLOAD, SHAPE, VEC, MAKER) \ + BENCHMARK_TEMPLATE(WORKLOAD, VEC, MAKER) \ + ->Name(#WORKLOAD "/" #SHAPE) \ + ->RangeMultiplier(8) \ + ->Range(kMinSize, kMaxSize) + +MGL_MAP_BENCH(LookupHit, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(LookupHit, pointer, Vector, PointerKeys); +MGL_MAP_BENCH(LookupHit, digest, Vector, DigestKeys); +MGL_MAP_BENCH(LookupHit, name, Vector, NameKeys); + +MGL_MAP_BENCH(LookupMiss, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(LookupMiss, pointer, Vector, PointerKeys); +MGL_MAP_BENCH(LookupMiss, digest, Vector, DigestKeys); +MGL_MAP_BENCH(LookupMiss, name, Vector, NameKeys); + +MGL_MAP_BENCH(InsertGrow, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(InsertGrow, pointer, Vector, PointerKeys); +MGL_MAP_BENCH(InsertGrow, digest, Vector, DigestKeys); +MGL_MAP_BENCH(InsertGrow, name, Vector, NameKeys); + +MGL_MAP_BENCH(EraseChurn, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(EraseChurn, digest, Vector, DigestKeys); +MGL_MAP_BENCH(EraseChurn, name, Vector, NameKeys); + +MGL_MAP_BENCH(EraseSweep, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(EraseSweep, digest, Vector, DigestKeys); + +MGL_MAP_BENCH(Iterate, sequential, Vector, SequentialKeys); +MGL_MAP_BENCH(Iterate, digest, Vector, DigestKeys); + +BENCHMARK_MAIN(); diff --git a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp b/MobileGL/MG_State/GLState/BufferState/BufferState.cpp index b9217f23..e43ee75c 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp +++ b/MobileGL/MG_State/GLState/BufferState/BufferState.cpp @@ -67,9 +67,13 @@ namespace MobileGL::MG_State::GLState { } } } - // Key-based erase skips FastSTL's successor-iterator scan, which is - // pure overhead here and dominates delete-heavy frames. - m_bufferObjects.erase(index); + // Erase through the iterator already in hand: erase(key) would repeat the + // find() above, and the successor scan that once made key-based + // erase the cheaper of the two no longer happens here - erase(iterator) + // hands back an unconverted proxy, and the scan is what converting it + // would cost. The unbind loops above touch only the binding arrays, so + // `it` is still live. + m_bufferObjects.erase(it); } m_indexGenerator.Delete(index); } diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp index b4f38401..0b64736e 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp @@ -70,9 +70,10 @@ namespace MobileGL::MG_State::GLState { void ShaderCompileAdoptionMap::SweepIfCrowded() { if (m_entries.size() < m_sweepThreshold) return; - // Collect first, erase after: FastSTL::unordered_map is open-addressed, so erasing - // through an iterator that the same loop is still advancing is not worth reasoning - // about on a path this cold. + // Collect first, erase after: the map is open-addressed and erases by shifting the + // rest of the probe cluster into the hole, so an erase moves entries other than the + // erased one. Copying the keys out sidesteps that entirely, and this path is cold + // enough that the extra vector is not worth reasoning about the alternative. Vector dead; for (const auto& entry : m_entries) { const SharedPtr node = entry.second.lock(); diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 744fbf71..6994c1db 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -2694,8 +2694,7 @@ void main() { ASSERT_TRUE(shaderResult) << shaderResult.error().log; // PARTIALLY bound, and deliberately not a dense 0..N run - exactly what Iris does. - // mc_midTexCoord and a_Unreferenced are left unbound (FastSTL's map has no - // initializer-list constructor, hence the explicit inserts). + // mc_midTexCoord and a_Unreferenced are left unbound. UnorderedMap explicitVertexIns; explicitVertexIns["a_Position"] = 0; explicitVertexIns["a_Color"] = 1; diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 6a7b9f3f..41b72bcb 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -33,7 +33,8 @@ #include #include #include -#include +#include +#include namespace { class DynamicParameterBackend final : public MobileGL::MG_Backend::BackendObject { @@ -1998,34 +1999,51 @@ TEST(DirectGLESStateGuards, DefaultFramebufferBindGoesThroughShadow) { EXPECT_EQ(mocks.log.Count("BindFramebuffer:"), 3u); } -// FastSTL::unordered_map::erase(iterator) regression coverage. The open-addressing -// iterator constructor snaps forward from a tombstoned slot to the successor, so -// erase must NOT advance the rebuilt iterator again: the old double-advance skipped -// one live element per erase, and erasing the element in the highest occupied -// bucket pushed the returned index past bucket_count where it never compared equal -// to end() again - erase-while-iterating sweeps (pipeline/program cache eviction) -// then ran off the bucket array and fed garbage handles to vkDestroyPipeline -// (device crash on first mass eviction during world load). -TEST(FastSTLSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { - FastSTL::unordered_map map; +// UnorderedMap::erase(iterator) contract coverage. Erase-while-iterating sweeps +// (pipeline/program cache eviction) depend on `it = map.erase(it)` naming the next +// live element exactly once: a sweep that skips entries leaks them, and one that +// runs off the end feeds garbage handles to vkDestroyPipeline (device crash on the +// first mass eviction during world load - the failure FastSTL's double-advancing +// erase actually produced before it was fixed). +// +// These pin the behaviour the call sites rely on, not one map's implementation, so +// they are written against MobileGL::UnorderedMap and survive changing what it +// names. Under ska::flat_hash_map the mechanism is different - erase backward-shifts +// the rest of the probe cluster into the hole and hands back the same slot, which +// now holds the shifted-in successor - but the observable contract is the same. +TEST(UnorderedMapSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { + MobileGL::UnorderedMap map; constexpr MobileGL::Uint64 kCount = 1000; for (MobileGL::Uint64 key = 0; key < kCount; ++key) { map.emplace(key * 0x9e3779b97f4a7c15ull, key); } ASSERT_EQ(map.size(), kCount); + // Record WHICH keys the sweep hands back, not just how many. A count alone cannot + // tell a correct sweep from one that visits some element twice and misses another, + // which is exactly the shape a backward-shift bug takes: the shift rewrites the + // probe cluster, so a defect duplicates or strands elements rather than changing + // the tally. + std::set visitedKeys; MobileGL::SizeT visited = 0; for (auto it = map.begin(); it != map.end();) { + const MobileGL::Uint64 key = it->first; + EXPECT_TRUE(visitedKeys.insert(key).second) << "key " << key << " was visited twice"; it = map.erase(it); ++visited; - ASSERT_LE(visited, kCount); // old code: runaway past end / skipped entries + ASSERT_LE(visited, kCount); // runaway past end / skipped entries } EXPECT_EQ(visited, kCount); + EXPECT_EQ(visitedKeys.size(), kCount); + for (MobileGL::Uint64 key = 0; key < kCount; ++key) { + EXPECT_TRUE(visitedKeys.count(key * 0x9e3779b97f4a7c15ull) != 0) + << "key " << key << " was never visited by the sweep"; + } EXPECT_EQ(map.size(), 0u); } -TEST(FastSTLSanity, EraseReturnsTheSuccessorElement) { - FastSTL::unordered_map map; +TEST(UnorderedMapSanity, EraseReturnsTheSuccessorElement) { + MobileGL::UnorderedMap map; for (MobileGL::Uint32 key = 1; key <= 64; ++key) { map.emplace(key, key); } @@ -2033,25 +2051,48 @@ TEST(FastSTLSanity, EraseReturnsTheSuccessorElement) { // Erasing every other visited element must still visit all 64 exactly once: // the iterator returned by erase names the very next element, not one past it. MobileGL::SizeT visited = 0; - MobileGL::SizeT erased = 0; + std::set erasedKeys; + std::set keptKeys; for (auto it = map.begin(); it != map.end();) { ++visited; + const MobileGL::Uint32 key = it->first; if ((visited & 1) != 0) { + erasedKeys.insert(key); it = map.erase(it); - ++erased; } else { + keptKeys.insert(key); ++it; } ASSERT_LE(visited, 64u); } EXPECT_EQ(visited, 64u); - EXPECT_EQ(map.size(), 64u - erased); + EXPECT_EQ(erasedKeys.size() + keptKeys.size(), 64u); + EXPECT_EQ(map.size(), keptKeys.size()); + + // The interleaved erases rewrite probe clusters underneath the cursor, so the real + // question is not how many elements the loop counted but whether the table still + // resolves every key correctly afterwards. A stranded element stays in size() but + // stops being findable; a duplicated one answers for a key it does not own. + for (const MobileGL::Uint32 key : keptKeys) { + const auto found = map.find(key); + ASSERT_NE(found, map.end()) << "surviving key " << key << " is no longer findable"; + EXPECT_EQ(found->second, key) << "key " << key << " resolves to the wrong value"; + } + for (const MobileGL::Uint32 key : erasedKeys) { + EXPECT_EQ(map.find(key), map.end()) << "erased key " << key << " is still findable"; + } } -TEST(FastSTLSanity, ErasingTheOnlyElementReturnsEnd) { - FastSTL::unordered_map map; +TEST(UnorderedMapSanity, ErasingTheOnlyElementReturnsEnd) { + using Map = MobileGL::UnorderedMap; + Map map; map.emplace(42u, 1u); - auto next = map.erase(map.begin()); + + // Spell the type: erase(iterator) hands back a proxy that is convertible to an + // iterator but is not one, because finding the next element is not free and the + // callers that discard the result should not pay for it. `auto next = ...` binds + // the proxy instead, and then nothing it is compared against compiles. + Map::iterator next = map.erase(map.begin()); EXPECT_EQ(next, map.end()); EXPECT_TRUE(map.empty()); } diff --git a/MobileGL/MG_Util/Types.h b/MobileGL/MG_Util/Types.h index d34443f1..40a9568f 100644 --- a/MobileGL/MG_Util/Types.h +++ b/MobileGL/MG_Util/Types.h @@ -55,9 +55,37 @@ namespace MobileGL { using SizeT = std::size_t; template using Array = std::array; + // ska::flat_hash_map, the same table MobileGlues settled on, at the same commit. + // + // Open addressing with robin-hood probing. Any insert, emplace, operator[], + // reserve or rehash invalidates every iterator, reference and pointer into the + // map - and NOT only by rehashing: robin-hood insertion swaps the entry being + // placed against the occupant whenever it has travelled further from its desired + // position, so an insert well under the load factor still relocates entries. + // Erase relocates too, and less obviously - deletion shifts the rest of the probe + // cluster backwards, so erasing one key can move a DIFFERENT key's element. + // Where a mapped value's address has to outlive later mutation, the map holds a + // UniquePtr/SharedPtr and the pointee stays put; those sites say so where they + // are declared. + // + // Erase destroys the mapped value BEFORE it repairs the probe cluster, so a + // mapped-value destructor that re-enters the same map sees a hole in the middle + // of a chain and a stale size: a re-entrant find() misses every key past the hole. + // Nothing does that today; do not be the first without checking. + // + // Its value_type is pair with the key exposed mutably, so `it->first =` + // compiles and silently corrupts the table - the one sharp edge this map has + // that a node-based one does not. Note the Allocator default matches that + // value_type: pair, not pair. + // + // T must be move-ASSIGNABLE, not merely move-constructible: robin-hood probing + // swaps the entry being inserted against the one already in the slot whenever it + // has travelled further from its desired position. A move-only RAII type that + // declares a destructor gets no implicit move assignment, so it needs an explicit + // one or the table will not instantiate (see RenderPassEntry). template , class KeyEqual = std::equal_to, - class Allocator = std::allocator>> - using UnorderedMap = FastSTL::unordered_map; + class Allocator = std::allocator>> + using UnorderedMap = ska::flat_hash_map; template inline constexpr std::remove_reference_t&& Move(T&& t) noexcept { return static_cast&&>(t); diff --git a/README.md b/README.md index b7564ebb..693615d4 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,7 @@ MobileGL reuses several open-source projects: * **SPIRV-Cross** by **KhronosGroup** - [Apache License 2.0](https://github.com/KhronosGroup/SPIRV-Cross/blob/master/LICENSE): [github](https://github.com/KhronosGroup/SPIRV-Cross) * **glslang** by **KhronosGroup** - [Various Licenses](https://github.com/KhronosGroup/glslang/blob/main/LICENSE.txt): [github](https://github.com/KhronosGroup/glslang) * **DiligentCore** by **Diligent Graphics** - [Apache License 2.0](https://github.com/DiligentGraphics/DiligentCore/blob/master/License.txt): [github](https://github.com/DiligentGraphics/DiligentCore) +* **flat_hash_map** by **Malte Skarupke** - [Boost Software License 1.0](https://github.com/MobileGL-Dev/flat_hash_map/blob/master/LICENSE): [github](https://github.com/MobileGL-Dev/flat_hash_map) Refer to each component's repository for exact license texts. Any bundled third-party code in this repository is included under the upstream project's license. diff --git a/include/FastSTL b/include/FastSTL deleted file mode 160000 index 022211c9..00000000 --- a/include/FastSTL +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 022211c9983c70daf86d7d4cfbdb017eb1598c81 diff --git a/include/ska b/include/ska new file mode 160000 index 00000000..21c1cec9 --- /dev/null +++ b/include/ska @@ -0,0 +1 @@ +Subproject commit 21c1cec95abee1beef827e4a7c95f692875d9594