From 9779081fde9d420e925c991698d640ebf973d3ff Mon Sep 17 00:00:00 2001 From: shijiashuai Date: Tue, 26 May 2026 09:28:21 +0800 Subject: [PATCH] fix: harden memory utilities and build regressions - fix make_aligned object lifetime semantics and add regression test\n- wire OpenMP into auto_vectorize to honor omp simd pragmas\n- stabilize core cache-line constant and remove shadowing warning source\n- add build configuration integration tests and repair benchmark compare integration test path/expectations Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../02-memory-cache/include/memory_utils.hpp | 31 ++++- examples/04-simd-vectorization/CMakeLists.txt | 1 + include/hpc/core.hpp | 12 +- tests/integration/build_configuration_test.py | 107 ++++++++++++++++++ tests/integration/compare_benchmarks_test.py | 39 ++++--- tests/unit/memory/memory_utils_test.cpp | 26 +++++ 6 files changed, 187 insertions(+), 29 deletions(-) create mode 100644 tests/integration/build_configuration_test.py diff --git a/examples/02-memory-cache/include/memory_utils.hpp b/examples/02-memory-cache/include/memory_utils.hpp index c391690..5f0ecba 100644 --- a/examples/02-memory-cache/include/memory_utils.hpp +++ b/examples/02-memory-cache/include/memory_utils.hpp @@ -63,18 +63,24 @@ inline void aligned_free(void* ptr) { #endif } -/** - * @brief Custom deleter for aligned memory - */ +template struct AlignedDeleter { - void operator()(void* ptr) const { aligned_free(ptr); } + std::size_t count = 0; + + void operator()(T* ptr) const noexcept { + if (ptr == nullptr) { + return; + } + std::destroy_n(ptr, count); + aligned_free(ptr); + } }; /** * @brief Unique pointer with aligned memory */ template -using aligned_unique_ptr = std::unique_ptr; +using aligned_unique_ptr = std::unique_ptr>; /** * @brief Create aligned unique pointer @@ -82,11 +88,24 @@ using aligned_unique_ptr = std::unique_ptr; template aligned_unique_ptr make_aligned(std::size_t count, std::size_t alignment = hpc::core::CACHE_LINE_SIZE) { + if (count == 0) { + return aligned_unique_ptr(nullptr, AlignedDeleter{}); + } + void* ptr = aligned_alloc(count * sizeof(T), alignment); if (!ptr) { throw std::bad_alloc(); } - return aligned_unique_ptr(static_cast(ptr)); + + T* typed_ptr = static_cast(ptr); + try { + std::uninitialized_default_construct_n(typed_ptr, count); + } catch (...) { + aligned_free(typed_ptr); + throw; + } + + return aligned_unique_ptr(typed_ptr, AlignedDeleter{count}); } //------------------------------------------------------------------------------ diff --git a/examples/04-simd-vectorization/CMakeLists.txt b/examples/04-simd-vectorization/CMakeLists.txt index cda63ea..1ff3a04 100644 --- a/examples/04-simd-vectorization/CMakeLists.txt +++ b/examples/04-simd-vectorization/CMakeLists.txt @@ -16,6 +16,7 @@ hpc_add_example( NAME auto_vectorize SOURCES src/auto_vectorize.cpp LIBRARIES simd_utils + ENABLE_OPENMP ENABLE_SIMD AVX2 ) diff --git a/include/hpc/core.hpp b/include/hpc/core.hpp index a66a549..c7e9c2a 100644 --- a/include/hpc/core.hpp +++ b/include/hpc/core.hpp @@ -54,11 +54,7 @@ inline std::size_t cache_line_size() { * Use this when the value must be a compile-time constant (e.g., alignas). * For runtime detection, prefer cache_line_size() function. */ -#if defined(__cpp_lib_hardware_interference_size) -constexpr std::size_t CACHE_LINE_SIZE = std::hardware_destructive_interference_size; -#else constexpr std::size_t CACHE_LINE_SIZE = 64; -#endif //------------------------------------------------------------------------------ // Page Size @@ -72,19 +68,19 @@ constexpr std::size_t CACHE_LINE_SIZE = 64; * @return Page size in bytes */ inline std::size_t page_size() { - static const std::size_t ps = []() { + static const std::size_t page_size_value = []() { #if defined(_WIN32) SYSTEM_INFO sysInfo; GetSystemInfo(&sysInfo); return static_cast(sysInfo.dwPageSize); #elif defined(__unix__) || defined(__APPLE__) - long ps = sysconf(_SC_PAGESIZE); - return ps > 0 ? static_cast(ps) : 4096; + long queried_page_size = sysconf(_SC_PAGESIZE); + return queried_page_size > 0 ? static_cast(queried_page_size) : 4096; #else return 4096; // Fallback #endif }(); - return ps; + return page_size_value; } /** diff --git a/tests/integration/build_configuration_test.py b/tests/integration/build_configuration_test.py new file mode 100644 index 0000000..53a2670 --- /dev/null +++ b/tests/integration/build_configuration_test.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + + +PROJECT_ROOT = Path(__file__).resolve().parents[2] + + +class BuildConfigurationTest(unittest.TestCase): + def run_configure(self, *cmake_args: str) -> subprocess.CompletedProcess[str]: + with tempfile.TemporaryDirectory() as tmpdir: + build_dir = Path(tmpdir) / "build" + return subprocess.run( + [ + "cmake", + "-S", + str(PROJECT_ROOT), + "-B", + str(build_dir), + "-G", + "Ninja", + *cmake_args, + ], + capture_output=True, + text=True, + check=False, + ) + + def run_configure_and_build( + self, + target: str, + *cmake_args: str, + ) -> subprocess.CompletedProcess[str]: + with tempfile.TemporaryDirectory() as tmpdir: + build_dir = Path(tmpdir) / "build" + configure = subprocess.run( + [ + "cmake", + "-S", + str(PROJECT_ROOT), + "-B", + str(build_dir), + "-G", + "Ninja", + *cmake_args, + ], + capture_output=True, + text=True, + check=False, + ) + if configure.returncode != 0: + return configure + + return subprocess.run( + [ + "cmake", + "--build", + str(build_dir), + "--target", + target, + "-j2", + ], + capture_output=True, + text=True, + check=False, + ) + + def test_configure_succeeds_with_benchmarks_enabled_and_tests_disabled(self): + result = self.run_configure( + "-DCMAKE_BUILD_TYPE=Debug", + "-DHPC_BUILD_TESTS=OFF", + "-DHPC_BUILD_BENCHMARKS=ON", + ) + + self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr) + + def test_auto_vectorize_build_does_not_ignore_omp_simd_pragmas(self): + result = self.run_configure_and_build( + "auto_vectorize", + "-DCMAKE_BUILD_TYPE=Debug", + "-DHPC_BUILD_TESTS=OFF", + "-DHPC_BUILD_BENCHMARKS=OFF", + ) + + self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr) + combined_output = result.stdout + "\n" + result.stderr + self.assertNotIn("ignoring '#pragma omp simd'", combined_output, msg=combined_output) + + def test_core_test_build_avoids_core_header_warnings(self): + result = self.run_configure_and_build( + "core_test", + "-DCMAKE_BUILD_TYPE=Debug", + "-DHPC_BUILD_TESTS=ON", + "-DHPC_BUILD_BENCHMARKS=OFF", + ) + + self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr) + combined_output = result.stdout + "\n" + result.stderr + self.assertNotIn("-Winterference-size", combined_output, msg=combined_output) + self.assertNotIn("shadows a previous local", combined_output, msg=combined_output) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/integration/compare_benchmarks_test.py b/tests/integration/compare_benchmarks_test.py index 8692c0b..8c87667 100644 --- a/tests/integration/compare_benchmarks_test.py +++ b/tests/integration/compare_benchmarks_test.py @@ -8,11 +8,11 @@ PROJECT_ROOT = Path(__file__).resolve().parents[2] -SCRIPT_PATH = PROJECT_ROOT / "scripts" / "compare_benchmarks.py" +SCRIPT_PATH = PROJECT_ROOT / "tools" / "performance" / "benchmark_compare.py" class CompareBenchmarksScriptTest(unittest.TestCase): - def run_script(self, baseline_payload, candidate_payload, threshold=10): + def run_script(self, baseline_payload, candidate_payload, threshold=0.1, fail_on_regression=False): with tempfile.TemporaryDirectory() as tmpdir: tmp_path = Path(tmpdir) baseline_path = tmp_path / "baseline.json" @@ -20,15 +20,19 @@ def run_script(self, baseline_payload, candidate_payload, threshold=10): baseline_path.write_text(json.dumps(baseline_payload), encoding="utf-8") candidate_path.write_text(json.dumps(candidate_payload), encoding="utf-8") + args = [ + sys.executable, + str(SCRIPT_PATH), + str(baseline_path), + str(candidate_path), + "--threshold", + str(threshold), + ] + if fail_on_regression: + args.append("--fail-on-regression") + return subprocess.run( - [ - sys.executable, - str(SCRIPT_PATH), - str(baseline_path), - str(candidate_path), - "--threshold", - str(threshold), - ], + args, capture_output=True, text=True, check=False, @@ -46,11 +50,11 @@ def test_exit_zero_when_all_benchmarks_stay_within_threshold(self): ] } - result = self.run_script(baseline, candidate, threshold=10) + result = self.run_script(baseline, candidate, threshold=0.10) self.assertEqual(result.returncode, 0, msg=result.stderr or result.stdout) self.assertIn("BM_AddArrays/1024", result.stdout) - self.assertIn("+8.00%", result.stdout) + self.assertIn("+8.0%", result.stdout) def test_exit_one_when_any_benchmark_regresses_beyond_threshold(self): baseline = { @@ -64,11 +68,16 @@ def test_exit_one_when_any_benchmark_regresses_beyond_threshold(self): ] } - result = self.run_script(baseline, candidate, threshold=10) + result = self.run_script( + baseline, + candidate, + threshold=0.10, + fail_on_regression=True, + ) self.assertEqual(result.returncode, 1, msg=result.stderr or result.stdout) - self.assertIn("Regression detected", result.stdout) - self.assertIn("+20.00%", result.stdout) + self.assertIn("regression(s) detected", result.stdout) + self.assertIn("+20.0%", result.stdout) if __name__ == "__main__": diff --git a/tests/unit/memory/memory_utils_test.cpp b/tests/unit/memory/memory_utils_test.cpp index d7e5d31..f13b30b 100644 --- a/tests/unit/memory/memory_utils_test.cpp +++ b/tests/unit/memory/memory_utils_test.cpp @@ -12,6 +12,18 @@ using hpc::core::PAGE_SIZE; namespace hpc::memory::test { +namespace { + +struct TrackedObject { + inline static int constructions = 0; + inline static int destructions = 0; + + TrackedObject() { ++constructions; } + ~TrackedObject() { ++destructions; } +}; + +} // namespace + // --------------------------------------------------------------------------- // AlignedAllocator tests // --------------------------------------------------------------------------- @@ -62,6 +74,20 @@ TEST(AlignedAllocTest, MakeAlignedUniquePtr) { EXPECT_EQ(reinterpret_cast(ptr.get()) % CACHE_LINE_SIZE, 0u); } +TEST(AlignedAllocTest, MakeAlignedConstructsAndDestroysObjects) { + TrackedObject::constructions = 0; + TrackedObject::destructions = 0; + + { + auto ptr = make_aligned(3, CACHE_LINE_SIZE); + ASSERT_NE(ptr.get(), nullptr); + EXPECT_EQ(TrackedObject::constructions, 3); + EXPECT_EQ(reinterpret_cast(ptr.get()) % CACHE_LINE_SIZE, 0u); + } + + EXPECT_EQ(TrackedObject::destructions, 3); +} + // --------------------------------------------------------------------------- // Constants tests // ---------------------------------------------------------------------------