From e2ad95d806d2e364c99781074aae0b6e3f98c503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Mon, 30 Sep 2024 19:24:58 +0200 Subject: [PATCH] [wip] Test RID_Owner thread safety --- .github/workflows/linux_builds.yml | 69 ----------------- .github/workflows/macos_builds.yml | 29 +------ .github/workflows/runner.yml | 39 ---------- .github/workflows/windows_builds.yml | 26 ------- tests/core/templates/test_rid.h | 112 +++++++++++++++++++++++++++ 5 files changed, 113 insertions(+), 162 deletions(-) diff --git a/.github/workflows/linux_builds.yml b/.github/workflows/linux_builds.yml index dc3d9f378624..a5a566a34432 100644 --- a/.github/workflows/linux_builds.yml +++ b/.github/workflows/linux_builds.yml @@ -23,18 +23,6 @@ jobs: fail-fast: false matrix: include: - - name: Editor w/ Mono (target=editor) - cache-name: linux-editor-mono - target: editor - sconsflags: module_mono_enabled=yes - bin: ./bin/godot.linuxbsd.editor.x86_64.mono - build-mono: true - tests: false # Disabled due freeze caused by mix Mono build and CI - doc-test: true - proj-conv: true - api-compat: true - artifact: true - - name: Editor with doubles and GCC sanitizers (target=editor, tests=yes, dev_build=yes, scu_build=yes, precision=double, use_asan=yes, use_ubsan=yes, linker=gold) cache-name: linux-editor-double-sanitizers target: editor @@ -71,23 +59,6 @@ jobs: # Skip 2GiB artifact speeding up action. artifact: false - - name: Template w/ Mono (target=template_release, tests=yes) - cache-name: linux-template-mono - target: template_release - sconsflags: module_mono_enabled=yes - bin: ./bin/godot.linuxbsd.template_release.x86_64.mono - build-mono: false - tests: true - artifact: true - - - name: Minimal template (target=template_release, tests=yes, everything disabled) - cache-name: linux-template-minimal - target: template_release - sconsflags: modules_enabled_by_default=no disable_3d=yes disable_advanced_gui=yes deprecated=no minizip=no - bin: ./bin/godot.linuxbsd.template_release.x86_64 - tests: true - artifact: true - steps: - name: Checkout uses: actions/checkout@v4 @@ -159,24 +130,6 @@ jobs: run: | ./modules/mono/build_scripts/build_assemblies.py --godot-output-dir=./bin --godot-platform=linuxbsd - - name: Prepare artifact - if: matrix.artifact - run: | - strip bin/godot.* - chmod +x bin/godot.* - - - name: Upload artifact - uses: ./.github/actions/upload-artifact - if: matrix.artifact - with: - name: ${{ matrix.cache-name }} - - - name: Dump Godot API - uses: ./.github/actions/godot-api-dump - if: matrix.api-dump - with: - bin: ${{ matrix.bin }} - - name: Unit tests if: matrix.tests run: | @@ -189,31 +142,9 @@ jobs: run: | dotnet test modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests - # Check class reference - - name: Check for class reference updates - if: matrix.doc-test - run: | - echo "Running --doctool to see if this changes the public API without updating the documentation." - echo -e "If a diff is shown, it means that your code/doc changes are incomplete and you should update the class reference with --doctool.\n\n" - ${{ matrix.bin }} --doctool --headless 2>&1 > /dev/null || true - git diff --color --exit-code && ! git ls-files --others --exclude-standard | sed -e 's/^/New doc file missing in PR: /' | grep 'xml$' - - # Check API backwards compatibility - - name: Check for GDExtension compatibility - if: matrix.api-compat - run: | - ./misc/scripts/validate_extension_api.sh "${{ matrix.bin }}" - # Download and run the test project - name: Test Godot project uses: ./.github/actions/godot-project-test if: matrix.proj-test with: bin: ${{ matrix.bin }} - - # Test the project converter - - name: Test project converter - uses: ./.github/actions/godot-converter-test - if: matrix.proj-conv - with: - bin: ${{ matrix.bin }} diff --git a/.github/workflows/macos_builds.yml b/.github/workflows/macos_builds.yml index fcf4b00afb18..ad6e541bdddb 100644 --- a/.github/workflows/macos_builds.yml +++ b/.github/workflows/macos_builds.yml @@ -24,14 +24,7 @@ jobs: cache-name: macos-editor target: editor tests: true - bin: ./bin/godot.macos.editor.universal - - - name: Template (target=template_release, tests=yes) - cache-name: macos-template - target: template_release - tests: true - sconsflags: debug_symbols=no - bin: ./bin/godot.macos.template_release.universal + bin: ./bin/godot.macos.editor.arm64 steps: - name: Checkout @@ -52,14 +45,6 @@ jobs: run: | sh misc/scripts/install_vulkan_sdk_macos.sh - - name: Compilation (x86_64) - uses: ./.github/actions/godot-build - with: - sconsflags: ${{ env.SCONSFLAGS }} arch=x86_64 - platform: macos - target: ${{ matrix.target }} - tests: ${{ matrix.tests }} - - name: Compilation (arm64) uses: ./.github/actions/godot-build with: @@ -74,18 +59,6 @@ jobs: cache-name: ${{ matrix.cache-name }} continue-on-error: true - - name: Prepare artifact - run: | - lipo -create ./bin/godot.macos.${{ matrix.target }}.x86_64 ./bin/godot.macos.${{ matrix.target }}.arm64 -output ./bin/godot.macos.${{ matrix.target }}.universal - rm ./bin/godot.macos.${{ matrix.target }}.x86_64 ./bin/godot.macos.${{ matrix.target }}.arm64 - strip bin/godot.* - chmod +x bin/godot.* - - - name: Upload artifact - uses: ./.github/actions/upload-artifact - with: - name: ${{ matrix.cache-name }} - - name: Unit tests if: matrix.tests run: | diff --git a/.github/workflows/runner.yml b/.github/workflows/runner.yml index d2d0e3571fda..52f0897d7d8a 100644 --- a/.github/workflows/runner.yml +++ b/.github/workflows/runner.yml @@ -6,53 +6,14 @@ concurrency: cancel-in-progress: true jobs: - # First stage: Only static checks, fast and prevent expensive builds from running. - - static-checks: - if: '!vars.DISABLE_GODOT_CI' - name: 📊 Static checks - uses: ./.github/workflows/static_checks.yml - - # Second stage: Run all the builds and some of the tests. - - android-build: - name: 🤖 Android - needs: static-checks - uses: ./.github/workflows/android_builds.yml - - ios-build: - name: 🍏 iOS - needs: static-checks - uses: ./.github/workflows/ios_builds.yml - linux-build: name: 🐧 Linux - needs: static-checks uses: ./.github/workflows/linux_builds.yml macos-build: name: 🍎 macOS - needs: static-checks uses: ./.github/workflows/macos_builds.yml windows-build: name: 🏁 Windows - needs: static-checks uses: ./.github/workflows/windows_builds.yml - - web-build: - name: 🌐 Web - needs: static-checks - uses: ./.github/workflows/web_builds.yml - - # Third stage: Run auxiliary tests using build artifacts from previous jobs. - - # Can be turned off for PRs that intentionally break compat with godot-cpp, - # until both the upstream PR and the matching godot-cpp changes are merged. - godot-cpp-test: - name: 🪲 Godot CPP - # This can be changed to depend on another platform, if we decide to use it for - # godot-cpp instead. Make sure to move the .github/actions/godot-api-dump step - # appropriately. - needs: linux-build - uses: ./.github/workflows/godot_cpp_test.yml diff --git a/.github/workflows/windows_builds.yml b/.github/workflows/windows_builds.yml index 95e3d4a553d1..8c28b90f31ad 100644 --- a/.github/workflows/windows_builds.yml +++ b/.github/workflows/windows_builds.yml @@ -32,21 +32,6 @@ jobs: bin: ./bin/godot.windows.editor.x86_64.exe artifact: true - - name: Editor w/ clang-cl (target=editor, tests=yes, use_llvm=yes) - cache-name: windows-editor-clang - target: editor - tests: true - sconsflags: debug_symbols=no windows_subsystem=console use_llvm=yes - bin: ./bin/godot.windows.editor.x86_64.llvm.exe - - - name: Template (target=template_release, tests=yes) - cache-name: windows-template - target: template_release - tests: true - sconsflags: debug_symbols=no - bin: ./bin/godot.windows.template_release.x86_64.console.exe - artifact: true - steps: - name: Checkout uses: actions/checkout@v4 @@ -93,17 +78,6 @@ jobs: cache-name: ${{ matrix.cache-name }} continue-on-error: true - - name: Prepare artifact - if: ${{ matrix.artifact }} - run: | - Remove-Item bin/* -Include *.exp,*.lib,*.pdb -Force - - - name: Upload artifact - if: ${{ matrix.artifact }} - uses: ./.github/actions/upload-artifact - with: - name: ${{ matrix.cache-name }} - - name: Unit tests if: matrix.tests run: | diff --git a/tests/core/templates/test_rid.h b/tests/core/templates/test_rid.h index ba9a2bb5e267..4ae8f03b5fe2 100644 --- a/tests/core/templates/test_rid.h +++ b/tests/core/templates/test_rid.h @@ -31,7 +31,10 @@ #ifndef TEST_RID_H #define TEST_RID_H +#include "core/os/thread.h" +#include "core/templates/local_vector.h" #include "core/templates/rid.h" +#include "core/templates/rid_owner.h" #include "tests/test_macros.h" @@ -96,6 +99,115 @@ TEST_CASE("[RID] 'get_local_index'") { CHECK(RID::from_uint64(4'294'967'295).get_local_index() == 4'294'967'295); CHECK(RID::from_uint64(4'294'967'297).get_local_index() == 1); } + +TEST_CASE("[RID_Owner] Thread safety") { + static const size_t CACHE_LINE_BYTES = std::hardware_constructive_interference_size; + struct alignas(CACHE_LINE_BYTES) DataHolder { + uint32_t value = 0; + }; + + struct RID_OwnerTester { + RID_Owner rid_owner; + TightLocalVector threads; + SafeNumeric next_thread_idx; + TightLocalVector rids; + // Using std::atomic directly since SafeNumeric doesn't support relaxed ordering. + std::atomic sync[2] = {}; + std::atomic correct = 0; + + // A latch that doesn't introduce memory ordering constraints, only compiler ones. + // The idea is not to cause any sync traffic that would make the code we want to test + // seem correct as a side effect. + void lockstep(uint32_t p_step) { + uint32_t buf_idx = p_step % 2; + uint32_t target = (p_step / 2 + 1) * threads.size(); + std::atomic_signal_fence(std::memory_order_seq_cst); + sync[buf_idx].fetch_add(1, std::memory_order_relaxed); + do { + std::atomic_signal_fence(std::memory_order_seq_cst); + } while (sync[buf_idx].load(std::memory_order_relaxed) != target); + std::atomic_signal_fence(std::memory_order_seq_cst); + } + + explicit RID_OwnerTester(bool p_chunk_for_all, bool p_chunks_preallocated) : + rid_owner(sizeof(DataHolder) * (p_chunk_for_all ? OS::get_singleton()->get_processor_count() : 1)) { + threads.resize(OS::get_singleton()->get_processor_count()); + rids.resize(threads.size()); + if (p_chunks_preallocated) { + LocalVector prealloc_rids; + for (uint32_t i = 0; i < (p_chunk_for_all ? 1 : threads.size()); i++) { + prealloc_rids.push_back(rid_owner.make_rid()); + } + for (uint32_t i = 0; i < prealloc_rids.size(); i++) { + rid_owner.free(prealloc_rids[i]); + } + } + } + + ~RID_OwnerTester() { + for (uint32_t i = 0; i < threads.size(); i++) { + rid_owner.free(rids[i]); + } + } + + void test() { + for (uint32_t i = 0; i < threads.size(); i++) { + threads[i].start( + [](void *p_data) { + RID_OwnerTester *rot = (RID_OwnerTester *)p_data; + + // 1. Each thread gets a zero-based index. + uint32_t th_idx = rot->next_thread_idx.postincrement(); + + rot->lockstep(0); + + // 2. Each thread makes a RID holding its index. + const DataHolder initial_data = { th_idx }; + rot->rids[th_idx] = rot->rid_owner.make_rid(initial_data); + + rot->lockstep(1); + + // 3. Each thread verifies all the others. + uint32_t local_correct = 0; + for (uint32_t i = 0; i < rot->threads.size(); i++) { + DataHolder *data = rot->rid_owner.get_or_null(rot->rids[i]); + if (data->value == i) { + local_correct++; + } + } + + rot->lockstep(2); + + rot->correct.fetch_add(local_correct, std::memory_order_acq_rel); + }, + this); + } + + for (uint32_t i = 0; i < threads.size(); i++) { + threads[i].wait_to_finish(); + } + + CHECK_EQ(correct.load(), threads.size() * threads.size()); + } + }; + + SUBCASE("All items in one chunk, pre-allocated") { + RID_OwnerTester tester(true, true); + tester.test(); + } + SUBCASE("All items in one chunk, NOT pre-allocated") { + RID_OwnerTester tester(true, false); + tester.test(); + } + SUBCASE("One item per chunk, pre-allocated") { + RID_OwnerTester tester(false, true); + tester.test(); + } + SUBCASE("One item per chunk, NOT pre-allocated") { + RID_OwnerTester tester(false, false); + tester.test(); + } +} } // namespace TestRID #endif // TEST_RID_H