Some performance enhancements by aliddell · Pull Request #72 · acquire-project/acquire-zarr

aliddell · 2025-03-20T19:36:26Z

Uses platform-specific file I/O. Before the other interventions, I found this modestly improved performance of file I/O on all machines I tested on.
Precomputes several values which are often queried from ArrayDimension.
Uses OpenMP and wraps the main for loop in ArrayWriter::write_frame_to_chunks_() in a #pragma omp parallel for.

… constructor.

aliddell · 2025-03-20T19:38:31Z

I still need to update the CI build and test scripts because the Mac (as per) needs some more TLC to get it right.

This reverts commit c212a5f.

…o see something.)

…ool." This reverts commit 90fda37.

nclack

Left some comments. Be sure to break the performance improvements into multiple pr's if you can.

nclack · 2025-03-24T14:09:22Z

src/streaming/array.writer.cpp

+    size_t bytes_written = 0;
+    const auto n_tiles = n_tiles_x * n_tiles_y;
+
+#pragma omp parallel for reduction(+ : bytes_written)


Check this with a benchmark, but this will scale better if you make bytes_written an atomic and a plain old parallel for.

Most of the work here is not involved with the reduction, and the way reductions get parallelized might be limiting.

Maybe omp figures this out for you, but it's better not to rely on that.

I actually see significant improvement with the reduction.

atomic: 1.081 GiB/s

reduction: 1.354 GiB/s

I ran multiple times and saw similar numbers.

nclack · 2025-03-24T14:13:48Z

src/streaming/file.sink.cpp

-  : file_(filename.data(),
-          truncate ? (std::ios::binary | std::ios::trunc) : std::ios::binary)
+namespace {
+#ifdef _WIN32


If there's a way to factor out the platform-dependent things into their own source files, it makes the code much easier to read and maintain.

nclack · 2025-03-24T14:17:42Z

src/streaming/zarr.stream.cpp

 #undef min
 #endif

+#ifdef max


You might be able to just say #undef without needing the #ifdef.

More importantly, where are these symbols leaking in from? If possible you should handle this as close to the source as possible.

This reverts commit 687efc0.

jeskesen

Trying to remove my "approve", because I hit it accidentally before.

Accidental approval

shlomnissan

I’ve flagged one potential bug — it looks like the wrong pointer might be getting deleted, so I’m requesting changes just to ensure that’s double-checked.

Beyond that, I think there’s a lot of room to improve the robustness and readability of the code. The manual pointer management, in particular, feels risky and could lead to maintenance issues. If possible, I'd recommend avoiding raw pointers in favor of safer patterns.

That said, the rest of my comments are just suggestions — feel free to weigh them against your judgment.

cmake/macos-toolchain.cmake

shlomnissan · 2025-03-21T17:58:02Z

cmake/macos-toolchain.cmake

+        # Apple Silicon
+        set(LIBOMP_PATH "/opt/homebrew/opt/libomp")
+        set(CMAKE_OSX_ARCHITECTURES "arm64")
+    else ()


This might be overkill since it handles 99% of cases, but if you want to be defensive, you could improve this by:

Making the architecture checks case-insensitive

Adding a fallback for unknown architectures (e.g. older or future variants)

if(CMAKE_HOST_SYSTEM_PROCESSOR MATCHES "^[Aa][Rr][Mm]64") set(ARCH "arm64") set(LIBOMP_PATH "/opt/homebrew/opt/libomp") elseif(CMAKE_HOST_SYSTEM_PROCESSOR MATCHES "^[Xx]86_64") set(ARCH "x86_64") set(LIBOMP_PATH "/usr/local/opt/libomp") else() message(WARNING "Unknown architecture: ${CMAKE_HOST_SYSTEM_PROCESSOR}") set(ARCH "${CMAKE_HOST_SYSTEM_PROCESSOR}") endif()

shlomnissan · 2025-03-21T17:59:10Z

cmake/macos-toolchain.cmake

+    endif ()
+
+    # OpenMP support
+    set(OpenMP_C_FLAGS "-Xclang -fopenmp -I${LIBOMP_PATH}/include")


Is OpenMP being used in the C code as well?

Not at this point, but in case any is added down the line I don't want the configuration to break inexplicably for future users.

shlomnissan · 2025-03-24T20:01:37Z

.github/workflows/build.yml

      - name: Install vcpkg
        run: |
-          git clone https://github.com/microsoft/vcpkg.git
+          git clone https://github.com/microsoft/vcpkg.git -b 2025.02.14


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

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.

shlomnissan · 2025-03-24T20:02:16Z

.github/workflows/build.yml

          ./vcpkg integrate install
        shell: bash

+      - name: Install OpenMP


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.

src/streaming/file.sink.cpp

shlomnissan · 2025-03-24T21:23:57Z

src/streaming/file.sink.cpp

-  : file_(filename.data(),
-          truncate ? (std::ios::binary | std::ios::trunc) : std::ios::binary)
+namespace {
+#ifdef _WIN32


src/streaming/zarr.dimension.cpp

aliddell · 2025-03-25T14:43:54Z

Thanks for comments @nclack @shlomnissan. I've made the following changes:

Renamed macos-toolchain.cmake to openmp.cmake.
Added instructions to the README for Mac users to install OpenMP with Homebrew.
Broken out Windows and POSIX implementations for file.sink.cpp.
Added a comment above #pragma omp parallel for describing why we only use 75% of the thread pool allotment for OMP parallelism. (Will make an issue for this once it's merged.)
Updated the interior of the parfor loop to have some more descriptive variable names.
- Added some more verbiage to help with debugging in the case of buffer overrun.
Used auto and static_cast<T> a bit more liberally in the file I/O code.
Fixed the bug Shlomi found re: deleting the wrong pointer
Consolidated a few for loops in the ArrayDimension constructor.

shlomnissan

Unblocking now that the bug is fixed.

aliddell added 9 commits March 19, 2025 21:14

Use ConstByteSpan in signature of Sink::write().

af2df49

Use Windows/POSIX file I/O. Remove truncate parameter from FileSink…

6e22786

… constructor.

Update version.

4f0fa08

Precompute values in ArrayDimensions.

e6fc79b

parfor (sort of) main loop in ArrayWriter::write_frame_to_chunks_().

6b7b924

Release the GIL when calling ZarrStream_append().

32bcdf1

Use OpenMP (cross-platform issues).

aa00d26

Enable OpenMP builds on Mac

65979c9

Link omp statically.

239d8d1

aliddell requested review from jeskesen and shlomnissan March 20, 2025 19:36

aliddell added 3 commits March 20, 2025 16:26

Update tests to install OpenMP

8293bdf

(wip) check something

4fab59d

(wip) check something else

1f36f1d

aliddell force-pushed the os-specific-file-sink branch from 1377afa to 1f36f1d Compare March 21, 2025 15:04

aliddell added 6 commits March 21, 2025 11:54

(wip) redo some calculations

98faae2

(wip) wild theory

c212a5f

Revert "(wip) wild theory"

499d519

This reverts commit c212a5f.

Remove collapse(2) from omp pragma

4a9e71e

(wip) Use a temporary cleanup fixture between Python tests. (Trying t…

74aba06

…o see something.)

(wip) Reset OpenMP thread states on ZarrStream_destroy.

2f76347

aliddell marked this pull request as draft March 21, 2025 18:07

aliddell added 6 commits March 21, 2025 16:17

(wip) Python bindings -- don't release GIL?

db2d0e9

(wip) Restrict omp threads?

2d7c9b4

(wip) Restrict max threads in broken test.

d1efad1

Reinstate CI tests.

9b0a9f6

clone old vcpkg in CI

27319bc

Reserve some threads for OpenMP.

8bdf70d

aliddell force-pushed the os-specific-file-sink branch from 73ecdc6 to 8bdf70d Compare March 21, 2025 22:36

aliddell added 3 commits March 24, 2025 10:08

Don't try to account for OMP threads when allocating thread pool.

90fda37

Revert "Don't try to account for OMP threads when allocating thread p…

48294fe

…ool." This reverts commit 90fda37.

Use a single thread in failing test.

090cb0e

nclack reviewed Mar 24, 2025

View reviewed changes

aliddell added 3 commits March 24, 2025 11:45

wip (reductio)

687efc0

Revert "wip (reductio)"

542cb1e

This reverts commit 687efc0.

(wip) limit number of omp threads

ce52121

aliddell force-pushed the os-specific-file-sink branch from 79bf22c to ce52121 Compare March 24, 2025 16:57

(wip) slightly increase number of omp threads

4d95d24

jeskesen previously approved these changes Mar 24, 2025

View reviewed changes

aliddell added 6 commits March 24, 2025 14:15

(wip) Update build CI file.

3dda2b4

Simplify thread allocation.

7ac9933

Use macos-13 instead of macos-latest-large

429dbb9

Update build file.

7d47fc1

Update build file.

6f7ebe5

Update release file.

84ecc27

aliddell marked this pull request as ready for review March 24, 2025 18:49

jeskesen reviewed Mar 24, 2025

View reviewed changes

jeskesen self-requested a review March 24, 2025 18:57

Wrap Python version in quotes.

29249ec

shlomnissan requested changes Mar 24, 2025

View reviewed changes

aliddell added 2 commits March 25, 2025 09:04

Merge remote-tracking branch 'upstream/main' into os-specific-file-sink

c25d599

Respond to PR comments.

bf1bc86

aliddell force-pushed the os-specific-file-sink branch from 248a378 to bf1bc86 Compare March 25, 2025 14:41

aliddell requested a review from shlomnissan March 25, 2025 15:48

shlomnissan approved these changes Mar 25, 2025

View reviewed changes

aliddell merged commit fafffc3 into main Mar 25, 2025
7 checks passed

aliddell deleted the os-specific-file-sink branch March 25, 2025 17:18

Conversation

aliddell commented Mar 20, 2025

Uh oh!

aliddell commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nclack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeskesen left a comment

Choose a reason for hiding this comment

Uh oh!

shlomnissan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aliddell commented Mar 25, 2025

Uh oh!

shlomnissan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aliddell commented Mar 20, 2025 •

edited

Loading