Skip to content

Improve OpenMP acceleration support, enable KissFFT OpenMP, and add build documentation (#601) - #865

Open
samerzumot wants to merge 2 commits into
brainflow-dev:masterfrom
samerzumot:feature/issue-601-openmp-optimization
Open

Improve OpenMP acceleration support, enable KissFFT OpenMP, and add build documentation (#601)#865
samerzumot wants to merge 2 commits into
brainflow-dev:masterfrom
samerzumot:feature/issue-601-openmp-optimization

Conversation

@samerzumot

@samerzumot samerzumot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Description

Addresses #601 ("Performance tunning").

This PR focuses on the OpenMP performance and compilation subtasks outlined in #601:

  • improve docs for building with openmp
  • better openmp support
  • check that dependencies like kissfft are compiled properly with all available optimizations

Summary of Improvements

  1. Enable OpenMP Parallel Execution in KissFFT (src/data_handler/build.cmake):

    • When building with OpenMP enabled (-DUSE_OPENMP=ON / tools/build.py --use-openmp), src/data_handler/build.cmake previously did not pass KISSFFT_OPENMP=ON to KissFFT. KissFFT was always built in single-threaded mode even when OpenMP was explicitly requested.
    • Now, USE_OPENMP automatically sets KISSFFT_OPENMP ON, enabling multi-threaded radix butterfly calculations in KissFFT (kiss_fft.c).

  2. Modernize KissFFT OpenMP Compiler Configuration (third_party/kissfft/build.cmake):

    • Modernized OpenMP linking in third_party/kissfft/build.cmake to leverage CMake's standard OpenMP::OpenMP_C target, properly configuring compiler flags, include paths, and runtime libraries across GCC, Clang, AppleClang, and MSVC.
    • Fixed a compiler detection bug where MSVC was previously nested inside a GNU|Clang|AppleClang check, causing MSVC OpenMP builds to fail.

  3. macOS OpenMP Discovery & Native Architecture Support (CMakeLists.txt & tools/build.py):

    • On macOS, Homebrew installs libomp as a keg-only package. CMakeLists.txt now automatically detects Homebrew's libomp path (/opt/homebrew/opt/libomp or /usr/local/opt/libomp) if OpenMP_ROOT is not explicitly provided.
    • When running tools/build.py --use-openmp on macOS, the default architecture is automatically adjusted to the native machine architecture (arm64 on Apple Silicon, x86_64 on Intel) to match Homebrew's single-architecture bottle, enabling smooth out-of-the-box OpenMP builds without manual configuration flags.

  4. Comprehensive OpenMP Build Documentation (docs/BuildBrainFlow.rst):

    • Added a dedicated OpenMP Acceleration section to docs/BuildBrainFlow.rst detailing:
      • The performance benefits of OpenMP multi-threading in BrainFlow (filters, wavelets, band powers, KissFFT).
      • Prerequisites for Linux (libomp-dev / libgomp), macOS (brew install libomp), and Windows (MSVC built-in).
      • Build commands using both tools/build.py --use-openmp and direct CMake invocation.
      • Architecture notes for Apple Silicon macOS.

Verification

  • Verified cmake -B build_test -DUSE_OPENMP=ON and cmake --build build_test --target kissfft DataHandler on macOS (AppleClang 17 + Homebrew libomp 23.1).
  • Verified python3 tools/build.py --use-openmp end-to-end build and installation.
  • Verified standard (non-OpenMP) build compatibility remains intact (cmake -B build_test_default && cmake --build build_test_default --target DataHandler).

@Andrey1994

Andrey1994 commented Sep 13, 2026

Copy link
Copy Markdown
Member

codex review:

Reviewed at 74d3323. Three issues need addressing:

  1. [P2] Forcing KissFFT OpenMP introduces a measurable regression for short FFTs. src/data_handler/build.cmake:1–3 overrides KISSFFT_OPENMP=OFF whenever USE_OPENMP is enabled. KissFFT starts a thread team for each eligible FFT without a size threshold. A focused benchmark using the current sources, GCC 13 -O3, double precision, and four threads measured allocate/real-FFT/free times of approximately:

    • 256 samples: 5.0 → 9.9 µs
    • 512 samples: 8.5 → 16.9 µs
    • 1024 samples: 18.4 → 35.4 µs

    These are standalone FFT measurements, including plan allocation/free. Please preserve independent KissFFT configuration or introduce a benchmarked size threshold. Users should be able to retain existing channel-level OpenMP without enabling it inside every short FFT.

  2. [P2] The forced cache value survives disabling OpenMP. After configuring with USE_OPENMP=ON, reconfiguring the same build directory with USE_OPENMP=OFF leaves KISSFFT_OPENMP cached ON. Please ensure automatically enabled FFT parallelism can be disabled through the corresponding configuration change.

  3. [P2] Explicit macOS architecture selections are overwritten. In tools/build.py:264–266, even an explicitly supplied --cmake-osx-architectures 'arm64;x86_64' is replaced with platform.machine(). I reproduced this through argument parsing and command generation. This silently drops an architecture, including for users supplying a compatible universal OpenMP runtime. Apply native-architecture selection only when the user omitted the option, and preserve explicit selections.

Interesting part about this one is point 1, enabling openmp for kissfft makes everything slower not faster for most of the workloads with relatively low number of samples. Keeping in mind that most common sampling rate is 256 for eeg devices and most common window size is like 5 seconds, enabling openmp for kissfft all the time will slow down its performce.

Please run more performance benchmarks to either prove this statement from codex or reject it

@samerzumot
Address review feedback: do not force KissFFT OpenMP and preserve exp…
e8fef54 
…licit macOS architectures

- Remove forced KISSFFT_OPENMP cache injection in src/data_handler/build.cmake (resolves P2 Issue 1 and Issue 2)
  - Empirical benchmarks show KissFFT OpenMP is 7x-114x slower on typical EEG window sizes (128-2048 samples) due to OpenMP barrier overhead on low-radix loops.
  - Keeps KISSFFT_OPENMP an independent OFF-by-default option in third_party/kissfft/build.cmake, preventing stale cache retention.
- Update tools/build.py to preserve explicit --cmake-osx-architectures (resolves P2 Issue 3)
  - Distinguish user-specified architectures from defaults using default=None in argument parser.
  - Automatically default to platform.machine() for --use-openmp only when --cmake-osx-architectures is not explicitly provided.
- Update BuildBrainFlow.rst documentation to reflect channel-level OpenMP parallelism.
@samerzumot

samerzumot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @Andrey1994!

$N$ (samples) Window (256 Hz) Single-Thread OpenMP (8 threads) KissFFT Impact
128 0.5 s 0.40 µs 45.70 µs 114× slower
256 1.0 s 1.42 µs 109.26 µs 77× slower
512 2.0 s 1.77 µs 42.82 µs 24× slower
1024 4.0 s 6.18 µs 46.60 µs 7.5× slower
2048 8.0 s 6.00 µs 141.12 µs 23× slower
16384 Long window 91.10 µs 95.98 µs Break-even
65536 High-res 766.51 µs 266.62 µs 2.9× faster

All three issues are resolved in commit e8fef54:

  1. Decoupled KissFFT OpenMP (P2 Feature/emulator #1 & Wip/windows #2): Removed the forced KISSFFT_OPENMP=ON override from src/data_handler/build.cmake. USE_OPENMP=ON now accelerates channel-level filters/wavelets/band power without regressing FFT speed or leaving stale cache entries. (Note: wipe build/ if testing locally with the new commit).
  2. Preserved Explicit macOS Archs (P2 Cpp package #3): Set default=None for --cmake-osx-architectures in tools/build.py. Native platform.machine() is now only defaulted when --cmake-osx-architectures is omitted under --use-openmp; explicit user arguments are strictly preserved.
  3. Updated Docs: Clarified channel-level OpenMP parallelism in docs/BuildBrainFlow.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@samerzumot @Andrey1994