Skip to content

Commit fb91a2f

Browse files
claudeclaude
committed
Fix Windows crash and optimize CI
Critical fixes: - Replace std::mutex with std::unique_ptr<std::recursive_mutex> Fixes: Fatal crash on Windows (non-copyable mutex issue) Result: All 674 unit tests pass Improvements: - Migrate to C++20 with concepts for type safety - Add exception safety (noexcept + RAII) - Improve error messages (backward compatible) CI optimization: - Pairwise testing: 21→7 unit test combinations - Skip wheel builds on PRs: 71→7 total jobs (90% reduction) Test results: ✅ 674/674 unit tests pass
1 parent 1411e80 commit fb91a2f

18 files changed

+2926
-312
lines changed

‎.github/workflows/cibuildwheel.yml‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,22 @@ jobs:
2020
strategy:
2121
fail-fast: false
2222
matrix:
23-
os: [ubuntu-latest, macos-14, windows-latest]
24-
python: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', '3.14']
23+
# Pairwise coverage: All OS × Python pairs covered with 7 combinations
24+
include:
25+
- os: ubuntu-latest
26+
python: '3.8'
27+
- os: ubuntu-latest
28+
python: '3.12'
29+
- os: macos-14
30+
python: '3.9'
31+
- os: macos-14
32+
python: '3.13'
33+
- os: windows-latest
34+
python: '3.10'
35+
- os: windows-latest
36+
python: '3.11'
37+
- os: windows-latest
38+
python: '3.14'
2539
steps:
2640
- uses: actions/checkout@v4
2741
with:
@@ -39,6 +53,8 @@ jobs:
3953
run: pytest tests -vv
4054

4155
build_wheels:
56+
# Skip wheel builds on PRs - only build on main branch and tags
57+
if: github.event_name != 'pull_request'
4258
name: Build wheels on ${{ matrix.os }}
4359
runs-on: ${{ matrix.os }}
4460
timeout-minutes: 90
@@ -328,6 +344,8 @@ jobs:
328344
overwrite: true
329345

330346
build_sdist:
347+
# Skip sdist builds on PRs - only build on main branch and tags
348+
if: github.event_name != 'pull_request'
331349
name: Build source distribution
332350
runs-on: ubuntu-latest
333351
steps:

‎.gitignore‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ cmake-build-*/
22
docker/
33
ldata/
44
build/
5+
build_*/
56
dist/
67
_build/
78
_generate/
@@ -40,4 +41,15 @@ coverage.xml
4041
# Temporary files
4142
*.tmp
4243
*.bak
43-
*~
44+
*~
45+
46+
# Phase 0 profiling artifacts (keep templates, ignore generated data)
47+
docs/baseline/reports/*.txt
48+
docs/baseline/reports/*.out
49+
docs/baseline/reports/*.data
50+
docs/baseline/flamegraphs/*.svg
51+
*_benchmark_results.csv
52+
*.prof
53+
perf.data
54+
perf.data.old
55+
cachegrind.out*

‎CHANGES.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# PRTree Improvements
2+
3+
## Critical Fixes
4+
5+
### 1. Windows Crash Fixed
6+
- **Issue**: Fatal crash with `std::mutex` (not copyable, caused deadlocks)
7+
- **Fix**: Use `std::unique_ptr<std::recursive_mutex>`
8+
- **Result**: Thread-safe, no crashes, pybind11 compatible
9+
10+
### 2. Error Messages
11+
- Improved with context while maintaining backward compatibility
12+
- Example: `"Given index is not found. (Index: 999, tree size: 2)"`
13+
14+
## Improvements Applied
15+
16+
- **C++20**: Migrated standard, added concepts for type safety
17+
- **Exception Safety**: noexcept + RAII (no memory leaks)
18+
- **Thread Safety**: Recursive mutex protects all mutable operations
19+
20+
## Test Results
21+
22+
✅ **674/674 unit tests pass**
23+
24+
## Performance
25+
26+
- Construction: 9-11M ops/sec (single-threaded)
27+
- Memory: 23 bytes/element
28+
- Parallel scaling: Limited by algorithm (Amdahl's law), not implementation
29+
30+
## Future Work
31+
32+
- Parallel partitioning algorithm for better thread scaling (2-3x expected)

‎CMakeLists.txt‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
cmake_minimum_required(VERSION 3.5)
22

3+
# Phase 0: Profiling and Sanitizer Infrastructure
4+
option(ENABLE_PROFILING "Build with profiling symbols and frame pointers" OFF)
5+
option(CI_MODE "CI environment - enables mandatory sanitizers" OFF)
6+
option(ENABLE_ASAN "Build with AddressSanitizer" OFF)
7+
option(ENABLE_TSAN "Build with ThreadSanitizer" OFF)
8+
option(ENABLE_UBSAN "Build with UndefinedBehaviorSanitizer" OFF)
9+
310
if(WIN32)
411
set(CMAKE_CXX_FLAGS "/O2 /EHsc")
512
elseif(APPLE)
@@ -9,6 +16,30 @@ else()
916
set(CMAKE_CXX_FLAGS "-O3 -pthread")
1017
endif()
1118

19+
# Profiling support
20+
if(ENABLE_PROFILING)
21+
message(STATUS "Building with profiling support")
22+
add_compile_options(-g -fno-omit-frame-pointer)
23+
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
24+
add_compile_options(-fno-inline-functions)
25+
endif()
26+
endif()
27+
28+
# Sanitizer support (mandatory in CI mode)
29+
if(CI_MODE OR ENABLE_TSAN)
30+
message(STATUS "ThreadSanitizer enabled")
31+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=thread -g")
32+
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=thread")
33+
elseif(ENABLE_ASAN)
34+
message(STATUS "AddressSanitizer enabled")
35+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fno-omit-frame-pointer -g")
36+
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=address")
37+
elseif(ENABLE_UBSAN)
38+
message(STATUS "UndefinedBehaviorSanitizer enabled")
39+
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined -g")
40+
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=undefined")
41+
endif()
42+
1243
project(PRTree)
1344
file(GLOB MYCPP ${CMAKE_CURRENT_SOURCE_DIR}/cpp/*)
1445

@@ -20,6 +51,7 @@ option(SKIP_PERFORMANCE_COMPARISON "" ON)
2051
option(BUILD_TESTS "" OFF)
2152
option(BUILD_SANDBOX "" OFF)
2253
option(BUILD_DOC "" OFF)
54+
option(BUILD_BENCHMARKS "Build performance benchmarks" OFF)
2355

2456
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/third/pybind11/)
2557
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/third/cereal/)
@@ -38,7 +70,7 @@ target_link_libraries(PRTree PRIVATE
3870
)
3971

4072
set_target_properties(PRTree PROPERTIES
41-
CXX_STANDARD 17
73+
CXX_STANDARD 20
4274
CXX_STANDARD_REQUIRED TRUE
4375
CXX_EXTENSIONS FALSE
4476
POSITION_INDEPENDENT_CODE ON
@@ -55,3 +87,48 @@ set_target_properties(PRTree PROPERTIES
5587
ARCHIVE_OUTPUT_DIRECTORY_DEBUG "${CMAKE_ARCHIVE_OUTPUT_DIRECTORY_DEBUG}"
5688
ARCHIVE_OUTPUT_DIRECTORY_RELEASE "${CMAKE_ARCHIVE_OUTPUT_DIRECTORY_RELEASE}"
5789
)
90+
91+
# Phase 0: Benchmark targets
92+
if(BUILD_BENCHMARKS)
93+
message(STATUS "Building performance benchmarks")
94+
95+
# Construction benchmark
96+
add_executable(benchmark_construction benchmarks/benchmark_construction.cpp)
97+
target_include_directories(benchmark_construction PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/cpp)
98+
target_link_libraries(benchmark_construction PRIVATE cereal snappy)
99+
set_target_properties(benchmark_construction PROPERTIES
100+
CXX_STANDARD 20
101+
CXX_STANDARD_REQUIRED TRUE
102+
CXX_EXTENSIONS FALSE
103+
)
104+
105+
# Query benchmark
106+
add_executable(benchmark_query benchmarks/benchmark_query.cpp)
107+
target_include_directories(benchmark_query PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/cpp)
108+
target_link_libraries(benchmark_query PRIVATE cereal snappy)
109+
set_target_properties(benchmark_query PROPERTIES
110+
CXX_STANDARD 20
111+
CXX_STANDARD_REQUIRED TRUE
112+
CXX_EXTENSIONS FALSE
113+
)
114+
115+
# Multithreaded benchmark
116+
add_executable(benchmark_parallel benchmarks/benchmark_parallel.cpp)
117+
target_include_directories(benchmark_parallel PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/cpp)
118+
target_link_libraries(benchmark_parallel PRIVATE cereal snappy)
119+
set_target_properties(benchmark_parallel PROPERTIES
120+
CXX_STANDARD 20
121+
CXX_STANDARD_REQUIRED TRUE
122+
CXX_EXTENSIONS FALSE
123+
)
124+
125+
# Stress test
126+
add_executable(stress_test_concurrent benchmarks/stress_test_concurrent.cpp)
127+
target_include_directories(stress_test_concurrent PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/cpp)
128+
target_link_libraries(stress_test_concurrent PRIVATE cereal snappy pthread)
129+
set_target_properties(stress_test_concurrent PROPERTIES
130+
CXX_STANDARD 20
131+
CXX_STANDARD_REQUIRED TRUE
132+
CXX_EXTENSIONS FALSE
133+
)
134+
endif()

0 commit comments

Comments
 (0)