Skip to content

Some performance enhancements #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aliddell merged 41 commits into from
Mar 25, 2025
Merged

Some performance enhancements #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
af2df49
Use `ConstByteSpan` in signature of `Sink::write()`.
aliddell Mar 20, 2025
6e22786
Use Windows/POSIX file I/O. Remove `truncate` parameter from FileSink…
aliddell Mar 20, 2025
4f0fa08
Update version.
aliddell Mar 20, 2025
e6fc79b
Precompute values in ArrayDimensions.
aliddell Mar 20, 2025
6b7b924
parfor (sort of) main loop in ArrayWriter::write_frame_to_chunks_().
aliddell Mar 20, 2025
32bcdf1
Release the GIL when calling ZarrStream_append().
aliddell Mar 20, 2025
aa00d26
Use OpenMP (cross-platform issues).
aliddell Mar 20, 2025
65979c9
Enable OpenMP builds on Mac
aliddell Mar 20, 2025
239d8d1
Link omp statically.
aliddell Mar 20, 2025
8293bdf
Update tests to install OpenMP
aliddell Mar 20, 2025
4fab59d
(wip) check something
aliddell Mar 21, 2025
1f36f1d
(wip) check something else
aliddell Mar 21, 2025
98faae2
(wip) redo some calculations
aliddell Mar 21, 2025
c212a5f
(wip) wild theory
aliddell Mar 21, 2025
499d519
Revert "(wip) wild theory"
aliddell Mar 21, 2025
4a9e71e
Remove collapse(2) from omp pragma
aliddell Mar 21, 2025
74aba06
(wip) Use a temporary cleanup fixture between Python tests. (Trying t…
aliddell Mar 21, 2025
2f76347
(wip) Reset OpenMP thread states on ZarrStream_destroy.
aliddell Mar 21, 2025
db2d0e9
(wip) Python bindings -- don't release GIL?
aliddell Mar 21, 2025
2d7c9b4
(wip) Restrict omp threads?
aliddell Mar 21, 2025
d1efad1
(wip) Restrict max threads in broken test.
aliddell Mar 21, 2025
9b0a9f6
Reinstate CI tests.
aliddell Mar 21, 2025
27319bc
clone old vcpkg in CI
aliddell Mar 21, 2025
8bdf70d
Reserve some threads for OpenMP.
aliddell Mar 21, 2025
25f58a4
Distribute threads differently.
aliddell Mar 21, 2025
90fda37
Don't try to account for OMP threads when allocating thread pool.
aliddell Mar 24, 2025
48294fe
Revert "Don't try to account for OMP threads when allocating thread p…
aliddell Mar 24, 2025
090cb0e
Use a single thread in failing test.
aliddell Mar 24, 2025
687efc0
wip (reductio)
aliddell Mar 24, 2025
542cb1e
Revert "wip (reductio)"
aliddell Mar 24, 2025
ce52121
(wip) limit number of omp threads
aliddell Mar 24, 2025
4d95d24
(wip) slightly increase number of omp threads
aliddell Mar 24, 2025
3dda2b4
(wip) Update build CI file.
aliddell Mar 24, 2025
7ac9933
Simplify thread allocation.
aliddell Mar 24, 2025
429dbb9
Use macos-13 instead of macos-latest-large
aliddell Mar 24, 2025
7d47fc1
Update build file.
aliddell Mar 24, 2025
6f7ebe5
Update build file.
aliddell Mar 24, 2025
84ecc27
Update release file.
aliddell Mar 24, 2025
29249ec
Wrap Python version in quotes.
aliddell Mar 24, 2025
c25d599
Merge remote-tracking branch 'upstream/main' into os-specific-file-sink
aliddell Mar 25, 2025
bf1bc86
Respond to PR comments.
aliddell Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 18 additions & 61 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
- "main"

jobs:
windows-and-linux-build:
build:
name: Build on ${{ matrix.platform }} with ${{ matrix.build_type }} configuration
strategy:
matrix:
Expand All @@ -16,11 +16,17 @@ jobs:
platform:
- "windows-latest"
- "ubuntu-latest"
- "macos-latest" # arm
- "macos-13" # x86_64
include:
- platform: "windows-latest"
vcpkg_triplet: "x64-windows-static"
- platform: "ubuntu-latest"
vcpkg_triplet: "x64-linux"
- platform: "macos-latest"
vcpkg_triplet: "arm64-osx"
- platform: "macos-13"
vcpkg_triplet: "x64-osx"

runs-on: ${{ matrix.platform }}

Expand All @@ -38,13 +44,18 @@ jobs:

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
Copy link
Copy Markdown
Collaborator

@shlomnissan shlomnissan Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity — what’s the reason for tagging vcpkg here?

Copy link
Copy Markdown
Member Author

@aliddell aliddell Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into an issue in CI on Friday afternoon which was introduced with this and apparently resolved with this. I decided to tag a known working version so this doesn't happen again.

cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Install OpenMP
Copy link
Copy Markdown
Collaborator

@shlomnissan shlomnissan Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend renaming this task to "Install OpenMP on macOS" to make the platform-specific purpose clearer. The same comment applies to all instances in this PR.

if: matrix.platform == 'macos-latest' || matrix.platform == 'macos-13'
run: |
brew install libomp

- name: CMake
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=${{matrix.vcpkg_triplet}}
Expand All @@ -57,62 +68,7 @@ jobs:

- uses: actions/upload-artifact@v4
with:
name: ${{matrix.platform}} ${{matrix.build_type}} binaries
path: ${{github.workspace}}/*.zip

mac-build:
strategy:
matrix:
build_type:
- "Debug"
- "Release"

runs-on: "macos-latest"

permissions:
actions: write

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-macos-latest-${{ matrix.build_type }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Build for x64
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=x64-osx -DVCPKG_INSTALLED_DIR=${{github.workspace}}/vcpkg-x64 -B ${{github.workspace}}/build-x64 -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DCMAKE_OSX_ARCHITECTURES="x86_64" -DBUILD_TESTING=OFF
cmake --build ${{github.workspace}}/build-x64 --config ${{matrix.build_type}}

- name: Build for arm64
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=arm64-osx -DVCPKG_INSTALLED_DIR=${{github.workspace}}/vcpkg-arm64 -B ${{github.workspace}}/build-arm64 -DCMAKE_BUILD_TYPE=${{matrix.build_type}} -DCMAKE_OSX_ARCHITECTURES="arm64" -DBUILD_TESTING=OFF
cmake --build ${{github.workspace}}/build-arm64 --config ${{matrix.build_type}}

- name: Create a universal binary
run: |
cp -r ${{github.workspace}}/build-x64 ${{github.workspace}}/build && cd ${{github.workspace}}/build
for filename in $(find . -type f -exec grep -H "build-x64" {} \; | awk '{print $1}' | sed -e 's/:.*//' | sort -u); do sed -i.bak -e "s/build-x64/build/g" $filename && rm ${filename}.bak; done
for lib in `find . -type f \( -name "*.so" -o -name "*.a" \)`; do rm $lib && lipo -create ../build-x64/${lib} ../build-arm64/${lib} -output $lib; done

- name: Package
run: |
cpack --config ${{github.workspace}}/build/CPackConfig.cmake -C ${{matrix.build_type}} -G ZIP

- uses: actions/upload-artifact@v4
with:
name: macos-latest ${{matrix.build_type}} binaries
name: ${{matrix.platform == 'macos-latest' && 'macos-arm64' || (matrix.platform == 'macos-13' && 'macos-x86_64' || matrix.platform)}} ${{matrix.build_type}} binaries
path: ${{github.workspace}}/*.zip

build-wheel:
Expand All @@ -121,7 +77,8 @@ jobs:
platform:
- "windows-latest"
- "ubuntu-22.04"
- "macos-latest" # TODO (aliddell): universal binary?
- "macos-latest" # arm
- "macos-13" # x86_64

runs-on: ${{ matrix.platform }}

Expand All @@ -140,11 +97,11 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: "3.10"
python-version: "3.13"

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
Expand Down
82 changes: 22 additions & 60 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,24 @@ env:
BUILD_TYPE: Release

jobs:
windows-and-linux-build:
build:
name: Build on ${{ matrix.platform }}
strategy:
matrix:
platform:
- "windows-latest"
- "ubuntu-latest"
- "macos-latest"
- "macos-13"
include:
- platform: "windows-latest"
vcpkg_triplet: "x64-windows-static"
- platform: "ubuntu-latest"
vcpkg_triplet: "x64-linux"
- platform: "macos-latest"
vcpkg_triplet: "arm64-osx"
- platform: "macos-13"
vcpkg_triplet: "x64-osx"

runs-on: ${{ matrix.platform }}

Expand All @@ -39,13 +45,18 @@ jobs:

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
Copy link
Copy Markdown
Collaborator

@shlomnissan shlomnissan Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we currently caching any of the vcpkg build artifacts? I believe GitHub Actions supports this pretty easily. If we’re not caching them yet, it might be worth opening an issue to track — could be a nice win for reducing CI/CD runtime.

Copy link
Copy Markdown
Member Author

@aliddell aliddell Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not something I've ever looked into, but if if could speed up builds that would be a huge win.

cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Install OpenMP
if: matrix.platform == 'macos-latest' || matrix.platform == 'macos-13'
run: |
brew install libomp

- name: Build
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=${{matrix.vcpkg_triplet}} -B ${{github.workspace}}/build -DCMAKE_BUILD_TYPE=Release
Expand All @@ -66,68 +77,15 @@ jobs:
name: ${{matrix.platform}} binaries
path: ${{github.workspace}}/*.zip

mac-build:
name: Build on macos-latest
runs-on: "macos-latest"

permissions:
actions: write

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.platform }}
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}

steps:
- uses: actions/checkout@v3
with:
submodules: true

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Build for x64
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=x64-osx -DVCPKG_INSTALLED_DIR=${{github.workspace}}/vcpkg-x64 -B ${{github.workspace}}/build-x64 -DCMAKE_BUILD_TYPE=Release -DCMAKE_OSX_ARCHITECTURES="x86_64" -DBUILD_TESTING=OFF
cmake --build ${{github.workspace}}/build-x64 --config Release

- name: Build for arm64
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=arm64-osx -DVCPKG_INSTALLED_DIR=${{github.workspace}}/vcpkg-arm64 -B ${{github.workspace}}/build-arm64 -DCMAKE_BUILD_TYPE=Release -DCMAKE_OSX_ARCHITECTURES="arm64" -DBUILD_TESTING=OFF
cmake --build ${{github.workspace}}/build-arm64 --config Release

- name: Test # don't release if tests are failing
working-directory: ${{github.workspace}}/build-arm64
run: ctest -C Release -L anyplatform --output-on-failure

- name: Create a universal binary
run: |
cp -r ${{github.workspace}}/build-x64 ${{github.workspace}}/build && cd ${{github.workspace}}/build
for filename in $(find . -type f -exec grep -H "build-x64" {} \; | awk '{print $1}' | sed -e 's/:.*//' | sort -u); do sed -i.bak -e "s/build-x64/build/g" $filename && rm ${filename}.bak; done
for lib in `find . -type f \( -name "*.so" -o -name "*.a" \)`; do rm $lib && lipo -create ../build-x64/${lib} ../build-arm64/${lib} -output $lib; done

- name: Package
run: |
cpack --config ${{github.workspace}}/build/CPackConfig.cmake -C Release -G ZIP

- uses: actions/upload-artifact@v4
with:
name: macos-latest binaries
path: ${{github.workspace}}/*.zip

build-wheel:
name: Build wheels for Python
strategy:
matrix:
platform:
- "windows-latest"
- "ubuntu-22.04"
- "macos-latest" # TODO (aliddell): universal binary?
- "macos-latest" # arm
- "macos-13" # x86_64
python:
- "3.9"
- "3.10"
Expand Down Expand Up @@ -156,13 +114,18 @@ jobs:

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Install OpenMP
if: matrix.platform == 'macos-latest' || matrix.platform == 'macos-13'
run: |
brew install libomp

- name: Install system dependencies
if: ${{ matrix.platform == 'ubuntu-22.04' }}
run: |
Expand Down Expand Up @@ -190,8 +153,7 @@ jobs:

release:
needs:
- windows-and-linux-build
- mac-build
- build
- build-wheel
name: "Release"
runs-on: "ubuntu-latest"
Expand Down
20 changes: 15 additions & 5 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,18 @@ jobs:

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Install OpenMP
if: matrix.platform == 'macos-latest'
run: |
brew install libomp

- name: Configure CMake
run: |
cmake --preset=default -DVCPKG_TARGET_TRIPLET=${{matrix.vcpkg_triplet}}
Expand Down Expand Up @@ -104,7 +109,7 @@ jobs:

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
Expand Down Expand Up @@ -155,20 +160,25 @@ jobs:
submodules: true
ref: ${{ github.event.pull_request.head.sha }}

- name: Set up Python 3.11
- name: Set up Python 3.13
uses: actions/setup-python@v4
with:
python-version: "3.11"
python-version: "3.13"

- name: Install vcpkg
run: |
git clone https://github.com/microsoft/vcpkg.git
git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14
cd vcpkg && ./bootstrap-vcpkg.sh
echo "VCPKG_ROOT=${{github.workspace}}/vcpkg" >> $GITHUB_ENV
echo "${{github.workspace}}/vcpkg" >> $GITHUB_PATH
./vcpkg integrate install
shell: bash

- name: Install OpenMP
if: matrix.platform == 'macos-latest'
run: |
brew install libomp

- name: Install dependencies
run: python -m pip install -U pip "pybind11[global]" cmake build numpy pytest

Expand Down
12 changes: 7 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,19 @@ cmake_policy(SET CMP0057 NEW) # allows IN_LIST operator (for pybind11)
cmake_policy(SET CMP0079 NEW) # allows use with targets in other directories
enable_testing()

find_package(nlohmann_json CONFIG REQUIRED)
find_package(blosc CONFIG REQUIRED)
find_package(miniocpp CONFIG REQUIRED)
find_package(Crc32c CONFIG REQUIRED)

include(cmake/aq_require.cmake)
include(cmake/git-versioning.cmake)
include(cmake/ide.cmake)
include(cmake/install-prefix.cmake)
include(cmake/wsl.cmake)
include(cmake/simd.cmake)
include(cmake/openmp.cmake)

find_package(nlohmann_json CONFIG REQUIRED)
find_package(blosc CONFIG REQUIRED)
find_package(miniocpp CONFIG REQUIRED)
find_package(Crc32c CONFIG REQUIRED)
find_package(OpenMP REQUIRED)
Comment on lines +15 to +19
Copy link
Copy Markdown
Collaborator

@shlomnissan shlomnissan Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have multiple targets using these packages? If they’re only needed for something specific like streaming or testing, it might make sense to move the find_package calls closer to where they’re actually used, rather than keeping them at the top level.


set(CMAKE_C_STANDARD 11)
set(CMAKE_CXX_STANDARD 20)
Expand Down
6 changes: 6 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ EOF
If you're using Windows, learn how to set environment variables [here](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-7.4#set-environment-variables-in-the-system-control-panel).
You will need to set both the `VCPKG_ROOT` and `PATH` variables in the system control panel.

On the Mac, you will also need to install OpenMP using Homebrew:

```bash
brew install libomp
```

### Configuring

To build the library, you can use CMake:
Expand Down
Loading