Skip to content

Commit 9779081

Browse files
LessUpCopilot
andcommitted
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>
1 parent 4d282bb commit 9779081

6 files changed

Lines changed: 187 additions & 29 deletions

File tree

examples/02-memory-cache/include/memory_utils.hpp

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,30 +63,49 @@ inline void aligned_free(void* ptr) {
6363
#endif
6464
}
6565

66-
/**
67-
* @brief Custom deleter for aligned memory
68-
*/
66+
template <typename T>
6967
struct AlignedDeleter {
70-
void operator()(void* ptr) const { aligned_free(ptr); }
68+
std::size_t count = 0;
69+
70+
void operator()(T* ptr) const noexcept {
71+
if (ptr == nullptr) {
72+
return;
73+
}
74+
std::destroy_n(ptr, count);
75+
aligned_free(ptr);
76+
}
7177
};
7278

7379
/**
7480
* @brief Unique pointer with aligned memory
7581
*/
7682
template <typename T>
77-
using aligned_unique_ptr = std::unique_ptr<T, AlignedDeleter>;
83+
using aligned_unique_ptr = std::unique_ptr<T[], AlignedDeleter<T>>;
7884

7985
/**
8086
* @brief Create aligned unique pointer
8187
*/
8288
template <typename T>
8389
aligned_unique_ptr<T> make_aligned(std::size_t count,
8490
std::size_t alignment = hpc::core::CACHE_LINE_SIZE) {
91+
if (count == 0) {
92+
return aligned_unique_ptr<T>(nullptr, AlignedDeleter<T>{});
93+
}
94+
8595
void* ptr = aligned_alloc(count * sizeof(T), alignment);
8696
if (!ptr) {
8797
throw std::bad_alloc();
8898
}
89-
return aligned_unique_ptr<T>(static_cast<T*>(ptr));
99+
100+
T* typed_ptr = static_cast<T*>(ptr);
101+
try {
102+
std::uninitialized_default_construct_n(typed_ptr, count);
103+
} catch (...) {
104+
aligned_free(typed_ptr);
105+
throw;
106+
}
107+
108+
return aligned_unique_ptr<T>(typed_ptr, AlignedDeleter<T>{count});
90109
}
91110

92111
//------------------------------------------------------------------------------

examples/04-simd-vectorization/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ hpc_add_example(
1616
NAME auto_vectorize
1717
SOURCES src/auto_vectorize.cpp
1818
LIBRARIES simd_utils
19+
ENABLE_OPENMP
1920
ENABLE_SIMD AVX2
2021
)
2122

include/hpc/core.hpp

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,7 @@ inline std::size_t cache_line_size() {
5454
* Use this when the value must be a compile-time constant (e.g., alignas).
5555
* For runtime detection, prefer cache_line_size() function.
5656
*/
57-
#if defined(__cpp_lib_hardware_interference_size)
58-
constexpr std::size_t CACHE_LINE_SIZE = std::hardware_destructive_interference_size;
59-
#else
6057
constexpr std::size_t CACHE_LINE_SIZE = 64;
61-
#endif
6258

6359
//------------------------------------------------------------------------------
6460
// Page Size
@@ -72,19 +68,19 @@ constexpr std::size_t CACHE_LINE_SIZE = 64;
7268
* @return Page size in bytes
7369
*/
7470
inline std::size_t page_size() {
75-
static const std::size_t ps = []() {
71+
static const std::size_t page_size_value = []() {
7672
#if defined(_WIN32)
7773
SYSTEM_INFO sysInfo;
7874
GetSystemInfo(&sysInfo);
7975
return static_cast<std::size_t>(sysInfo.dwPageSize);
8076
#elif defined(__unix__) || defined(__APPLE__)
81-
long ps = sysconf(_SC_PAGESIZE);
82-
return ps > 0 ? static_cast<std::size_t>(ps) : 4096;
77+
long queried_page_size = sysconf(_SC_PAGESIZE);
78+
return queried_page_size > 0 ? static_cast<std::size_t>(queried_page_size) : 4096;
8379
#else
8480
return 4096; // Fallback
8581
#endif
8682
}();
87-
return ps;
83+
return page_size_value;
8884
}
8985

9086
/**
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
#!/usr/bin/env python3
2+
import subprocess
3+
import sys
4+
import tempfile
5+
import unittest
6+
from pathlib import Path
7+
8+
9+
PROJECT_ROOT = Path(__file__).resolve().parents[2]
10+
11+
12+
class BuildConfigurationTest(unittest.TestCase):
13+
def run_configure(self, *cmake_args: str) -> subprocess.CompletedProcess[str]:
14+
with tempfile.TemporaryDirectory() as tmpdir:
15+
build_dir = Path(tmpdir) / "build"
16+
return subprocess.run(
17+
[
18+
"cmake",
19+
"-S",
20+
str(PROJECT_ROOT),
21+
"-B",
22+
str(build_dir),
23+
"-G",
24+
"Ninja",
25+
*cmake_args,
26+
],
27+
capture_output=True,
28+
text=True,
29+
check=False,
30+
)
31+
32+
def run_configure_and_build(
33+
self,
34+
target: str,
35+
*cmake_args: str,
36+
) -> subprocess.CompletedProcess[str]:
37+
with tempfile.TemporaryDirectory() as tmpdir:
38+
build_dir = Path(tmpdir) / "build"
39+
configure = subprocess.run(
40+
[
41+
"cmake",
42+
"-S",
43+
str(PROJECT_ROOT),
44+
"-B",
45+
str(build_dir),
46+
"-G",
47+
"Ninja",
48+
*cmake_args,
49+
],
50+
capture_output=True,
51+
text=True,
52+
check=False,
53+
)
54+
if configure.returncode != 0:
55+
return configure
56+
57+
return subprocess.run(
58+
[
59+
"cmake",
60+
"--build",
61+
str(build_dir),
62+
"--target",
63+
target,
64+
"-j2",
65+
],
66+
capture_output=True,
67+
text=True,
68+
check=False,
69+
)
70+
71+
def test_configure_succeeds_with_benchmarks_enabled_and_tests_disabled(self):
72+
result = self.run_configure(
73+
"-DCMAKE_BUILD_TYPE=Debug",
74+
"-DHPC_BUILD_TESTS=OFF",
75+
"-DHPC_BUILD_BENCHMARKS=ON",
76+
)
77+
78+
self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr)
79+
80+
def test_auto_vectorize_build_does_not_ignore_omp_simd_pragmas(self):
81+
result = self.run_configure_and_build(
82+
"auto_vectorize",
83+
"-DCMAKE_BUILD_TYPE=Debug",
84+
"-DHPC_BUILD_TESTS=OFF",
85+
"-DHPC_BUILD_BENCHMARKS=OFF",
86+
)
87+
88+
self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr)
89+
combined_output = result.stdout + "\n" + result.stderr
90+
self.assertNotIn("ignoring '#pragma omp simd'", combined_output, msg=combined_output)
91+
92+
def test_core_test_build_avoids_core_header_warnings(self):
93+
result = self.run_configure_and_build(
94+
"core_test",
95+
"-DCMAKE_BUILD_TYPE=Debug",
96+
"-DHPC_BUILD_TESTS=ON",
97+
"-DHPC_BUILD_BENCHMARKS=OFF",
98+
)
99+
100+
self.assertEqual(result.returncode, 0, msg=result.stdout + "\n" + result.stderr)
101+
combined_output = result.stdout + "\n" + result.stderr
102+
self.assertNotIn("-Winterference-size", combined_output, msg=combined_output)
103+
self.assertNotIn("shadows a previous local", combined_output, msg=combined_output)
104+
105+
106+
if __name__ == "__main__":
107+
unittest.main()

tests/integration/compare_benchmarks_test.py

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,27 +8,31 @@
88

99

1010
PROJECT_ROOT = Path(__file__).resolve().parents[2]
11-
SCRIPT_PATH = PROJECT_ROOT / "scripts" / "compare_benchmarks.py"
11+
SCRIPT_PATH = PROJECT_ROOT / "tools" / "performance" / "benchmark_compare.py"
1212

1313

1414
class CompareBenchmarksScriptTest(unittest.TestCase):
15-
def run_script(self, baseline_payload, candidate_payload, threshold=10):
15+
def run_script(self, baseline_payload, candidate_payload, threshold=0.1, fail_on_regression=False):
1616
with tempfile.TemporaryDirectory() as tmpdir:
1717
tmp_path = Path(tmpdir)
1818
baseline_path = tmp_path / "baseline.json"
1919
candidate_path = tmp_path / "candidate.json"
2020
baseline_path.write_text(json.dumps(baseline_payload), encoding="utf-8")
2121
candidate_path.write_text(json.dumps(candidate_payload), encoding="utf-8")
2222

23+
args = [
24+
sys.executable,
25+
str(SCRIPT_PATH),
26+
str(baseline_path),
27+
str(candidate_path),
28+
"--threshold",
29+
str(threshold),
30+
]
31+
if fail_on_regression:
32+
args.append("--fail-on-regression")
33+
2334
return subprocess.run(
24-
[
25-
sys.executable,
26-
str(SCRIPT_PATH),
27-
str(baseline_path),
28-
str(candidate_path),
29-
"--threshold",
30-
str(threshold),
31-
],
35+
args,
3236
capture_output=True,
3337
text=True,
3438
check=False,
@@ -46,11 +50,11 @@ def test_exit_zero_when_all_benchmarks_stay_within_threshold(self):
4650
]
4751
}
4852

49-
result = self.run_script(baseline, candidate, threshold=10)
53+
result = self.run_script(baseline, candidate, threshold=0.10)
5054

5155
self.assertEqual(result.returncode, 0, msg=result.stderr or result.stdout)
5256
self.assertIn("BM_AddArrays/1024", result.stdout)
53-
self.assertIn("+8.00%", result.stdout)
57+
self.assertIn("+8.0%", result.stdout)
5458

5559
def test_exit_one_when_any_benchmark_regresses_beyond_threshold(self):
5660
baseline = {
@@ -64,11 +68,16 @@ def test_exit_one_when_any_benchmark_regresses_beyond_threshold(self):
6468
]
6569
}
6670

67-
result = self.run_script(baseline, candidate, threshold=10)
71+
result = self.run_script(
72+
baseline,
73+
candidate,
74+
threshold=0.10,
75+
fail_on_regression=True,
76+
)
6877

6978
self.assertEqual(result.returncode, 1, msg=result.stderr or result.stdout)
70-
self.assertIn("Regression detected", result.stdout)
71-
self.assertIn("+20.00%", result.stdout)
79+
self.assertIn("regression(s) detected", result.stdout)
80+
self.assertIn("+20.0%", result.stdout)
7281

7382

7483
if __name__ == "__main__":

tests/unit/memory/memory_utils_test.cpp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,18 @@ using hpc::core::PAGE_SIZE;
1212

1313
namespace hpc::memory::test {
1414

15+
namespace {
16+
17+
struct TrackedObject {
18+
inline static int constructions = 0;
19+
inline static int destructions = 0;
20+
21+
TrackedObject() { ++constructions; }
22+
~TrackedObject() { ++destructions; }
23+
};
24+
25+
} // namespace
26+
1527
// ---------------------------------------------------------------------------
1628
// AlignedAllocator tests
1729
// ---------------------------------------------------------------------------
@@ -62,6 +74,20 @@ TEST(AlignedAllocTest, MakeAlignedUniquePtr) {
6274
EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr.get()) % CACHE_LINE_SIZE, 0u);
6375
}
6476

77+
TEST(AlignedAllocTest, MakeAlignedConstructsAndDestroysObjects) {
78+
TrackedObject::constructions = 0;
79+
TrackedObject::destructions = 0;
80+
81+
{
82+
auto ptr = make_aligned<TrackedObject>(3, CACHE_LINE_SIZE);
83+
ASSERT_NE(ptr.get(), nullptr);
84+
EXPECT_EQ(TrackedObject::constructions, 3);
85+
EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr.get()) % CACHE_LINE_SIZE, 0u);
86+
}
87+
88+
EXPECT_EQ(TrackedObject::destructions, 3);
89+
}
90+
6591
// ---------------------------------------------------------------------------
6692
// Constants tests
6793
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)