From 5f2795e582c4a2396263b234293b97534e1d3f0c Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 20 Nov 2024 11:28:46 -0500 Subject: [PATCH 01/26] Add benchmark to test different chunk/shard/compression/storage configurations --- CMakeLists.txt | 7 + benchmarks/CMakeLists.txt | 18 +++ benchmarks/benchmark.cpp | 233 +++++++++++++++++++++++++++++++++ src/streaming/array.writer.cpp | 1 - 4 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 benchmarks/CMakeLists.txt create mode 100644 benchmarks/benchmark.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 5d877cb..19e97d7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -21,6 +21,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) option(BUILD_PYTHON "Build Python bindings" OFF) +option(BUILD_BENCHMARK "Build benchmarks" OFF) if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) include(CTest) @@ -33,6 +34,12 @@ else () message(STATUS "Skipping test targets") endif () +if (BUILD_BENCHMARK) + add_subdirectory(benchmarks) +else () + message(STATUS "Skipping benchmarks") +endif () + if (${BUILD_PYTHON}) add_subdirectory(python) else () diff --git a/benchmarks/CMakeLists.txt b/benchmarks/CMakeLists.txt new file mode 100644 index 0000000..80fda26 --- /dev/null +++ b/benchmarks/CMakeLists.txt @@ -0,0 +1,18 @@ +set(project acquire-zarr) + +set(tgt acquire-zarr-benchmark) +add_executable(${tgt} benchmark.cpp) +target_compile_definitions(${tgt} PUBLIC "TEST=\"${tgt}\"") +set_target_properties(${tgt} PROPERTIES + MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>" +) +target_include_directories(${tgt} PRIVATE + ${PROJECT_SOURCE_DIR}/include + ${PROJECT_SOURCE_DIR}/src/logger +) +target_link_libraries(${tgt} PRIVATE + acquire-logger + acquire-zarr + nlohmann_json::nlohmann_json + miniocpp::miniocpp +) diff --git a/benchmarks/benchmark.cpp b/benchmarks/benchmark.cpp new file mode 100644 index 0000000..93b1e7d --- /dev/null +++ b/benchmarks/benchmark.cpp @@ -0,0 +1,233 @@ +#include "acquire.zarr.h" +#include +#include +#include +#include +#include +#include +#include + +#define DIM(name_, type_, array_size, chunk_size, shard_size) \ + { .name = (name_), \ + .type = (type_), \ + .array_size_px = (array_size), \ + .chunk_size_px = (chunk_size), \ + .shard_size_chunks = (shard_size) } + +namespace fs = std::filesystem; + +struct ChunkConfig +{ + unsigned int t, c, z, y, x; +}; + +const std::vector CHUNK_CONFIGS = { { 1, 1, 64, 64, 64 }, + { 1, 1, 128, 128, 128 }, + { 1, 1, 256, 256, 256 } }; + +const unsigned int ARRAY_WIDTH = 1920, ARRAY_HEIGHT = 1080, ARRAY_PLANES = 6, + ARRAY_CHANNELS = 3, ARRAY_TIMEPOINTS = 10; + +const unsigned int NUM_RUNS = 1; + +struct BenchmarkConfig +{ + ChunkConfig chunk; + int zarr_version; + std::string compression; + std::string storage; + unsigned int chunks_per_shard_x; + unsigned int chunks_per_shard_y; + std::string s3_endpoint; + std::string s3_bucket; + std::string s3_access_key; + std::string s3_secret_key; +}; + +class Timer +{ + using Clock = std::chrono::high_resolution_clock; + Clock::time_point start; + + public: + Timer() + : start(Clock::now()) + { + } + double elapsed() + { + auto end = Clock::now(); + return std::chrono::duration(end - start).count(); + } +}; + +ZarrStream* +setup_stream(const BenchmarkConfig& config) +{ + ZarrStreamSettings settings = { .store_path = "benchmark.zarr", + .s3_settings = nullptr, + .compression_settings = nullptr, + .data_type = ZarrDataType_uint16, + .version = static_cast( + config.zarr_version) }; + + ZarrCompressionSettings comp_settings = {}; + if (config.compression != "none") { + comp_settings.compressor = ZarrCompressor_Blosc1; + comp_settings.codec = config.compression == "lz4" + ? ZarrCompressionCodec_BloscLZ4 + : ZarrCompressionCodec_BloscZstd; + comp_settings.level = 1; + comp_settings.shuffle = 1; + settings.compression_settings = &comp_settings; + } + + ZarrS3Settings s3_settings = {}; + if (config.storage == "s3") { + s3_settings = { + .endpoint = config.s3_endpoint.c_str(), + .bucket_name = config.s3_bucket.c_str(), + .access_key_id = config.s3_access_key.c_str(), + .secret_access_key = config.s3_secret_key.c_str(), + }; + settings.s3_settings = &s3_settings; + } + + ZarrStreamSettings_create_dimension_array(&settings, 5); + auto* dims = settings.dimensions; + + dims[0] = + DIM("t", ZarrDimensionType_Time, ARRAY_TIMEPOINTS, config.chunk.t, 1); + dims[1] = + DIM("c", ZarrDimensionType_Channel, ARRAY_CHANNELS, config.chunk.c, 1); + dims[2] = + DIM("z", ZarrDimensionType_Space, ARRAY_PLANES, config.chunk.z, 1); + dims[3] = DIM("y", + ZarrDimensionType_Space, + ARRAY_HEIGHT, + config.chunk.y, + config.chunks_per_shard_y); + dims[4] = DIM("x", + ZarrDimensionType_Space, + ARRAY_WIDTH, + config.chunk.x, + config.chunks_per_shard_x); + + return ZarrStream_create(&settings); +} + +double +run_benchmark(const BenchmarkConfig& config) +{ + auto* stream = setup_stream(config); + if (!stream) + return -1.0; + + const size_t frame_size = ARRAY_WIDTH * ARRAY_HEIGHT * sizeof(uint16_t); + std::vector frame(ARRAY_WIDTH * ARRAY_HEIGHT, 0); + const auto num_frames = ARRAY_PLANES * ARRAY_CHANNELS * ARRAY_TIMEPOINTS; + + Timer timer; + size_t bytes_out; + for (int i = 0; i < num_frames; ++i) { + if (ZarrStream_append(stream, frame.data(), frame_size, &bytes_out) != + ZarrStatusCode_Success) { + ZarrStream_destroy(stream); + return -1.0; + } + } + double elapsed = timer.elapsed(); + + ZarrStream_destroy(stream); + if (config.storage == "filesystem") { + fs::remove_all("benchmark.zarr"); + } + return elapsed; +} + +int +main() +{ + std::ofstream csv("zarr_benchmarks.csv"); + csv << "chunk_size,zarr_version,compression,storage,chunks_per_shard_y," + "chunks_per_shard_x,run,time_seconds\n"; + + std::vector configs; + for (const auto& chunk : CHUNK_CONFIGS) { + + // V2 configurations (no sharding) + for (const auto& compression : { "none", "lz4", "zstd" }) { + configs.push_back({ chunk, 2, compression, "filesystem", 1, 1 }); + + if (std::getenv("ZARR_S3_ENDPOINT")) { + configs.push_back({ chunk, + 2, + compression, + "s3", + 1, + 1, + std::getenv("ZARR_S3_ENDPOINT"), + std::getenv("ZARR_S3_BUCKET_NAME"), + std::getenv("ZARR_S3_ACCESS_KEY_ID"), + std::getenv("ZARR_S3_SECRET_ACCESS_KEY") }); + } + } + + unsigned int max_cps_y = (ARRAY_HEIGHT + chunk.y - 1) / chunk.y; + unsigned int max_cps_x = (ARRAY_WIDTH + chunk.x - 1) / chunk.x; + + // V3 configurations (with sharding) + for (unsigned int cps_y = 1; cps_y <= max_cps_y; cps_y *= 2) { + for (unsigned int cps_x = 1; cps_x <= max_cps_x; cps_x *= 2) { + for (const auto& compression : { "none", "lz4", "zstd" }) { + configs.push_back( + { chunk, 3, compression, "filesystem", cps_x, cps_y }); + + if (std::getenv("ZARR_S3_ENDPOINT")) { + configs.push_back( + { chunk, + 3, + compression, + "s3", + cps_x, + cps_y, + std::getenv("ZARR_S3_ENDPOINT"), + std::getenv("ZARR_S3_BUCKET_NAME"), + std::getenv("ZARR_S3_ACCESS_KEY_ID"), + std::getenv("ZARR_S3_SECRET_ACCESS_KEY") }); + } + } + } + } + } + + for (const auto& config : configs) { + std::string chunk_str = std::to_string(config.chunk.t) + "x" + + std::to_string(config.chunk.c) + "x" + + std::to_string(config.chunk.z) + "x" + + std::to_string(config.chunk.y) + "x" + + std::to_string(config.chunk.x); + + for (unsigned int run = 1; run <= NUM_RUNS; ++run) { + std::cout << "Benchmarking " << chunk_str << " Zarr V" + << config.zarr_version + << ", compression: " << config.compression + << ", storage: " << config.storage + << ", CPS (y): " << config.chunks_per_shard_y + << ", CPS (x): " << config.chunks_per_shard_x << ", (run " + << run << " / " << NUM_RUNS << ")..."; + double time = run_benchmark(config); + std::cout << " " << time << "s\n"; + if (time >= 0) { + csv << chunk_str << "," << config.zarr_version << "," + << config.compression << "," << config.storage << "," + << config.chunks_per_shard_y << "," + << config.chunks_per_shard_x << "," << run << "," + << std::fixed << std::setprecision(3) << time << "\n"; + } + csv.flush(); + } + } + + return 0; +} \ No newline at end of file diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index f1a3b55..6ca4671 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -190,7 +190,6 @@ bool zarr::ArrayWriter::make_metadata_sink_() { if (metadata_sink_) { - LOG_INFO("Metadata sink already exists"); return true; } From 1cf276e61e6c00e121f9038d8ca037a5542dd3b2 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 21 Nov 2024 09:41:43 -0500 Subject: [PATCH 02/26] Add ShardBuffer class to keep track of ready-to-write chunks. --- src/streaming/array.writer.cpp | 2 +- src/streaming/array.writer.hh | 3 +- src/streaming/zarrv3.array.writer.hh | 45 ++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 6ca4671..0c7fd21 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -360,7 +360,7 @@ zarr::ArrayWriter::compress_buffers_() try { const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; - std::vector tmp(tmp_size); + ChunkBuffer tmp(tmp_size); const auto nb = blosc_compress_ctx(params.clevel, params.shuffle, diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index cf934f4..076b114 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -56,10 +56,11 @@ class ArrayWriter [[nodiscard]] size_t write_frame(std::span data); protected: + using ChunkBuffer = std::vector; ArrayWriterConfig config_; /// Chunking - std::vector> chunk_buffers_; + std::vector chunk_buffers_; /// Filesystem std::vector> data_sinks_; diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index 3ad1b05..bbc7f9e 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -3,6 +3,49 @@ #include "array.writer.hh" namespace zarr { +struct ChunkIndex +{ + uint32_t buffer_idx; + uint64_t offset; + uint64_t size; +}; + +class ShardBuffer +{ + public: + ShardBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } + + bool try_add_chunk(uint32_t chunk_buffer_index, uint64_t chunk_size) + { + std::lock_guard lock(mutex_); + if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { + return false; + } + ready_chunks_.push_back( + { chunk_buffer_index, cumulative_size_, chunk_size }); + cumulative_size_ += chunk_size; + ready_count_++; + return true; + } + + std::vector take_chunks() + { + std::lock_guard lock(mutex_); + std::vector chunks = std::move(ready_chunks_); + ready_chunks_.clear(); + ready_count_ = 0; + return chunks; + } + + private: + static constexpr size_t MAX_BUFFER_SIZE_ = 16; + + std::mutex mutex_; + std::vector ready_chunks_; + uint64_t cumulative_size_{ 0 }; + uint32_t ready_count_{ 0 }; +}; + struct ZarrV3ArrayWriter : public ArrayWriter { public: @@ -14,6 +57,8 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::shared_ptr s3_connection_pool); private: + std::vector shard_buffers_; + std::vector shard_file_offsets_; std::vector> shard_tables_; From 48fe1a6fbdd4c1a74326871d37b627430c0d3808 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 21 Nov 2024 10:34:27 -0500 Subject: [PATCH 03/26] Increase number of runs/config in benchmark. --- benchmarks/benchmark.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/benchmark.cpp b/benchmarks/benchmark.cpp index 93b1e7d..69bf060 100644 --- a/benchmarks/benchmark.cpp +++ b/benchmarks/benchmark.cpp @@ -28,7 +28,7 @@ const std::vector CHUNK_CONFIGS = { { 1, 1, 64, 64, 64 }, const unsigned int ARRAY_WIDTH = 1920, ARRAY_HEIGHT = 1080, ARRAY_PLANES = 6, ARRAY_CHANNELS = 3, ARRAY_TIMEPOINTS = 10; -const unsigned int NUM_RUNS = 1; +const unsigned int NUM_RUNS = 5; struct BenchmarkConfig { From 1d8c6056113710d73c086b6982f5152cf6c44daa Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 21 Nov 2024 13:30:24 -0500 Subject: [PATCH 04/26] Add ChunkIndexBuffer to ZarrV2ArrayWriter. --- src/streaming/zarrv2.array.writer.hh | 35 ++++++++++++++++++++++++++++ src/streaming/zarrv3.array.writer.hh | 9 +++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index 2a6039a..febe88f 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -3,6 +3,39 @@ #include "array.writer.hh" namespace zarr { +class ChunkIndexBuffer +{ + public: + ChunkIndexBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } + + bool try_add_chunk(uint32_t chunk_buffer_index) + { + std::lock_guard lock(mutex_); + if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { + return false; + } + + ready_chunks_.push_back(chunk_buffer_index); + ++ready_count_; + return true; + } + + std::vector take_chunks() + { + std::lock_guard lock(mutex_); + std::vector chunks = std::move(ready_chunks_); + ready_chunks_.clear(); + ready_count_ = 0; + return chunks; + } + + private: + static constexpr size_t MAX_BUFFER_SIZE_ = 16; + + std::mutex mutex_; + std::vector ready_chunks_; + uint32_t ready_count_{ 0 }; +}; class ZarrV2ArrayWriter final : public ArrayWriter { public: @@ -14,6 +47,8 @@ class ZarrV2ArrayWriter final : public ArrayWriter std::shared_ptr s3_connection_pool); private: + ChunkIndexBuffer ready_chunks_; + ZarrVersion version_() const override { return ZarrVersion_2; }; bool flush_impl_() override; bool write_array_metadata_() override; diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index bbc7f9e..1e9d5fa 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -10,10 +10,10 @@ struct ChunkIndex uint64_t size; }; -class ShardBuffer +class ShardIndexBuffer { public: - ShardBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } + ShardIndexBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } bool try_add_chunk(uint32_t chunk_buffer_index, uint64_t chunk_size) { @@ -21,10 +21,11 @@ class ShardBuffer if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { return false; } + ready_chunks_.push_back( { chunk_buffer_index, cumulative_size_, chunk_size }); cumulative_size_ += chunk_size; - ready_count_++; + ++ready_count_; return true; } @@ -57,7 +58,7 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::shared_ptr s3_connection_pool); private: - std::vector shard_buffers_; + std::vector shards_ready_; std::vector shard_file_offsets_; std::vector> shard_tables_; From f0a97e8e2328b84afbb651df0e6059d780a77900 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 27 Nov 2024 15:02:47 -0500 Subject: [PATCH 05/26] wip: Vectorized file write (Windows). --- src/streaming/CMakeLists.txt | 2 + src/streaming/vectorized.file.writer.cpp | 221 +++++++++++++++++++++ src/streaming/vectorized.file.writer.hh | 42 ++++ tests/unit-tests/CMakeLists.txt | 1 + tests/unit-tests/vectorized-file-write.cpp | 90 +++++++++ 5 files changed, 356 insertions(+) create mode 100644 src/streaming/vectorized.file.writer.cpp create mode 100644 src/streaming/vectorized.file.writer.hh create mode 100644 tests/unit-tests/vectorized-file-write.cpp diff --git a/src/streaming/CMakeLists.txt b/src/streaming/CMakeLists.txt index cfa734c..c9b5fef 100644 --- a/src/streaming/CMakeLists.txt +++ b/src/streaming/CMakeLists.txt @@ -29,6 +29,8 @@ add_library(${tgt} zarrv2.array.writer.cpp zarrv3.array.writer.hh zarrv3.array.writer.cpp + vectorized.file.writer.hh + vectorized.file.writer.cpp ) target_include_directories(${tgt} diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp new file mode 100644 index 0000000..39ce693 --- /dev/null +++ b/src/streaming/vectorized.file.writer.cpp @@ -0,0 +1,221 @@ +#include "vectorized.file.writer.hh" +#include "macros.hh" + +namespace { +#ifdef _WIN32 +std::string +get_last_error_as_string() +{ + DWORD errorMessageID = ::GetLastError(); + if (errorMessageID == 0) { + return std::string(); // No error message has been recorded + } + + LPSTR messageBuffer = nullptr; + + size_t size = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | + FORMAT_MESSAGE_FROM_SYSTEM | + FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, + errorMessageID, + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + (LPSTR)&messageBuffer, + 0, + NULL); + + std::string message(messageBuffer, size); + + LocalFree(messageBuffer); + + return message; +} + +size_t +get_sector_size(const std::string& path) +{ + // Get volume root path + char volume_path[MAX_PATH]; + if (!GetVolumePathNameA(path.c_str(), volume_path, MAX_PATH)) { + return 0; + } + + DWORD sectors_per_cluster; + DWORD bytes_per_sector; + DWORD number_of_free_clusters; + DWORD total_number_of_clusters; + + if (!GetDiskFreeSpaceA(volume_path, + §ors_per_cluster, + &bytes_per_sector, + &number_of_free_clusters, + &total_number_of_clusters)) { + return 0; + } + + return bytes_per_sector; +} +#endif + +bool +is_aligned(const void* ptr, size_t alignment) +{ + return reinterpret_cast(ptr) % alignment == 0; +} +} // namespace + +zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) +{ +#ifdef _WIN32 + SYSTEM_INFO si; + GetSystemInfo(&si); + page_size_ = si.dwPageSize; + + sector_size_ = get_sector_size(path); + if (sector_size_ == 0) { + throw std::runtime_error("Failed to get sector size"); + } + + handle_ = CreateFileA(path.c_str(), + GENERIC_WRITE, + 0, // No sharing + nullptr, + OPEN_ALWAYS, + FILE_FLAG_OVERLAPPED | FILE_FLAG_NO_BUFFERING | + FILE_FLAG_SEQUENTIAL_SCAN, + nullptr); + if (handle_ == INVALID_HANDLE_VALUE) { + auto err = get_last_error_as_string(); + throw std::runtime_error("Failed to open file '" + path + "': " + err); + } +#else + page_size_ = sysconf(_SC_PAGESIZE); + fd_ = open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_DIRECT, 0644); + if (fd_ < 0) { + throw std::runtime_error("Failed to open file: " + path); + } +#endif +} + +zarr::VectorizedFileWriter::~VectorizedFileWriter() +{ +#ifdef _WIN32 + if (handle_ != INVALID_HANDLE_VALUE) { + CloseHandle(handle_); + } +#else + if (fd_ >= 0) { + close(fd_); + } +#endif +} + +bool +zarr::VectorizedFileWriter::write_vectors( + const std::vector>& buffers, + const std::vector& offsets) +{ + std::lock_guard lock(mutex_); + +#ifdef _WIN32 + size_t total_bytes_to_write = 0; + for (const auto& buffer : buffers) { + total_bytes_to_write += buffer.size(); + } + + const size_t nbytes_aligned = align_size_(total_bytes_to_write); + CHECK(nbytes_aligned >= total_bytes_to_write); + + auto* aligned_ptr = (std::byte*)_aligned_malloc(nbytes_aligned, page_size_); + if (!aligned_ptr) { + return false; + } + + auto* cur = aligned_ptr; + for (const auto& buffer : buffers) { + std::copy(buffer.begin(), buffer.end(), cur); + cur += buffer.size(); + } + + std::vector segments(nbytes_aligned / page_size_); + + cur = aligned_ptr; + for (auto& segment : segments) { + memset(&segment, 0, sizeof(segment)); + segment.Buffer = PtrToPtr64(cur); + cur += page_size_; + } + + OVERLAPPED overlapped = { 0 }; + overlapped.Offset = static_cast(offsets[0] & 0xFFFFFFFF); + overlapped.OffsetHigh = static_cast(offsets[0] >> 32); + overlapped.hEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); + + DWORD bytes_written; + + bool retval{ true }; + if (!WriteFileGather( + handle_, segments.data(), nbytes_aligned, nullptr, &overlapped)) { + if (GetLastError() != ERROR_IO_PENDING) { + LOG_ERROR("Failed to write file: ", get_last_error_as_string()); + retval = false; + } + + // Wait for the operation to complete + if (!GetOverlappedResult(handle_, &overlapped, &bytes_written, TRUE)) { + LOG_ERROR("Failed to get overlapped result: ", + get_last_error_as_string()); + retval = false; + } + } + + _aligned_free(aligned_ptr); + return retval; +#else + std::vector iovecs; + iovecs.reserve(buffers.size()); + + for (const auto& buffer : buffers) { + if (!is_aligned(buffer.data(), page_size_)) { + return false; + } + struct iovec iov; + iov.iov_base = + const_cast(static_cast(buffer.data())); + iov.iov_len = buffer.size(); + iovecs.push_back(iov); + } + + if (lseek(fd_, offsets[0], SEEK_SET) == -1) { + return false; + } + + ssize_t total_bytes = 0; + for (const auto& buffer : buffers) { + total_bytes += buffer.size(); + } + + ssize_t bytes_written = writev(fd_, iovecs.data(), iovecs.size()); + if (bytes_written != total_bytes) { + return false; + } +#endif + return true; +} + +size_t +zarr::VectorizedFileWriter::align_size_(size_t size) const +{ + return align_to_sector_(align_to_page_(size)); +} + +size_t +zarr::VectorizedFileWriter::align_to_page_(size_t size) const +{ + return (size + page_size_ - 1) & ~(page_size_ - 1); +} + +size_t +zarr::VectorizedFileWriter::align_to_sector_(size_t size) const +{ + return (size + sector_size_ - 1) & ~(sector_size_ - 1); +} \ No newline at end of file diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh new file mode 100644 index 0000000..2a15f62 --- /dev/null +++ b/src/streaming/vectorized.file.writer.hh @@ -0,0 +1,42 @@ +#pragma once + +#include +#include +#include +#include + +#ifdef _WIN32 +#include +#else +#include +#include +#include +#endif + +namespace zarr { +class VectorizedFileWriter +{ + public: + explicit VectorizedFileWriter(const std::string& path); + ~VectorizedFileWriter(); + + bool write_vectors(const std::vector>& buffers, + const std::vector& offsets); + + std::mutex& mutex() { return mutex_; } + + private: + std::mutex mutex_; + size_t page_size_; +#ifdef _WIN32 + HANDLE handle_; + size_t sector_size_; +#else + int fd_; +#endif + + size_t align_size_(size_t size) const; + size_t align_to_page_(size_t size) const; + size_t align_to_sector_(size_t size) const; +}; +} // namespace zarr \ No newline at end of file diff --git a/tests/unit-tests/CMakeLists.txt b/tests/unit-tests/CMakeLists.txt index 6069b73..fba62cc 100644 --- a/tests/unit-tests/CMakeLists.txt +++ b/tests/unit-tests/CMakeLists.txt @@ -25,6 +25,7 @@ set(tests zarrv3-writer-write-even zarrv3-writer-write-ragged-append-dim zarrv3-writer-write-ragged-internal-dim + vectorized-file-write ) foreach (name ${tests}) diff --git a/tests/unit-tests/vectorized-file-write.cpp b/tests/unit-tests/vectorized-file-write.cpp new file mode 100644 index 0000000..a24ff5a --- /dev/null +++ b/tests/unit-tests/vectorized-file-write.cpp @@ -0,0 +1,90 @@ +#include "vectorized.file.writer.hh" +#include "unit.test.macros.hh" + +#include +#include +#include + +namespace fs = std::filesystem; + +size_t +write_to_file(const std::string& filename) +{ + size_t file_size = 0; + zarr::VectorizedFileWriter writer(filename); + + std::vector> data(10); + std::vector offsets(10); + size_t offset = 0; + for (auto i = 0; i < data.size(); ++i) { + data[i].resize((i + 1) * 1024); + std::fill(data[i].begin(), data[i].end(), std::byte(i)); + + offsets[i] = offset; + offset += data[i].size(); + } + + file_size = offsets.back() + data.back().size(); + CHECK(writer.write_vectors(data, offsets)); + + return file_size; +} + +void +verify_file_data(const std::string& filename, size_t file_size) +{ + std::ifstream file(filename, std::ios::binary); + std::vector read_buffer(file_size); + + file.read(reinterpret_cast(read_buffer.data()), file_size); + CHECK(file.good() && file.gcount() == file_size); + + // Verify data pattern + size_t offset = 0; + for (size_t i = 0; i < 10; i++) { + size_t size = (i + 1) * 1024; + + for (size_t j = offset; j < offset + size; j++) { + auto byte = (int)read_buffer[j]; + EXPECT(byte == i, + "Data mismatch at offset ", + j, + ". Expected ", + i, + " got ", + byte, + "."); + } + offset += size; + } +} + +int +main() +{ + const auto base_dir = fs::temp_directory_path() / "vectorized-file-writer"; + if (!fs::exists(base_dir) && !fs::create_directories(base_dir)) { + std::cerr << "Failed to create directory: " << base_dir << std::endl; + return 1; + } + + int retval = 1; + const auto filename = (base_dir / "test.bin").string(); + + try { + const auto file_size = write_to_file(filename); + EXPECT(fs::exists(filename), "File not found: ", filename); + verify_file_data(filename, file_size); + + retval = 0; + } catch (const std::exception& exc) { + std::cerr << "Exception: " << exc.what() << std::endl; + } + + // cleanup + if (fs::exists(base_dir) && !fs::remove_all(base_dir)) { + std::cerr << "Failed to remove directory: " << base_dir << std::endl; + } + + return retval; +} \ No newline at end of file From 6aadf65a9e3335fa8823f8836851aeba8085780e Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 27 Nov 2024 16:21:07 -0500 Subject: [PATCH 06/26] Changes for Linux --- src/streaming/vectorized.file.writer.cpp | 53 +++++++++++++----------- src/streaming/vectorized.file.writer.hh | 1 - 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index 39ce693..6c32a4d 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -1,6 +1,8 @@ #include "vectorized.file.writer.hh" #include "macros.hh" +#include + namespace { #ifdef _WIN32 std::string @@ -54,6 +56,12 @@ get_sector_size(const std::string& path) return bytes_per_sector; } +#else +std::string +get_last_error_as_string() +{ + return strerror(errno); +} #endif bool @@ -171,31 +179,29 @@ zarr::VectorizedFileWriter::write_vectors( _aligned_free(aligned_ptr); return retval; #else - std::vector iovecs; - iovecs.reserve(buffers.size()); + std::vector iovecs(buffers.size()); - for (const auto& buffer : buffers) { - if (!is_aligned(buffer.data(), page_size_)) { - return false; - } - struct iovec iov; - iov.iov_base = - const_cast(static_cast(buffer.data())); - iov.iov_len = buffer.size(); - iovecs.push_back(iov); - } - - if (lseek(fd_, offsets[0], SEEK_SET) == -1) { - return false; + for (auto i = 0; i < buffers.size(); ++i) { + auto* iov = &iovecs[i]; + memset(iov, 0, sizeof(struct iovec)); + iov->iov_base = + const_cast(static_cast(buffers[i].data())); + iov->iov_len = buffers[i].size(); } ssize_t total_bytes = 0; for (const auto& buffer : buffers) { - total_bytes += buffer.size(); + total_bytes += static_cast(buffer.size()); } - ssize_t bytes_written = writev(fd_, iovecs.data(), iovecs.size()); + ssize_t bytes_written = pwritev(fd_, + iovecs.data(), + static_cast(iovecs.size()), + static_cast(offsets[0])); + if (bytes_written != total_bytes) { + auto error = get_last_error_as_string(); + LOG_ERROR("Failed to write file: ", error); return false; } #endif @@ -205,17 +211,16 @@ zarr::VectorizedFileWriter::write_vectors( size_t zarr::VectorizedFileWriter::align_size_(size_t size) const { - return align_to_sector_(align_to_page_(size)); + size = align_to_page_(size); +#ifdef _WIN32 + return (size + sector_size_ - 1) & ~(sector_size_ - 1); +#else + return size; +#endif } size_t zarr::VectorizedFileWriter::align_to_page_(size_t size) const { return (size + page_size_ - 1) & ~(page_size_ - 1); -} - -size_t -zarr::VectorizedFileWriter::align_to_sector_(size_t size) const -{ - return (size + sector_size_ - 1) & ~(sector_size_ - 1); } \ No newline at end of file diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh index 2a15f62..01bef6b 100644 --- a/src/streaming/vectorized.file.writer.hh +++ b/src/streaming/vectorized.file.writer.hh @@ -37,6 +37,5 @@ class VectorizedFileWriter size_t align_size_(size_t size) const; size_t align_to_page_(size_t size) const; - size_t align_to_sector_(size_t size) const; }; } // namespace zarr \ No newline at end of file From 97e93c12eef416386dd921db917fa346b84bcb2b Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 2 Dec 2024 09:27:03 -0500 Subject: [PATCH 07/26] Vectorized file write on OSX --- src/streaming/vectorized.file.writer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index 6c32a4d..23126b0 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -97,7 +97,7 @@ zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) } #else page_size_ = sysconf(_SC_PAGESIZE); - fd_ = open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC | O_DIRECT, 0644); + fd_ = open(path.c_str(), O_WRONLY | O_CREAT, 0644); if (fd_ < 0) { throw std::runtime_error("Failed to open file: " + path); } From d70ca8ef92cbe84fb444eb9e6204a4257ff88604 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 2 Dec 2024 10:25:18 -0500 Subject: [PATCH 08/26] Test vectorized file write with multiple writes to the same file. --- src/streaming/vectorized.file.writer.cpp | 8 ++-- src/streaming/vectorized.file.writer.hh | 2 +- tests/unit-tests/vectorized-file-write.cpp | 45 +++++++++++++++++----- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index 23126b0..c5a8623 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -120,7 +120,7 @@ zarr::VectorizedFileWriter::~VectorizedFileWriter() bool zarr::VectorizedFileWriter::write_vectors( const std::vector>& buffers, - const std::vector& offsets) + size_t offset) { std::lock_guard lock(mutex_); @@ -154,8 +154,8 @@ zarr::VectorizedFileWriter::write_vectors( } OVERLAPPED overlapped = { 0 }; - overlapped.Offset = static_cast(offsets[0] & 0xFFFFFFFF); - overlapped.OffsetHigh = static_cast(offsets[0] >> 32); + overlapped.Offset = static_cast(offset & 0xFFFFFFFF); + overlapped.OffsetHigh = static_cast(offset >> 32); overlapped.hEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); DWORD bytes_written; @@ -197,7 +197,7 @@ zarr::VectorizedFileWriter::write_vectors( ssize_t bytes_written = pwritev(fd_, iovecs.data(), static_cast(iovecs.size()), - static_cast(offsets[0])); + static_cast(offset)); if (bytes_written != total_bytes) { auto error = get_last_error_as_string(); diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh index 01bef6b..405bd5d 100644 --- a/src/streaming/vectorized.file.writer.hh +++ b/src/streaming/vectorized.file.writer.hh @@ -21,7 +21,7 @@ class VectorizedFileWriter ~VectorizedFileWriter(); bool write_vectors(const std::vector>& buffers, - const std::vector& offsets); + size_t offset); std::mutex& mutex() { return mutex_; } diff --git a/tests/unit-tests/vectorized-file-write.cpp b/tests/unit-tests/vectorized-file-write.cpp index a24ff5a..3b2c8c2 100644 --- a/tests/unit-tests/vectorized-file-write.cpp +++ b/tests/unit-tests/vectorized-file-write.cpp @@ -14,20 +14,21 @@ write_to_file(const std::string& filename) zarr::VectorizedFileWriter writer(filename); std::vector> data(10); - std::vector offsets(10); - size_t offset = 0; for (auto i = 0; i < data.size(); ++i) { data[i].resize((i + 1) * 1024); std::fill(data[i].begin(), data[i].end(), std::byte(i)); - - offsets[i] = offset; - offset += data[i].size(); + file_size += data[i].size(); } + CHECK(writer.write_vectors(data, 0)); - file_size = offsets.back() + data.back().size(); - CHECK(writer.write_vectors(data, offsets)); + // write more data + for (auto i = 0; i < 10; ++i) { + auto& vec = data[i]; + std::fill(vec.begin(), vec.end(), std::byte(i + 10)); + } + CHECK(writer.write_vectors(data, file_size)); - return file_size; + return 2 * file_size; } void @@ -41,10 +42,10 @@ verify_file_data(const std::string& filename, size_t file_size) // Verify data pattern size_t offset = 0; - for (size_t i = 0; i < 10; i++) { + for (size_t i = 0; i < 10; ++i) { size_t size = (i + 1) * 1024; - for (size_t j = offset; j < offset + size; j++) { + for (size_t j = offset; j < offset + size; ++j) { auto byte = (int)read_buffer[j]; EXPECT(byte == i, "Data mismatch at offset ", @@ -57,6 +58,23 @@ verify_file_data(const std::string& filename, size_t file_size) } offset += size; } + + for (size_t i = 0; i < 10; ++i) { + size_t size = (i + 1) * 1024; + + for (size_t j = offset; j < offset + size; ++j) { + auto byte = (int)read_buffer[j]; + EXPECT(byte == i + 10, + "Data mismatch at offset ", + j, + ". Expected ", + i + 10, + " got ", + byte, + "."); + } + offset += size; + } } int @@ -74,6 +92,13 @@ main() try { const auto file_size = write_to_file(filename); EXPECT(fs::exists(filename), "File not found: ", filename); + + auto file_size_on_disk = fs::file_size(filename); + EXPECT(file_size_on_disk >= file_size, // sum(1:10) * 1024 * 2 + "Expected file size of at least ", + file_size, + " bytes, got ", + file_size_on_disk); verify_file_data(filename, file_size); retval = 0; From 1f95b058e98511a80b4a182b195a8c298d286324 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 2 Dec 2024 11:31:03 -0500 Subject: [PATCH 09/26] Minor edits --- src/streaming/vectorized.file.writer.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index c5a8623..e4ea039 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -63,12 +63,6 @@ get_last_error_as_string() return strerror(errno); } #endif - -bool -is_aligned(const void* ptr, size_t alignment) -{ - return reinterpret_cast(ptr) % alignment == 0; -} } // namespace zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) @@ -123,6 +117,7 @@ zarr::VectorizedFileWriter::write_vectors( size_t offset) { std::lock_guard lock(mutex_); + bool retval{ true }; #ifdef _WIN32 size_t total_bytes_to_write = 0; @@ -133,7 +128,8 @@ zarr::VectorizedFileWriter::write_vectors( const size_t nbytes_aligned = align_size_(total_bytes_to_write); CHECK(nbytes_aligned >= total_bytes_to_write); - auto* aligned_ptr = (std::byte*)_aligned_malloc(nbytes_aligned, page_size_); + auto* aligned_ptr = + static_cast(_aligned_malloc(nbytes_aligned, page_size_)); if (!aligned_ptr) { return false; } @@ -160,7 +156,6 @@ zarr::VectorizedFileWriter::write_vectors( DWORD bytes_written; - bool retval{ true }; if (!WriteFileGather( handle_, segments.data(), nbytes_aligned, nullptr, &overlapped)) { if (GetLastError() != ERROR_IO_PENDING) { @@ -177,7 +172,6 @@ zarr::VectorizedFileWriter::write_vectors( } _aligned_free(aligned_ptr); - return retval; #else std::vector iovecs(buffers.size()); @@ -202,10 +196,10 @@ zarr::VectorizedFileWriter::write_vectors( if (bytes_written != total_bytes) { auto error = get_last_error_as_string(); LOG_ERROR("Failed to write file: ", error); - return false; + retval = false; } #endif - return true; + return retval; } size_t From d5e4b37f1378b3969816a2580192a6aa94f438ae Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 2 Dec 2024 16:19:32 -0500 Subject: [PATCH 10/26] Remove ArrayWriter::version_() --- src/streaming/array.writer.cpp | 40 ++----------------- src/streaming/array.writer.hh | 4 +- src/streaming/zarr.dimension.hh | 5 ++- src/streaming/zarrv2.array.writer.cpp | 20 ++++++++++ src/streaming/zarrv2.array.writer.hh | 4 +- src/streaming/zarrv3.array.writer.cpp | 21 ++++++++++ src/streaming/zarrv3.array.writer.hh | 5 ++- .../array-writer-write-frame-to-chunks.cpp | 7 +++- 8 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 0c7fd21..383e2a6 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -75,7 +75,6 @@ zarr::downsample(const ArrayWriterConfig& config, return true; } -/// Writer zarr::ArrayWriter::ArrayWriter(const ArrayWriterConfig& config, std::shared_ptr thread_pool) : ArrayWriter(std::move(config), thread_pool, nullptr) @@ -141,28 +140,10 @@ zarr::ArrayWriter::is_s3_array_() const bool zarr::ArrayWriter::make_data_sinks_() { - std::string data_root; - std::function parts_along_dimension; - switch (version_()) { - case ZarrVersion_2: - parts_along_dimension = chunks_along_dimension; - data_root = config_.store_path + "/" + - std::to_string(config_.level_of_detail) + "/" + - std::to_string(append_chunk_index_); - break; - case ZarrVersion_3: - parts_along_dimension = shards_along_dimension; - data_root = config_.store_path + "/data/root/" + - std::to_string(config_.level_of_detail) + "/c" + - std::to_string(append_chunk_index_); - break; - default: - LOG_ERROR("Unsupported Zarr version"); - return false; - } - SinkCreator creator(thread_pool_, s3_connection_pool_); + const auto data_root = data_root_(); + const auto parts_along_dimension = parts_along_dimension_(); if (is_s3_array_()) { if (!creator.make_data_sinks(*config_.bucket_name, data_root, @@ -193,22 +174,7 @@ zarr::ArrayWriter::make_metadata_sink_() return true; } - std::string metadata_path; - switch (version_()) { - case ZarrVersion_2: - metadata_path = config_.store_path + "/" + - std::to_string(config_.level_of_detail) + - "/.zarray"; - break; - case ZarrVersion_3: - metadata_path = config_.store_path + "/meta/root/" + - std::to_string(config_.level_of_detail) + - ".array.json"; - break; - default: - LOG_ERROR("Unsupported Zarr version"); - return false; - } + const auto metadata_path = metadata_path_(); if (is_s3_array_()) { SinkCreator creator(thread_pool_, s3_connection_pool_); diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index 076b114..30c6a29 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -78,7 +78,9 @@ class ArrayWriter std::shared_ptr s3_connection_pool_; - virtual ZarrVersion version_() const = 0; + virtual std::string data_root_() const = 0; + virtual std::string metadata_path_() const = 0; + virtual PartsAlongDimensionFun parts_along_dimension_() const = 0; bool is_s3_array_() const; diff --git a/src/streaming/zarr.dimension.hh b/src/streaming/zarr.dimension.hh index 9c0174c..66ba92d 100644 --- a/src/streaming/zarr.dimension.hh +++ b/src/streaming/zarr.dimension.hh @@ -2,6 +2,7 @@ #include "zarr.types.h" +#include #include #include #include @@ -111,4 +112,6 @@ class ArrayDimensions private: std::vector dims_; ZarrDataType dtype_; -}; \ No newline at end of file +}; + +using PartsAlongDimensionFun = std::function; \ No newline at end of file diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 8b620ca..022293c 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -72,6 +72,26 @@ zarr::ZarrV2ArrayWriter::ZarrV2ArrayWriter( { } +std::string +zarr::ZarrV2ArrayWriter::data_root_() const +{ + return config_.store_path + "/" + std::to_string(config_.level_of_detail) + + "/" + std::to_string(append_chunk_index_); +} + +std::string +zarr::ZarrV2ArrayWriter::metadata_path_() const +{ + return config_.store_path + "/" + std::to_string(config_.level_of_detail) + + "/.zarray"; +} + +PartsAlongDimensionFun +zarr::ZarrV2ArrayWriter::parts_along_dimension_() const +{ + return chunks_along_dimension; +} + bool zarr::ZarrV2ArrayWriter::flush_impl_() { diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index febe88f..8227db7 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -49,7 +49,9 @@ class ZarrV2ArrayWriter final : public ArrayWriter private: ChunkIndexBuffer ready_chunks_; - ZarrVersion version_() const override { return ZarrVersion_2; }; + std::string data_root_() const override; + std::string metadata_path_() const override; + PartsAlongDimensionFun parts_along_dimension_() const override; bool flush_impl_() override; bool write_array_metadata_() override; bool should_rollover_() const override; diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index d687076..e767abb 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -71,6 +71,27 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( } } +std::string +zarr::ZarrV3ArrayWriter::data_root_() const +{ + return config_.store_path + "/data/root/" + + std::to_string(config_.level_of_detail) + "/c" + + std::to_string(append_chunk_index_); +} + +std::string +zarr::ZarrV3ArrayWriter::metadata_path_() const +{ + return config_.store_path + "/meta/root/" + + std::to_string(config_.level_of_detail) + ".array.json"; +} + +PartsAlongDimensionFun +zarr::ZarrV3ArrayWriter::parts_along_dimension_() const +{ + return shards_along_dimension; +} + bool zarr::ZarrV3ArrayWriter::flush_impl_() { diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index 1e9d5fa..58e21d2 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -63,7 +63,10 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::vector shard_file_offsets_; std::vector> shard_tables_; - ZarrVersion version_() const override { return ZarrVersion_3; } + std::string data_root_() const override; + std::string metadata_path_() const override; + PartsAlongDimensionFun parts_along_dimension_() const override; + void compress_and_flush_() override; bool flush_impl_() override; bool write_array_metadata_() override; bool should_rollover_() const override; diff --git a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp index 9c42634..2ad53bd 100644 --- a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp +++ b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp @@ -13,7 +13,12 @@ class TestWriter : public zarr::ArrayWriter } private: - ZarrVersion version_() const override { return ZarrVersionCount; } + std::string data_root_() const override { return ""; } + std::string metadata_path_() const override { return ""; } + PartsAlongDimensionFun parts_along_dimension_() const override + { + return {}; + }; bool should_rollover_() const override { return false; } bool flush_impl_() override { return true; } bool write_array_metadata_() override { return true; } From f0bf009346375e4c14ae1d2aba71dc0b3f291ab3 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Mon, 2 Dec 2024 16:25:31 -0500 Subject: [PATCH 11/26] Do-nothing compress_and_flush_() method. --- src/streaming/array.writer.hh | 1 + src/streaming/zarrv2.array.writer.cpp | 5 +++++ src/streaming/zarrv2.array.writer.hh | 1 + src/streaming/zarrv3.array.writer.cpp | 10 ++++++++++ .../unit-tests/array-writer-write-frame-to-chunks.cpp | 1 + 5 files changed, 18 insertions(+) diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index 30c6a29..ed9fc3a 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -94,6 +94,7 @@ class ArrayWriter size_t write_frame_to_chunks_(std::span data); void compress_buffers_(); + virtual void compress_and_flush_() = 0; void flush_(); [[nodiscard]] virtual bool flush_impl_() = 0; void rollover_(); diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 022293c..1ac7267 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -92,6 +92,11 @@ zarr::ZarrV2ArrayWriter::parts_along_dimension_() const return chunks_along_dimension; } +void +zarr::ZarrV2ArrayWriter::compress_and_flush_() +{ +} + bool zarr::ZarrV2ArrayWriter::flush_impl_() { diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index 8227db7..aa80bad 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -52,6 +52,7 @@ class ZarrV2ArrayWriter final : public ArrayWriter std::string data_root_() const override; std::string metadata_path_() const override; PartsAlongDimensionFun parts_along_dimension_() const override; + void compress_and_flush_() override; bool flush_impl_() override; bool write_array_metadata_() override; bool should_rollover_() const override; diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index e767abb..e2b5d46 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -57,6 +57,7 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( std::shared_ptr thread_pool, std::shared_ptr s3_connection_pool) : ArrayWriter(config, thread_pool, s3_connection_pool) + , chunks_ready_count_{ 0 } { const auto number_of_shards = config_.dimensions->number_of_shards(); const auto chunks_per_shard = config_.dimensions->chunks_per_shard(); @@ -69,6 +70,10 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( std::fill( table.begin(), table.end(), std::numeric_limits::max()); } + + std::fill_n(chunks_ready_.begin(), + MAX_READY_CHUNK_COUNT_, + std::numeric_limits::max()); } std::string @@ -92,6 +97,11 @@ zarr::ZarrV3ArrayWriter::parts_along_dimension_() const return shards_along_dimension; } +void +zarr::ZarrV3ArrayWriter::compress_and_flush_() +{ +} + bool zarr::ZarrV3ArrayWriter::flush_impl_() { diff --git a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp index 2ad53bd..92d5681 100644 --- a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp +++ b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp @@ -20,6 +20,7 @@ class TestWriter : public zarr::ArrayWriter return {}; }; bool should_rollover_() const override { return false; } + void compress_and_flush_() override {} bool flush_impl_() override { return true; } bool write_array_metadata_() override { return true; } }; From 7559cbf21fe00c17f6d39ef6765afbb12c7a850d Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 3 Dec 2024 09:34:02 -0500 Subject: [PATCH 12/26] Compress right into flush (wip: Zarr V2 only). --- src/streaming/array.writer.cpp | 3 +- src/streaming/zarrv2.array.writer.cpp | 128 +++++++++++++++++++------- src/streaming/zarrv3.array.writer.cpp | 2 + 3 files changed, 97 insertions(+), 36 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 383e2a6..9065f56 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -366,8 +366,7 @@ zarr::ArrayWriter::flush_() } // compress buffers and write out - compress_buffers_(); - CHECK(flush_impl_()); + compress_and_flush_(); const auto should_rollover = should_rollover_(); if (should_rollover) { diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 1ac7267..3474648 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -95,51 +95,111 @@ zarr::ZarrV2ArrayWriter::parts_along_dimension_() const void zarr::ZarrV2ArrayWriter::compress_and_flush_() { -} + if (bytes_to_flush_ == 0) { + LOG_DEBUG("No data to flush"); + return; + } -bool -zarr::ZarrV2ArrayWriter::flush_impl_() -{ - // create chunk files CHECK(data_sinks_.empty()); - if (!make_data_sinks_()) { - return false; - } + CHECK(make_data_sinks_()); - CHECK(data_sinks_.size() == chunk_buffers_.size()); + const auto bytes_per_px = bytes_of_type(config_.dtype); std::latch latch(chunk_buffers_.size()); - { - std::scoped_lock lock(buffers_mutex_); - for (auto i = 0; i < data_sinks_.size(); ++i) { - auto& chunk = chunk_buffers_.at(i); - EXPECT(thread_pool_->push_job( - std::move([&sink = data_sinks_.at(i), - data_ = chunk.data(), - size = chunk.size(), - &latch](std::string& err) -> bool { - bool success = false; - try { - std::span data{ - reinterpret_cast(data_), size - }; - CHECK(sink->write(0, data)); - success = true; - } catch (const std::exception& exc) { - err = "Failed to write chunk: " + - std::string(exc.what()); - } - - latch.count_down(); - return success; - })), - "Failed to push job to thread pool"); + for (auto i = 0; i < data_sinks_.size(); ++i) { + auto& chunk = chunk_buffers_[i]; + + if (config_.compression_params) { + auto& params = *config_.compression_params; + auto job = [¶ms, + buf = &chunk, + bytes_per_px, + &sink = data_sinks_[i], + thread_pool = thread_pool_, + &latch](std::string& err) -> bool { + const size_t bytes_of_chunk = buf->size(); + + const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; + ChunkBuffer tmp(tmp_size); + const auto nb = + blosc_compress_ctx(params.clevel, + params.shuffle, + bytes_per_px, + bytes_of_chunk, + buf->data(), + tmp.data(), + tmp_size, + params.codec_id.c_str(), + 0 /* blocksize - 0:automatic */, + 1); + + CHECK(nb > 0); + tmp.resize(nb); + buf->swap(tmp); + + auto queued = thread_pool->push_job( + std::move([&sink, buf, &latch](std::string& err) -> bool { + bool success = false; + + try { + success = sink->write(0, *buf); + } catch (const std::exception& exc) { + err = + "Failed to write chunk: " + std::string(exc.what()); + } + + latch.count_down(); + return success; + })); + + if (!queued) { + err = "Failed to push job to thread pool"; + latch.count_down(); + } + + return queued; + }; + + CHECK(thread_pool_->push_job(std::move(job))); + } else { + auto job = [buf = &chunk, + &sink = data_sinks_[i], + thread_pool = thread_pool_, + &latch](std::string& err) -> bool { + auto queued = thread_pool->push_job( + std::move([&sink, buf, &latch](std::string& err) -> bool { + bool success = false; + + try { + success = sink->write(0, *buf); + } catch (const std::exception& exc) { + err = + "Failed to write chunk: " + std::string(exc.what()); + } + + latch.count_down(); + return success; + })); + + if (!queued) { + err = "Failed to push job to thread pool"; + latch.count_down(); + } + + return queued; + }; + + CHECK(thread_pool_->push_job(std::move(job))); } } // wait for all threads to finish latch.wait(); +} +bool +zarr::ZarrV2ArrayWriter::flush_impl_() +{ return true; } diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index e2b5d46..9a821ff 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -100,6 +100,8 @@ zarr::ZarrV3ArrayWriter::parts_along_dimension_() const void zarr::ZarrV3ArrayWriter::compress_and_flush_() { + compress_buffers_(); + CHECK(flush_impl_()); } bool From d0bc1d39bbdbb1ed5fbc7ed09f6c00646623f1c5 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 4 Dec 2024 15:28:17 -0500 Subject: [PATCH 13/26] wip --- src/streaming/CMakeLists.txt | 2 + src/streaming/shard.writer.cpp | 61 ++++++++++++++ src/streaming/shard.writer.hh | 53 ++++++++++++ src/streaming/zarrv2.array.writer.cpp | 14 +++- src/streaming/zarrv3.array.writer.cpp | 111 ++++++++++++++++++++------ src/streaming/zarrv3.array.writer.hh | 48 +---------- 6 files changed, 218 insertions(+), 71 deletions(-) create mode 100644 src/streaming/shard.writer.cpp create mode 100644 src/streaming/shard.writer.hh diff --git a/src/streaming/CMakeLists.txt b/src/streaming/CMakeLists.txt index c9b5fef..cf1dbaf 100644 --- a/src/streaming/CMakeLists.txt +++ b/src/streaming/CMakeLists.txt @@ -31,6 +31,8 @@ add_library(${tgt} zarrv3.array.writer.cpp vectorized.file.writer.hh vectorized.file.writer.cpp + shard.writer.hh + shard.writer.cpp ) target_include_directories(${tgt} diff --git a/src/streaming/shard.writer.cpp b/src/streaming/shard.writer.cpp new file mode 100644 index 0000000..f15d00c --- /dev/null +++ b/src/streaming/shard.writer.cpp @@ -0,0 +1,61 @@ +#include "shard.writer.hh" +#include "macros.hh" +#include "vectorized.file.writer.hh" + +#include +#include + +#ifdef max +#undef max +#endif + +zarr::ShardWriter::ShardWriter(std::shared_ptr thread_pool, + uint32_t chunks_before_flush, + uint32_t chunks_per_shard) + : thread_pool_(thread_pool) + , chunks_before_flush_{ chunks_before_flush } + , chunks_flushed_{ 0 } + , cumulative_size_{ 0 } + , file_offset_{ 0 } + , index_table_(2 * chunks_per_shard * sizeof(uint64_t)) +{ + std::fill_n(reinterpret_cast(index_table_.data()), + index_table_.size() / sizeof(uint64_t), + std::numeric_limits::max()); + + ready_chunks_.reserve(chunks_before_flush_); + chunks_.reserve(chunks_before_flush_); +} + +void +zarr::ShardWriter::add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard) +{ + std::unique_lock lock(mutex_); + cv_.wait(lock, + [this] { return ready_chunks_.size() < chunks_before_flush_; }); + + set_offset_extent_(index_in_shard, cumulative_size_, buffer->size()); + cumulative_size_ += buffer->size(); + + chunks_.push_back(buffer); + if (chunks_.size() == chunks_before_flush_) { + auto job = [this](std::string& err) -> bool { + std::vector> buffers; + return true; + }; + + EXPECT(thread_pool_->push_job(job), + "Failed to push job to thread pool."); + } +} + +void +zarr::ShardWriter::set_offset_extent_(uint32_t shard_internal_index, + uint64_t offset, + uint64_t size) +{ + auto* index_table_u64 = reinterpret_cast(index_table_.data()); + const auto index = 2 * shard_internal_index; + index_table_u64[index] = offset; + index_table_u64[index + 1] = size; +} \ No newline at end of file diff --git a/src/streaming/shard.writer.hh b/src/streaming/shard.writer.hh new file mode 100644 index 0000000..4c10826 --- /dev/null +++ b/src/streaming/shard.writer.hh @@ -0,0 +1,53 @@ +#pragma once + +#include "thread.pool.hh" + +#include +#include +#include + +#ifdef min +#undef min +#endif + +namespace zarr { +struct ChunkIndex +{ + uint32_t buffer_idx; + uint64_t offset; + uint64_t size; +}; + +class ShardWriter +{ + public: + using ChunkBufferPtr = std::vector*; + + ShardWriter(std::shared_ptr thread_pool, + uint32_t chunks_before_flush, + uint32_t chunks_per_shard); + + void add_chunk(ChunkBufferPtr buffer, uint32_t chunk_buffer_index); + + private: + uint32_t chunks_before_flush_; + uint32_t chunks_per_shard_; + uint32_t chunks_flushed_; + + std::vector index_table_; + + std::shared_ptr thread_pool_; + + std::mutex mutex_; + std::condition_variable cv_; + std::vector chunks_; + std::vector ready_chunks_; + std::vector chunk_table_; + uint64_t cumulative_size_; + uint64_t file_offset_; + + void set_offset_extent_(uint32_t shard_internal_index, + uint64_t offset, + uint64_t size); +}; +} // namespace zarr \ No newline at end of file diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 3474648..ee9070d 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -100,13 +100,16 @@ zarr::ZarrV2ArrayWriter::compress_and_flush_() return; } + const auto n_chunks = chunk_buffers_.size(); + CHECK(data_sinks_.empty()); CHECK(make_data_sinks_()); + CHECK(data_sinks_.size() == n_chunks); const auto bytes_per_px = bytes_of_type(config_.dtype); - std::latch latch(chunk_buffers_.size()); - for (auto i = 0; i < data_sinks_.size(); ++i) { + std::latch latch(n_chunks); + for (auto i = 0; i < n_chunks; ++i) { auto& chunk = chunk_buffers_[i]; if (config_.compression_params) { @@ -133,7 +136,12 @@ zarr::ZarrV2ArrayWriter::compress_and_flush_() 0 /* blocksize - 0:automatic */, 1); - CHECK(nb > 0); + if (nb <= 0) { + err = "Failed to compress chunk"; + latch.count_down(); + return false; + } + tmp.resize(nb); buf->swap(tmp); diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index 9a821ff..67b7513 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -57,13 +57,15 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( std::shared_ptr thread_pool, std::shared_ptr s3_connection_pool) : ArrayWriter(config, thread_pool, s3_connection_pool) - , chunks_ready_count_{ 0 } { - const auto number_of_shards = config_.dimensions->number_of_shards(); + const auto n_shards = config_.dimensions->number_of_shards(); + const auto chunks_in_memory = + config_.dimensions->number_of_chunks_in_memory(); const auto chunks_per_shard = config_.dimensions->chunks_per_shard(); + const auto chunks_before_flush = chunks_in_memory / n_shards; - shard_file_offsets_.resize(number_of_shards, 0); - shard_tables_.resize(number_of_shards); + shard_file_offsets_.resize(n_shards, 0); + shard_tables_.resize(n_shards); for (auto& table : shard_tables_) { table.resize(2 * chunks_per_shard); @@ -71,9 +73,17 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( table.begin(), table.end(), std::numeric_limits::max()); } - std::fill_n(chunks_ready_.begin(), - MAX_READY_CHUNK_COUNT_, - std::numeric_limits::max()); + // get shard indices for each chunk + chunk_in_shards_.resize(n_shards); + for (auto i = 0; i < chunks_in_memory; ++i) { + const auto index = config_.dimensions->shard_index_for_chunk(i); + chunk_in_shards_[index].push_back(i); + } + + shards_ready_.resize(n_shards); + for (auto& shard : shards_ready_) { + shard.reset(new ShardWriter(thread_pool_, chunks_before_flush, 0)); + } } std::string @@ -100,33 +110,88 @@ zarr::ZarrV3ArrayWriter::parts_along_dimension_() const void zarr::ZarrV3ArrayWriter::compress_and_flush_() { - compress_buffers_(); - CHECK(flush_impl_()); -} + if (bytes_to_flush_ == 0) { + LOG_DEBUG("No data to flush"); + return; + } -bool -zarr::ZarrV3ArrayWriter::flush_impl_() -{ // create shard files if they don't exist - if (data_sinks_.empty() && !make_data_sinks_()) { - return false; + if (data_sinks_.empty()) { + CHECK(make_data_sinks_()); } - const auto n_shards = config_.dimensions->number_of_shards(); + const auto n_chunks = chunk_buffers_.size(); + const auto n_shards = chunk_in_shards_.size(); CHECK(data_sinks_.size() == n_shards); - // get shard indices for each chunk - std::vector> chunk_in_shards(n_shards); - for (auto i = 0; i < chunk_buffers_.size(); ++i) { - const auto index = config_.dimensions->shard_index_for_chunk(i); - chunk_in_shards[index].push_back(i); + const auto bytes_per_px = bytes_of_type(config_.dtype); + + auto write_table = is_finalizing_ || should_rollover_(); + + std::latch latch(n_chunks); + + for (auto i = 0; i < n_chunks; ++i) { + auto& chunk = chunk_buffers_[i]; + const auto shard_index = config_.dimensions->shard_index_for_chunk(i); + auto& shard = shards_ready_[shard_index]; + + if (config_.compression_params) { + auto& params = *config_.compression_params; + auto job = [¶ms, + buf = &chunk, + bytes_per_px, + chunk_index = i, + &shard, + &latch](std::string& err) -> bool { + const size_t bytes_of_chunk = buf->size(); + + const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; + ChunkBuffer tmp(tmp_size); + const auto nb = + blosc_compress_ctx(params.clevel, + params.shuffle, + bytes_per_px, + bytes_of_chunk, + buf->data(), + tmp.data(), + tmp_size, + params.codec_id.c_str(), + 0 /* blocksize - 0:automatic */, + 1); + + if (nb <= 0) { + err = "Failed to compress chunk"; + latch.count_down(); + return false; + } + + tmp.resize(nb); + buf->swap(tmp); + + shard->add_chunk(buf, chunk_index); + + latch.count_down(); + + return true; + }; + } else { + shards_ready_[shard_index]->add_chunk(i, chunk.size()); + latch.count_down(); + } } + latch.wait(); + // compress_buffers_(); + // CHECK(flush_impl_()); +} + +bool +zarr::ZarrV3ArrayWriter::flush_impl_() +{ // write out chunks to shards - auto write_table = is_finalizing_ || should_rollover_(); std::latch latch(n_shards); for (auto i = 0; i < n_shards; ++i) { - const auto& chunks = chunk_in_shards.at(i); + const auto& chunks = chunk_in_shards_.at(i); auto& chunk_table = shard_tables_.at(i); auto* file_offset = &shard_file_offsets_.at(i); diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index 58e21d2..aaa131c 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -1,52 +1,9 @@ #pragma once #include "array.writer.hh" +#include "shard.writer.hh" namespace zarr { -struct ChunkIndex -{ - uint32_t buffer_idx; - uint64_t offset; - uint64_t size; -}; - -class ShardIndexBuffer -{ - public: - ShardIndexBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } - - bool try_add_chunk(uint32_t chunk_buffer_index, uint64_t chunk_size) - { - std::lock_guard lock(mutex_); - if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { - return false; - } - - ready_chunks_.push_back( - { chunk_buffer_index, cumulative_size_, chunk_size }); - cumulative_size_ += chunk_size; - ++ready_count_; - return true; - } - - std::vector take_chunks() - { - std::lock_guard lock(mutex_); - std::vector chunks = std::move(ready_chunks_); - ready_chunks_.clear(); - ready_count_ = 0; - return chunks; - } - - private: - static constexpr size_t MAX_BUFFER_SIZE_ = 16; - - std::mutex mutex_; - std::vector ready_chunks_; - uint64_t cumulative_size_{ 0 }; - uint32_t ready_count_{ 0 }; -}; - struct ZarrV3ArrayWriter : public ArrayWriter { public: @@ -58,10 +15,11 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::shared_ptr s3_connection_pool); private: - std::vector shards_ready_; + std::vector> shards_ready_; std::vector shard_file_offsets_; std::vector> shard_tables_; + std::vector> chunk_in_shards_; std::string data_root_() const override; std::string metadata_path_() const override; From 57178672447312415b6aa9ce2d5a342105b2ab5e Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 4 Dec 2024 15:42:20 -0500 Subject: [PATCH 14/26] Use a vector of spans to avoid copies. --- src/streaming/vectorized.file.writer.cpp | 2 +- src/streaming/vectorized.file.writer.hh | 2 +- tests/unit-tests/vectorized-file-write.cpp | 8 ++++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index e4ea039..18b57f8 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -113,7 +113,7 @@ zarr::VectorizedFileWriter::~VectorizedFileWriter() bool zarr::VectorizedFileWriter::write_vectors( - const std::vector>& buffers, + const std::vector>& buffers, size_t offset) { std::lock_guard lock(mutex_); diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh index 405bd5d..92350ea 100644 --- a/src/streaming/vectorized.file.writer.hh +++ b/src/streaming/vectorized.file.writer.hh @@ -20,7 +20,7 @@ class VectorizedFileWriter explicit VectorizedFileWriter(const std::string& path); ~VectorizedFileWriter(); - bool write_vectors(const std::vector>& buffers, + bool write_vectors(const std::vector>& buffers, size_t offset); std::mutex& mutex() { return mutex_; } diff --git a/tests/unit-tests/vectorized-file-write.cpp b/tests/unit-tests/vectorized-file-write.cpp index 3b2c8c2..79ebe6d 100644 --- a/tests/unit-tests/vectorized-file-write.cpp +++ b/tests/unit-tests/vectorized-file-write.cpp @@ -14,19 +14,23 @@ write_to_file(const std::string& filename) zarr::VectorizedFileWriter writer(filename); std::vector> data(10); + std::vector> spans(10); + for (auto i = 0; i < data.size(); ++i) { data[i].resize((i + 1) * 1024); std::fill(data[i].begin(), data[i].end(), std::byte(i)); file_size += data[i].size(); + spans[i] = data[i]; } - CHECK(writer.write_vectors(data, 0)); + CHECK(writer.write_vectors(spans, 0)); // write more data for (auto i = 0; i < 10; ++i) { auto& vec = data[i]; std::fill(vec.begin(), vec.end(), std::byte(i + 10)); + spans[i] = vec; } - CHECK(writer.write_vectors(data, file_size)); + CHECK(writer.write_vectors(spans, file_size)); return 2 * file_size; } From 0f71dd060b740358964391099a7daf1475370bfc Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 5 Dec 2024 13:07:47 -0500 Subject: [PATCH 15/26] Remove dead code. --- src/streaming/zarrv2.array.writer.hh | 35 --------------------- src/streaming/zarrv3.array.writer.hh | 46 ---------------------------- 2 files changed, 81 deletions(-) diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index febe88f..2a6039a 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -3,39 +3,6 @@ #include "array.writer.hh" namespace zarr { -class ChunkIndexBuffer -{ - public: - ChunkIndexBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } - - bool try_add_chunk(uint32_t chunk_buffer_index) - { - std::lock_guard lock(mutex_); - if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { - return false; - } - - ready_chunks_.push_back(chunk_buffer_index); - ++ready_count_; - return true; - } - - std::vector take_chunks() - { - std::lock_guard lock(mutex_); - std::vector chunks = std::move(ready_chunks_); - ready_chunks_.clear(); - ready_count_ = 0; - return chunks; - } - - private: - static constexpr size_t MAX_BUFFER_SIZE_ = 16; - - std::mutex mutex_; - std::vector ready_chunks_; - uint32_t ready_count_{ 0 }; -}; class ZarrV2ArrayWriter final : public ArrayWriter { public: @@ -47,8 +14,6 @@ class ZarrV2ArrayWriter final : public ArrayWriter std::shared_ptr s3_connection_pool); private: - ChunkIndexBuffer ready_chunks_; - ZarrVersion version_() const override { return ZarrVersion_2; }; bool flush_impl_() override; bool write_array_metadata_() override; diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index 1e9d5fa..3ad1b05 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -3,50 +3,6 @@ #include "array.writer.hh" namespace zarr { -struct ChunkIndex -{ - uint32_t buffer_idx; - uint64_t offset; - uint64_t size; -}; - -class ShardIndexBuffer -{ - public: - ShardIndexBuffer() { ready_chunks_.reserve(MAX_BUFFER_SIZE_); } - - bool try_add_chunk(uint32_t chunk_buffer_index, uint64_t chunk_size) - { - std::lock_guard lock(mutex_); - if (ready_chunks_.size() >= MAX_BUFFER_SIZE_) { - return false; - } - - ready_chunks_.push_back( - { chunk_buffer_index, cumulative_size_, chunk_size }); - cumulative_size_ += chunk_size; - ++ready_count_; - return true; - } - - std::vector take_chunks() - { - std::lock_guard lock(mutex_); - std::vector chunks = std::move(ready_chunks_); - ready_chunks_.clear(); - ready_count_ = 0; - return chunks; - } - - private: - static constexpr size_t MAX_BUFFER_SIZE_ = 16; - - std::mutex mutex_; - std::vector ready_chunks_; - uint64_t cumulative_size_{ 0 }; - uint32_t ready_count_{ 0 }; -}; - struct ZarrV3ArrayWriter : public ArrayWriter { public: @@ -58,8 +14,6 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::shared_ptr s3_connection_pool); private: - std::vector shards_ready_; - std::vector shard_file_offsets_; std::vector> shard_tables_; From ed89a98c9f9aa02e2881342cfd951ab29d70df7b Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Thu, 5 Dec 2024 13:20:46 -0500 Subject: [PATCH 16/26] Remove benchmark. --- CMakeLists.txt | 7 -- benchmarks/CMakeLists.txt | 18 --- benchmarks/benchmark.cpp | 233 -------------------------------------- 3 files changed, 258 deletions(-) delete mode 100644 benchmarks/CMakeLists.txt delete mode 100644 benchmarks/benchmark.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 19e97d7..5d877cb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -21,7 +21,6 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) option(BUILD_PYTHON "Build Python bindings" OFF) -option(BUILD_BENCHMARK "Build benchmarks" OFF) if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME) include(CTest) @@ -34,12 +33,6 @@ else () message(STATUS "Skipping test targets") endif () -if (BUILD_BENCHMARK) - add_subdirectory(benchmarks) -else () - message(STATUS "Skipping benchmarks") -endif () - if (${BUILD_PYTHON}) add_subdirectory(python) else () diff --git a/benchmarks/CMakeLists.txt b/benchmarks/CMakeLists.txt deleted file mode 100644 index 80fda26..0000000 --- a/benchmarks/CMakeLists.txt +++ /dev/null @@ -1,18 +0,0 @@ -set(project acquire-zarr) - -set(tgt acquire-zarr-benchmark) -add_executable(${tgt} benchmark.cpp) -target_compile_definitions(${tgt} PUBLIC "TEST=\"${tgt}\"") -set_target_properties(${tgt} PROPERTIES - MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>" -) -target_include_directories(${tgt} PRIVATE - ${PROJECT_SOURCE_DIR}/include - ${PROJECT_SOURCE_DIR}/src/logger -) -target_link_libraries(${tgt} PRIVATE - acquire-logger - acquire-zarr - nlohmann_json::nlohmann_json - miniocpp::miniocpp -) diff --git a/benchmarks/benchmark.cpp b/benchmarks/benchmark.cpp deleted file mode 100644 index 69bf060..0000000 --- a/benchmarks/benchmark.cpp +++ /dev/null @@ -1,233 +0,0 @@ -#include "acquire.zarr.h" -#include -#include -#include -#include -#include -#include -#include - -#define DIM(name_, type_, array_size, chunk_size, shard_size) \ - { .name = (name_), \ - .type = (type_), \ - .array_size_px = (array_size), \ - .chunk_size_px = (chunk_size), \ - .shard_size_chunks = (shard_size) } - -namespace fs = std::filesystem; - -struct ChunkConfig -{ - unsigned int t, c, z, y, x; -}; - -const std::vector CHUNK_CONFIGS = { { 1, 1, 64, 64, 64 }, - { 1, 1, 128, 128, 128 }, - { 1, 1, 256, 256, 256 } }; - -const unsigned int ARRAY_WIDTH = 1920, ARRAY_HEIGHT = 1080, ARRAY_PLANES = 6, - ARRAY_CHANNELS = 3, ARRAY_TIMEPOINTS = 10; - -const unsigned int NUM_RUNS = 5; - -struct BenchmarkConfig -{ - ChunkConfig chunk; - int zarr_version; - std::string compression; - std::string storage; - unsigned int chunks_per_shard_x; - unsigned int chunks_per_shard_y; - std::string s3_endpoint; - std::string s3_bucket; - std::string s3_access_key; - std::string s3_secret_key; -}; - -class Timer -{ - using Clock = std::chrono::high_resolution_clock; - Clock::time_point start; - - public: - Timer() - : start(Clock::now()) - { - } - double elapsed() - { - auto end = Clock::now(); - return std::chrono::duration(end - start).count(); - } -}; - -ZarrStream* -setup_stream(const BenchmarkConfig& config) -{ - ZarrStreamSettings settings = { .store_path = "benchmark.zarr", - .s3_settings = nullptr, - .compression_settings = nullptr, - .data_type = ZarrDataType_uint16, - .version = static_cast( - config.zarr_version) }; - - ZarrCompressionSettings comp_settings = {}; - if (config.compression != "none") { - comp_settings.compressor = ZarrCompressor_Blosc1; - comp_settings.codec = config.compression == "lz4" - ? ZarrCompressionCodec_BloscLZ4 - : ZarrCompressionCodec_BloscZstd; - comp_settings.level = 1; - comp_settings.shuffle = 1; - settings.compression_settings = &comp_settings; - } - - ZarrS3Settings s3_settings = {}; - if (config.storage == "s3") { - s3_settings = { - .endpoint = config.s3_endpoint.c_str(), - .bucket_name = config.s3_bucket.c_str(), - .access_key_id = config.s3_access_key.c_str(), - .secret_access_key = config.s3_secret_key.c_str(), - }; - settings.s3_settings = &s3_settings; - } - - ZarrStreamSettings_create_dimension_array(&settings, 5); - auto* dims = settings.dimensions; - - dims[0] = - DIM("t", ZarrDimensionType_Time, ARRAY_TIMEPOINTS, config.chunk.t, 1); - dims[1] = - DIM("c", ZarrDimensionType_Channel, ARRAY_CHANNELS, config.chunk.c, 1); - dims[2] = - DIM("z", ZarrDimensionType_Space, ARRAY_PLANES, config.chunk.z, 1); - dims[3] = DIM("y", - ZarrDimensionType_Space, - ARRAY_HEIGHT, - config.chunk.y, - config.chunks_per_shard_y); - dims[4] = DIM("x", - ZarrDimensionType_Space, - ARRAY_WIDTH, - config.chunk.x, - config.chunks_per_shard_x); - - return ZarrStream_create(&settings); -} - -double -run_benchmark(const BenchmarkConfig& config) -{ - auto* stream = setup_stream(config); - if (!stream) - return -1.0; - - const size_t frame_size = ARRAY_WIDTH * ARRAY_HEIGHT * sizeof(uint16_t); - std::vector frame(ARRAY_WIDTH * ARRAY_HEIGHT, 0); - const auto num_frames = ARRAY_PLANES * ARRAY_CHANNELS * ARRAY_TIMEPOINTS; - - Timer timer; - size_t bytes_out; - for (int i = 0; i < num_frames; ++i) { - if (ZarrStream_append(stream, frame.data(), frame_size, &bytes_out) != - ZarrStatusCode_Success) { - ZarrStream_destroy(stream); - return -1.0; - } - } - double elapsed = timer.elapsed(); - - ZarrStream_destroy(stream); - if (config.storage == "filesystem") { - fs::remove_all("benchmark.zarr"); - } - return elapsed; -} - -int -main() -{ - std::ofstream csv("zarr_benchmarks.csv"); - csv << "chunk_size,zarr_version,compression,storage,chunks_per_shard_y," - "chunks_per_shard_x,run,time_seconds\n"; - - std::vector configs; - for (const auto& chunk : CHUNK_CONFIGS) { - - // V2 configurations (no sharding) - for (const auto& compression : { "none", "lz4", "zstd" }) { - configs.push_back({ chunk, 2, compression, "filesystem", 1, 1 }); - - if (std::getenv("ZARR_S3_ENDPOINT")) { - configs.push_back({ chunk, - 2, - compression, - "s3", - 1, - 1, - std::getenv("ZARR_S3_ENDPOINT"), - std::getenv("ZARR_S3_BUCKET_NAME"), - std::getenv("ZARR_S3_ACCESS_KEY_ID"), - std::getenv("ZARR_S3_SECRET_ACCESS_KEY") }); - } - } - - unsigned int max_cps_y = (ARRAY_HEIGHT + chunk.y - 1) / chunk.y; - unsigned int max_cps_x = (ARRAY_WIDTH + chunk.x - 1) / chunk.x; - - // V3 configurations (with sharding) - for (unsigned int cps_y = 1; cps_y <= max_cps_y; cps_y *= 2) { - for (unsigned int cps_x = 1; cps_x <= max_cps_x; cps_x *= 2) { - for (const auto& compression : { "none", "lz4", "zstd" }) { - configs.push_back( - { chunk, 3, compression, "filesystem", cps_x, cps_y }); - - if (std::getenv("ZARR_S3_ENDPOINT")) { - configs.push_back( - { chunk, - 3, - compression, - "s3", - cps_x, - cps_y, - std::getenv("ZARR_S3_ENDPOINT"), - std::getenv("ZARR_S3_BUCKET_NAME"), - std::getenv("ZARR_S3_ACCESS_KEY_ID"), - std::getenv("ZARR_S3_SECRET_ACCESS_KEY") }); - } - } - } - } - } - - for (const auto& config : configs) { - std::string chunk_str = std::to_string(config.chunk.t) + "x" + - std::to_string(config.chunk.c) + "x" + - std::to_string(config.chunk.z) + "x" + - std::to_string(config.chunk.y) + "x" + - std::to_string(config.chunk.x); - - for (unsigned int run = 1; run <= NUM_RUNS; ++run) { - std::cout << "Benchmarking " << chunk_str << " Zarr V" - << config.zarr_version - << ", compression: " << config.compression - << ", storage: " << config.storage - << ", CPS (y): " << config.chunks_per_shard_y - << ", CPS (x): " << config.chunks_per_shard_x << ", (run " - << run << " / " << NUM_RUNS << ")..."; - double time = run_benchmark(config); - std::cout << " " << time << "s\n"; - if (time >= 0) { - csv << chunk_str << "," << config.zarr_version << "," - << config.compression << "," << config.storage << "," - << config.chunks_per_shard_y << "," - << config.chunks_per_shard_x << "," << run << "," - << std::fixed << std::setprecision(3) << time << "\n"; - } - csv.flush(); - } - } - - return 0; -} \ No newline at end of file From dbed6e7df02afa30a4f90ae5e63b32290714286e Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 10 Dec 2024 10:57:39 -0500 Subject: [PATCH 17/26] Update and test shard writer. --- src/streaming/shard.writer.cpp | 83 ++++++++--- src/streaming/shard.writer.hh | 28 ++-- src/streaming/vectorized.file.writer.cpp | 15 +- src/streaming/vectorized.file.writer.hh | 9 +- src/streaming/zarrv3.array.writer.cpp | 2 +- tests/unit-tests/CMakeLists.txt | 1 + .../shard-writer-add-chunk-data-to-flush.cpp | 131 ++++++++++++++++++ 7 files changed, 224 insertions(+), 45 deletions(-) create mode 100644 tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp diff --git a/src/streaming/shard.writer.cpp b/src/streaming/shard.writer.cpp index 5a3c0d0..6eb17d6 100644 --- a/src/streaming/shard.writer.cpp +++ b/src/streaming/shard.writer.cpp @@ -3,17 +3,21 @@ #include "vectorized.file.writer.hh" #include +#include #include +namespace fs = std::filesystem; + #ifdef max #undef max #endif -zarr::ShardWriter::ShardWriter(std::shared_ptr thread_pool, +zarr::ShardWriter::ShardWriter(std::string_view file_path, uint32_t chunks_before_flush, uint32_t chunks_per_shard) - : thread_pool_(thread_pool) + : file_path_(file_path) , chunks_before_flush_{ chunks_before_flush } + , chunks_per_shard_{ chunks_per_shard } , chunks_flushed_{ 0 } , cumulative_size_{ 0 } , file_offset_{ 0 } @@ -37,24 +41,36 @@ zarr::ShardWriter::add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard) chunks_.push_back(buffer); if (chunks_.size() == chunks_before_flush_) { - auto job = [this](std::string& err) -> bool { - std::vector> buffers; - buffers.reserve(chunks_.size() + 1); - for (const auto& chunk : chunks_) { - buffers.push_back(std::span(*chunk)); - } - buffers.push_back(std::span(index_table_)); - // VectorizedFileWriter writer() - - - chunks_.clear(); - cv_.notify_all(); - return true; - }; - - EXPECT(thread_pool_->push_job(job), - "Failed to push job to thread pool."); + flush_(); + } +} + +bool +zarr::ShardWriter::flush_() +{ + std::vector> buffers; + buffers.reserve(chunks_.size() + 1); + for (const auto& chunk : chunks_) { + buffers.emplace_back(*chunk); } + buffers.emplace_back(index_table_); + + try { + VectorizedFileWriter writer(file_path_); + writer.write_vectors(buffers, file_offset_); + } catch (const std::exception& exc) { + LOG_ERROR("Failed to write chunk: ", std::string(exc.what())); + return false; + } + + chunks_flushed_ += chunks_.size(); + chunks_.clear(); + chunks_.reserve(chunks_before_flush_); + file_offset_ = cumulative_size_; + + cv_.notify_all(); + + return true; } void @@ -62,8 +78,37 @@ zarr::ShardWriter::set_offset_extent_(uint32_t shard_internal_index, uint64_t offset, uint64_t size) { + EXPECT(shard_internal_index < chunks_per_shard_, + "Shard internal index ", + shard_internal_index, + " out of bounds"); + auto* index_table_u64 = reinterpret_cast(index_table_.data()); const auto index = 2 * shard_internal_index; index_table_u64[index] = offset; index_table_u64[index + 1] = size; +} + +bool +zarr::finalize_shard_writer(std::unique_ptr&& writer) +{ + if (writer == nullptr) { + LOG_INFO("Writer is null. Nothing to finalize."); + return true; + } + + if (!writer->flush_()) { + return false; + } + + // resize file if necessary + const auto file_size = fs::file_size(writer->file_path_); + const auto expected_size = writer->cumulative_size_ + + writer->chunks_per_shard_ * 2 * sizeof(uint64_t); + if (file_size > expected_size) { + fs::resize_file(writer->file_path_, expected_size); + } + + writer.reset(); + return true; } \ No newline at end of file diff --git a/src/streaming/shard.writer.hh b/src/streaming/shard.writer.hh index 15df293..30593b6 100644 --- a/src/streaming/shard.writer.hh +++ b/src/streaming/shard.writer.hh @@ -3,43 +3,35 @@ #include "thread.pool.hh" #include +#include #include #include - -#ifdef min -#undef min -#endif +#include namespace zarr { -struct ChunkIndex -{ - uint32_t buffer_idx; - uint64_t offset; - uint64_t size; -}; - class ShardWriter { public: using ChunkBufferPtr = std::vector*; - ShardWriter(std::shared_ptr thread_pool, + ShardWriter(std::string_view file_path, uint32_t chunks_before_flush, uint32_t chunks_per_shard); + ~ShardWriter() = default; - void add_chunk(ChunkBufferPtr buffer, uint32_t chunk_buffer_index); + void add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard); private: uint32_t chunks_before_flush_; uint32_t chunks_per_shard_; uint32_t chunks_flushed_; + std::string file_path_; std::vector index_table_; - std::shared_ptr thread_pool_; - std::mutex mutex_; std::condition_variable cv_; + std::vector chunks_; uint64_t cumulative_size_; uint64_t file_offset_; @@ -47,5 +39,11 @@ class ShardWriter void set_offset_extent_(uint32_t shard_internal_index, uint64_t offset, uint64_t size); + [[nodiscard]] bool flush_(); + + friend bool finalize_shard_writer(std::unique_ptr&& writer); }; + +bool +finalize_shard_writer(std::unique_ptr&& writer); } // namespace zarr \ No newline at end of file diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp index 18b57f8..8753247 100644 --- a/src/streaming/vectorized.file.writer.cpp +++ b/src/streaming/vectorized.file.writer.cpp @@ -33,11 +33,11 @@ get_last_error_as_string() } size_t -get_sector_size(const std::string& path) +get_sector_size(std::string_view path) { // Get volume root path char volume_path[MAX_PATH]; - if (!GetVolumePathNameA(path.c_str(), volume_path, MAX_PATH)) { + if (!GetVolumePathNameA(path.data(), volume_path, MAX_PATH)) { return 0; } @@ -65,7 +65,7 @@ get_last_error_as_string() #endif } // namespace -zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) +zarr::VectorizedFileWriter::VectorizedFileWriter(std::string_view path) { #ifdef _WIN32 SYSTEM_INFO si; @@ -77,7 +77,7 @@ zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) throw std::runtime_error("Failed to get sector size"); } - handle_ = CreateFileA(path.c_str(), + handle_ = CreateFileA(path.data(), GENERIC_WRITE, 0, // No sharing nullptr, @@ -87,13 +87,14 @@ zarr::VectorizedFileWriter::VectorizedFileWriter(const std::string& path) nullptr); if (handle_ == INVALID_HANDLE_VALUE) { auto err = get_last_error_as_string(); - throw std::runtime_error("Failed to open file '" + path + "': " + err); + throw std::runtime_error("Failed to open file '" + std::string(path) + + "': " + err); } #else page_size_ = sysconf(_SC_PAGESIZE); - fd_ = open(path.c_str(), O_WRONLY | O_CREAT, 0644); + fd_ = open(path.data(), O_WRONLY | O_CREAT, 0644); if (fd_ < 0) { - throw std::runtime_error("Failed to open file: " + path); + throw std::runtime_error("Failed to open file: " + std::string(path)); } #endif } diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh index 92350ea..0e15c8d 100644 --- a/src/streaming/vectorized.file.writer.hh +++ b/src/streaming/vectorized.file.writer.hh @@ -1,12 +1,15 @@ #pragma once +#include +#include #include +#include #include -#include -#include #ifdef _WIN32 #include +#undef min +#undef max #else #include #include @@ -17,7 +20,7 @@ namespace zarr { class VectorizedFileWriter { public: - explicit VectorizedFileWriter(const std::string& path); + explicit VectorizedFileWriter(std::string_view path); ~VectorizedFileWriter(); bool write_vectors(const std::vector>& buffers, diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index ed070ba..4eacc16 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -83,7 +83,7 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( shards_ready_.resize(n_shards); for (auto& shard : shards_ready_) { shard.reset( - new ShardWriter(thread_pool_, chunks_before_flush, chunks_per_shard)); + new ShardWriter("foo", chunks_before_flush, chunks_per_shard)); } } diff --git a/tests/unit-tests/CMakeLists.txt b/tests/unit-tests/CMakeLists.txt index fba62cc..f44373e 100644 --- a/tests/unit-tests/CMakeLists.txt +++ b/tests/unit-tests/CMakeLists.txt @@ -26,6 +26,7 @@ set(tests zarrv3-writer-write-ragged-append-dim zarrv3-writer-write-ragged-internal-dim vectorized-file-write + shard-writer-add-chunk-data-to-flush ) foreach (name ${tests}) diff --git a/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp new file mode 100644 index 0000000..32a2573 --- /dev/null +++ b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp @@ -0,0 +1,131 @@ +#include "shard.writer.hh" +#include "thread.pool.hh" +#include "unit.test.macros.hh" + +#include +#include +#include +#include + +namespace fs = std::filesystem; + +void +verify_file_data(const std::string& filename) +{ + const size_t file_size = 1024 * 8; + std::ifstream file(filename, std::ios::binary); + std::vector read_buffer(file_size); + + file.read(reinterpret_cast(read_buffer.data()), file_size); + CHECK(file.good() && file.gcount() == file_size); + + int values[] = { 0, 7, 3, 1, 5, 6, 2, 4 }; + + size_t offset = 0; + for (size_t i = 0; i < 8; ++i) { + for (size_t j = offset; j < offset + 1024; ++j) { + auto byte = (int)read_buffer[j]; + EXPECT(byte == values[i], + "Data mismatch at offset ", + j, + ". Expected ", + i, + " got ", + byte, + "."); + } + offset += 1024; + } + + // check the index table + std::vector index_table(16); + file.read(reinterpret_cast(index_table.data()), + 2 * 8 * sizeof(uint64_t)); + CHECK(file.good() && file.gcount() == 2 * 8 * sizeof(uint64_t)); + + // chunk 0 + EXPECT(index_table[0] == 0, "Expected 0, got ", index_table[0]); + EXPECT(index_table[1] == 1024, "Expected 1024, got ", index_table[1]); + + // chunk 1 + EXPECT(index_table[2] == 3 * 1024, "Expected 3072, got ", index_table[2]); + EXPECT(index_table[3] == 1024, "Expected 1024, got ", index_table[3]); + + // chunk 2 + EXPECT(index_table[4] == 6 * 1024, "Expected 6144, got ", index_table[4]); + EXPECT(index_table[5] == 1024, "Expected 1024, got ", index_table[5]); + + // chunk 3 + EXPECT(index_table[6] == 2 * 1024, "Expected 2048, got ", index_table[6]); + EXPECT(index_table[7] == 1024, "Expected 1024, got ", index_table[7]); + + // chunk 4 + EXPECT(index_table[8] == 7 * 1024, "Expected 7168, got ", index_table[8]); + EXPECT(index_table[9] == 1024, "Expected 1024, got ", index_table[9]); + + // chunk 5 + EXPECT(index_table[10] == 4 * 1024, "Expected 4096, got ", index_table[10]); + EXPECT(index_table[11] == 1024, "Expected 1024, got ", index_table[11]); + + // chunk 6 + EXPECT(index_table[12] == 5 * 1024, "Expected 5120, got ", index_table[12]); + EXPECT(index_table[13] == 1024, "Expected 1024, got ", index_table[13]); + + // chunk 7 + EXPECT(index_table[14] == 1 * 1024, "Expected 1024, got ", index_table[14]); + EXPECT(index_table[15] == 1024, "Expected 1024, got ", index_table[15]); +} + +int +main() +{ + int retval = 1; + + const uint32_t chunks_before_flush = 2; + const uint32_t chunks_per_shard = 8; + + auto shard_file_path = fs::temp_directory_path() / "shard-data.bin"; + auto writer = std::make_unique( + shard_file_path.string(), chunks_before_flush, chunks_per_shard); + + const auto index_table_size = chunks_per_shard * 2 * sizeof(uint64_t); + + std::vector> chunk_data(chunks_per_shard); + for (auto i = 0; i < chunks_per_shard; ++i) { + chunk_data[i].resize(1024); + std::fill(chunk_data[i].begin(), chunk_data[i].end(), std::byte(i)); + } + + try { + writer->add_chunk(&chunk_data[0], 0); + writer->add_chunk(&chunk_data[7], 7); + writer->add_chunk(&chunk_data[3], 3); + writer->add_chunk(&chunk_data[1], 1); + writer->add_chunk(&chunk_data[5], 5); + writer->add_chunk(&chunk_data[6], 6); + writer->add_chunk(&chunk_data[2], 2); + writer->add_chunk(&chunk_data[4], 4); + zarr::finalize_shard_writer(std::move(writer)); + + auto expected_file_size = 1024 * 8 + index_table_size; + auto actual_file_size = fs::file_size(shard_file_path); + EXPECT(actual_file_size == expected_file_size, + "Expected a file size of ", + expected_file_size, + ", got ", + actual_file_size); + + verify_file_data(shard_file_path.string()); + + retval = 0; + } catch (const std::exception& exc) { + LOG_ERROR("Exception: ", exc.what()); + } + + // cleanup + if (fs::exists(shard_file_path)) { + fs::remove(shard_file_path); + } + + return retval; +} \ No newline at end of file From a3e2aca24f560f2fe3b5b987cf2ed12c3787dfca Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 10 Dec 2024 15:34:43 -0500 Subject: [PATCH 18/26] Implement and test make_dirs. --- src/streaming/zarr.common.cpp | 116 +++++++++++++++++++++++++++++++- src/streaming/zarr.common.hh | 34 +++++++++- tests/unit-tests/CMakeLists.txt | 5 +- tests/unit-tests/make-dirs.cpp | 45 +++++++++++++ 4 files changed, 196 insertions(+), 4 deletions(-) create mode 100644 tests/unit-tests/make-dirs.cpp diff --git a/src/streaming/zarr.common.cpp b/src/streaming/zarr.common.cpp index f70fde0..ff2b837 100644 --- a/src/streaming/zarr.common.cpp +++ b/src/streaming/zarr.common.cpp @@ -1,8 +1,14 @@ #include "macros.hh" #include "zarr.common.hh" -#include #include +#include +#include +#include +#include +#include + +namespace fs = std::filesystem; std::string zarr::trim(std::string_view s) @@ -91,3 +97,111 @@ zarr::shards_along_dimension(const ZarrDimension& dimension) const auto n_chunks = chunks_along_dimension(dimension); return (n_chunks + shard_size - 1) / shard_size; } + +std::vector +zarr::construct_data_paths( + std::string_view base_path, + const ArrayDimensions& dimensions, + const std::function& parts_along_dimension) +{ + std::queue paths_queue; + paths_queue.emplace(base_path); + + // create intermediate paths + for (auto i = 1; // skip the last dimension + i < dimensions.ndims() - 1; // skip the x dimension + ++i) { + const auto& dim = dimensions.at(i); + const auto n_parts = parts_along_dimension(dim); + CHECK(n_parts); + + auto n_paths = paths_queue.size(); + for (auto j = 0; j < n_paths; ++j) { + const auto path = paths_queue.front(); + paths_queue.pop(); + + for (auto k = 0; k < n_parts; ++k) { + const auto kstr = std::to_string(k); + paths_queue.push(path + (path.empty() ? kstr : "/" + kstr)); + } + } + } + + // create final paths + std::vector paths_out; + paths_out.reserve(paths_queue.size() * + parts_along_dimension(dimensions.width_dim())); + { + const auto& dim = dimensions.width_dim(); + const auto n_parts = parts_along_dimension(dim); + CHECK(n_parts); + + auto n_paths = paths_queue.size(); + for (auto i = 0; i < n_paths; ++i) { + const auto path = paths_queue.front(); + paths_queue.pop(); + for (auto j = 0; j < n_parts; ++j) + paths_out.push_back(path + "/" + std::to_string(j)); + } + } + + return paths_out; +} + +std::vector +zarr::get_parent_paths(const std::vector& file_paths) +{ + std::unordered_set unique_paths; + for (const auto& file_path : file_paths) { + unique_paths.emplace(fs::path(file_path).parent_path().string()); + } + + return std::vector(unique_paths.begin(), unique_paths.end()); +} + +bool +zarr::make_dirs(const std::vector& dir_paths, + std::shared_ptr thread_pool) +{ + if (dir_paths.empty()) { + return true; + } + EXPECT(thread_pool, "Thread pool not provided."); + + std::atomic all_successful = 1; + + std::unordered_set unique_paths(dir_paths.begin(), + dir_paths.end()); + + std::latch latch(unique_paths.size()); + for (const auto& path : unique_paths) { + auto job = [&path, &latch, &all_successful](std::string& err) { + bool success = true; + if (fs::is_directory(path)) { + latch.count_down(); + return success; + } + + std::error_code ec; + if (!fs::create_directories(path, ec)) { + err = + "Failed to create directory '" + path + "': " + ec.message(); + success = false; + } + + latch.count_down(); + all_successful.fetch_and(static_cast(success)); + + return success; + }; + + if (!thread_pool->push_job(std::move(job))) { + LOG_ERROR("Failed to push job to thread pool."); + return false; + } + } + + latch.wait(); + + return static_cast(all_successful); +} diff --git a/src/streaming/zarr.common.hh b/src/streaming/zarr.common.hh index e45241d..43a2e00 100644 --- a/src/streaming/zarr.common.hh +++ b/src/streaming/zarr.common.hh @@ -1,7 +1,8 @@ #pragma once -#include "zarr.dimension.hh" #include "acquire.zarr.h" +#include "thread.pool.hh" +#include "zarr.dimension.hh" namespace zarr { /** @@ -60,4 +61,35 @@ chunks_along_dimension(const ZarrDimension& dimension); */ uint32_t shards_along_dimension(const ZarrDimension& dimension); + +/** + * @brief Construct paths for data sinks, given the dimensions and a function + * to determine the number of parts along a dimension. + * @param base_path The base path for the dataset. + * @param dimensions The dimensions of the dataset. + * @param parts_along_dimension Function to determine the number of parts + */ +std::vector +construct_data_paths( + std::string_view base_path, + const ArrayDimensions& dimensions, + const std::function& parts_along_dimension); + +/** + * @brief Get unique paths to the parent directories of each file in @p file_paths. + * @param file_paths Collection of paths to files. + * @return Collection of unique parent directories. + */ +std::vector +get_parent_paths(const std::vector& file_paths); + +/** + * @brief Parallel create directories for a collection of paths. + * @param dir_paths The directories to create. + * @param thread_pool The thread pool to use for parallel creation. + * @return True iff all directories were created successfully. + */ +bool +make_dirs(const std::vector& dir_paths, + std::shared_ptr thread_pool); } // namespace zarr \ No newline at end of file diff --git a/tests/unit-tests/CMakeLists.txt b/tests/unit-tests/CMakeLists.txt index f44373e..25d6ef1 100644 --- a/tests/unit-tests/CMakeLists.txt +++ b/tests/unit-tests/CMakeLists.txt @@ -8,6 +8,7 @@ set(tests array-dimensions-shard-index-for-chunk array-dimensions-shard-internal-index thread-pool-push-to-job-queue + make-dirs s3-connection-bucket-exists s3-connection-object-exists-check-false-positives s3-connection-put-object @@ -22,11 +23,11 @@ set(tests zarrv2-writer-write-even zarrv2-writer-write-ragged-append-dim zarrv2-writer-write-ragged-internal-dim + vectorized-file-write + shard-writer-add-chunk-data-to-flush zarrv3-writer-write-even zarrv3-writer-write-ragged-append-dim zarrv3-writer-write-ragged-internal-dim - vectorized-file-write - shard-writer-add-chunk-data-to-flush ) foreach (name ${tests}) diff --git a/tests/unit-tests/make-dirs.cpp b/tests/unit-tests/make-dirs.cpp new file mode 100644 index 0000000..d465b4e --- /dev/null +++ b/tests/unit-tests/make-dirs.cpp @@ -0,0 +1,45 @@ +#include "unit.test.macros.hh" +#include "zarr.common.hh" + +#include + +namespace fs = std::filesystem; + +int +main() +{ + int retval = 1; + auto temp_dir = fs::temp_directory_path() / TEST; + + auto thread_pool = std::make_shared( + std::thread::hardware_concurrency(), + [](const std::string& err) { LOG_ERROR("Error: ", err); }); + + std::vector dir_paths = { (temp_dir / "a").string(), + (temp_dir / "b/c").string(), + (temp_dir / "d/e/f").string() }; + + try { + for (const auto& dir_path : dir_paths) { + EXPECT( + !fs::exists(dir_path), "Directory ", dir_path, " already exists"); + } + + EXPECT(zarr::make_dirs(dir_paths, thread_pool)); + for (const auto& dir_path : dir_paths) { + EXPECT(fs::is_directory(temp_dir / dir_path), + "Failed to create directory ", + dir_path); + } + retval = 0; + } catch (const std::exception& exc) { + LOG_ERROR("Exception: ", exc.what()); + } + + // cleanup + if (fs::exists(temp_dir)) { + fs::remove_all(temp_dir); + } + + return retval; +} \ No newline at end of file From 5d6ce8d545837776140c0ccc65ae1148051f3863 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 10 Dec 2024 16:05:52 -0500 Subject: [PATCH 19/26] Rename SinkCreator methods to be more explicit. --- src/streaming/array.writer.cpp | 4 +- src/streaming/sink.creator.cpp | 94 ++++++------------- src/streaming/sink.creator.hh | 14 +-- src/streaming/zarr.stream.cpp | 10 +- tests/unit-tests/create-stream.cpp | 11 ++- .../sink-creator-make-data-sinks.cpp | 24 ++--- .../sink-creator-make-metadata-sinks.cpp | 12 +-- 7 files changed, 70 insertions(+), 99 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 9065f56..74c5cf3 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -179,9 +179,9 @@ zarr::ArrayWriter::make_metadata_sink_() if (is_s3_array_()) { SinkCreator creator(thread_pool_, s3_connection_pool_); metadata_sink_ = - creator.make_sink(*config_.bucket_name, metadata_path); + creator.make_s3_sink(*config_.bucket_name, metadata_path); } else { - metadata_sink_ = zarr::SinkCreator::make_sink(metadata_path); + metadata_sink_ = zarr::SinkCreator::make_file_sink(metadata_path); } if (!metadata_sink_) { diff --git a/src/streaming/sink.creator.cpp b/src/streaming/sink.creator.cpp index d1343c2..ddc18f5 100644 --- a/src/streaming/sink.creator.cpp +++ b/src/streaming/sink.creator.cpp @@ -1,8 +1,9 @@ -#include "macros.hh" -#include "sink.creator.hh" +#include "acquire.zarr.h" #include "file.sink.hh" +#include "macros.hh" #include "s3.sink.hh" -#include "acquire.zarr.h" +#include "sink.creator.hh" +#include "zarr.common.hh" #include #include @@ -20,7 +21,7 @@ zarr::SinkCreator::SinkCreator( } std::unique_ptr -zarr::SinkCreator::make_sink(std::string_view file_path) +zarr::SinkCreator::make_file_sink(std::string_view file_path) { if (file_path.starts_with("file://")) { file_path = file_path.substr(7); @@ -46,8 +47,8 @@ zarr::SinkCreator::make_sink(std::string_view file_path) } std::unique_ptr -zarr::SinkCreator::make_sink(std::string_view bucket_name, - std::string_view object_key) +zarr::SinkCreator::make_s3_sink(std::string_view bucket_name, + std::string_view object_key) { EXPECT(!bucket_name.empty(), "Bucket name must not be empty."); EXPECT(!object_key.empty(), "Object key must not be empty."); @@ -61,7 +62,7 @@ zarr::SinkCreator::make_sink(std::string_view bucket_name, } bool -zarr::SinkCreator::make_data_sinks( +zarr::SinkCreator::make_data_file_sinks( std::string_view base_path, const ArrayDimensions* dimensions, const std::function& parts_along_dimension, @@ -73,20 +74,27 @@ zarr::SinkCreator::make_data_sinks( EXPECT(!base_path.empty(), "Base path must not be empty."); - std::queue paths; + std::vector paths; try { - paths = make_data_sink_paths_( - base_path, dimensions, parts_along_dimension, true); + paths = + construct_data_paths(base_path, *dimensions, parts_along_dimension); + auto parent_paths = get_parent_paths(paths); + EXPECT(make_dirs(parent_paths, thread_pool_), + "Failed to create dataset paths."); } catch (const std::exception& exc) { LOG_ERROR("Failed to create dataset paths: ", exc.what()); return false; } - return make_files_(paths, part_sinks); + std::queue paths_queue; + for (const auto& path : paths) { + paths_queue.push(path); + } + return make_files_(paths_queue, part_sinks); } bool -zarr::SinkCreator::make_data_sinks( +zarr::SinkCreator::make_data_s3_sinks( std::string_view bucket_name, std::string_view base_path, const ArrayDimensions* dimensions, @@ -102,7 +110,7 @@ zarr::SinkCreator::make_data_sinks( } bool -zarr::SinkCreator::make_metadata_sinks( +zarr::SinkCreator::make_metadata_file_sinks( size_t version, std::string_view base_path, std::unordered_map>& metadata_sinks) @@ -119,7 +127,7 @@ zarr::SinkCreator::make_metadata_sinks( } bool -zarr::SinkCreator::make_metadata_sinks( +zarr::SinkCreator::make_metadata_s3_sinks( size_t version, std::string_view bucket_name, std::string_view base_path, @@ -255,57 +263,17 @@ zarr::SinkCreator::make_dirs_(std::queue& dir_paths) return true; } - std::atomic all_successful = 1; - - const auto n_dirs = dir_paths.size(); - std::latch latch(n_dirs); - - for (auto i = 0; i < n_dirs; ++i) { - const auto dirname = dir_paths.front(); + std::vector paths; + while (!dir_paths.empty()) { + paths.emplace_back(dir_paths.front()); dir_paths.pop(); - - EXPECT(thread_pool_->push_job( - [dirname, &latch, &all_successful](std::string& err) -> bool { - if (dirname.empty()) { - err = "Directory name must not be empty."; - latch.count_down(); - all_successful.fetch_and(0); - return false; - } - - if (fs::is_directory(dirname)) { - latch.count_down(); - return true; - } else if (fs::exists(dirname)) { - err = - "'" + dirname + "' exists but is not a directory"; - latch.count_down(); - all_successful.fetch_and(0); - return false; - } - - if (all_successful) { - std::error_code ec; - if (!fs::create_directories(dirname, ec)) { - err = "Failed to create directory '" + dirname + - "': " + ec.message(); - latch.count_down(); - all_successful.fetch_and(0); - return false; - } - } - - latch.count_down(); - return true; - }), - "Failed to push job to thread pool."); - - dir_paths.push(dirname); } - latch.wait(); + for (const auto& path : paths) { + dir_paths.push(path); + } - return (bool)all_successful; + return make_dirs(paths, thread_pool_); } bool @@ -345,7 +313,7 @@ zarr::SinkCreator::make_files_(std::queue& file_paths, } latch.count_down(); - all_successful.fetch_and((char)success); + all_successful.fetch_and(static_cast(success)); return success; }), @@ -354,7 +322,7 @@ zarr::SinkCreator::make_files_(std::queue& file_paths, latch.wait(); - return (bool)all_successful; + return static_cast(all_successful); } bool diff --git a/src/streaming/sink.creator.hh b/src/streaming/sink.creator.hh index b7f6e68..704c412 100644 --- a/src/streaming/sink.creator.hh +++ b/src/streaming/sink.creator.hh @@ -24,7 +24,7 @@ class SinkCreator * opened. * @throws std::runtime_error if the file path is not valid. */ - static std::unique_ptr make_sink(std::string_view file_path); + static std::unique_ptr make_file_sink(std::string_view file_path); /** * @brief Create a sink from an S3 bucket name and object key. @@ -35,8 +35,8 @@ class SinkCreator * @throws std::runtime_error if the bucket name or object key is not valid, * or if there is no connection pool. */ - std::unique_ptr make_sink(std::string_view bucket_name, - std::string_view object_key); + std::unique_ptr make_s3_sink(std::string_view bucket_name, + std::string_view object_key); /** * @brief Create a collection of file sinks for a Zarr dataset. @@ -49,7 +49,7 @@ class SinkCreator * @throws std::runtime_error if @p base_path is not valid, or if the number * of parts along a dimension is zero. */ - [[nodiscard]] bool make_data_sinks( + [[nodiscard]] bool make_data_file_sinks( std::string_view base_path, const ArrayDimensions* dimensions, const std::function& parts_along_dimension, @@ -66,7 +66,7 @@ class SinkCreator * @param[out] part_sinks The sinks created. * @return True iff all file sinks were created successfully. */ - [[nodiscard]] bool make_data_sinks( + [[nodiscard]] bool make_data_s3_sinks( std::string_view bucket_name, std::string_view base_path, const ArrayDimensions* dimensions, @@ -82,7 +82,7 @@ class SinkCreator * @throws std::runtime_error if @p base_uri is not valid, or if, for S3 * sinks, the bucket does not exist. */ - [[nodiscard]] bool make_metadata_sinks( + [[nodiscard]] bool make_metadata_file_sinks( size_t version, std::string_view base_path, std::unordered_map>& metadata_sinks); @@ -97,7 +97,7 @@ class SinkCreator * @throws std::runtime_error if @p version is invalid, if @p bucket_name is * empty or does not exist, or if @p base_path is empty. */ - [[nodiscard]] bool make_metadata_sinks( + [[nodiscard]] bool make_metadata_s3_sinks( size_t version, std::string_view bucket_name, std::string_view base_path, diff --git a/src/streaming/zarr.stream.cpp b/src/streaming/zarr.stream.cpp index 1f1f8cb..de166d7 100644 --- a/src/streaming/zarr.stream.cpp +++ b/src/streaming/zarr.stream.cpp @@ -664,15 +664,15 @@ ZarrStream_s::create_metadata_sinks_() try { if (s3_connection_pool_) { - if (!creator.make_metadata_sinks(version_, - s3_settings_->bucket_name, - store_path_, - metadata_sinks_)) { + if (!creator.make_metadata_s3_sinks(version_, + s3_settings_->bucket_name, + store_path_, + metadata_sinks_)) { set_error_("Error creating metadata sinks"); return false; } } else { - if (!creator.make_metadata_sinks( + if (!creator.make_metadata_file_sinks( version_, store_path_, metadata_sinks_)) { set_error_("Error creating metadata sinks"); return false; diff --git a/tests/unit-tests/create-stream.cpp b/tests/unit-tests/create-stream.cpp index 2dc8e39..a9f4df5 100644 --- a/tests/unit-tests/create-stream.cpp +++ b/tests/unit-tests/create-stream.cpp @@ -46,21 +46,24 @@ main() memset(&settings, 0, sizeof(settings)); settings.version = ZarrVersion_2; + std::string store_path = + (fs::temp_directory_path() / (TEST ".zarr")).string(); + try { // try to create a stream with no store path stream = ZarrStream_create(&settings); - CHECK(nullptr == stream); + CHECK(!stream); // try to create a stream with no dimensions - settings.store_path = static_cast(TEST ".zarr"); + settings.store_path = store_path.c_str(); stream = ZarrStream_create(&settings); - CHECK(nullptr == stream); + CHECK(!stream); CHECK(!fs::exists(settings.store_path)); // allocate dimensions configure_stream_dimensions(&settings); stream = ZarrStream_create(&settings); - CHECK(nullptr != stream); + CHECK(stream); CHECK(fs::is_directory(settings.store_path)); retval = 0; diff --git a/tests/unit-tests/sink-creator-make-data-sinks.cpp b/tests/unit-tests/sink-creator-make-data-sinks.cpp index cac596a..4b13a1b 100644 --- a/tests/unit-tests/sink-creator-make-data-sinks.cpp +++ b/tests/unit-tests/sink-creator-make-data-sinks.cpp @@ -56,7 +56,7 @@ sink_creator_make_chunk_sinks(std::shared_ptr thread_pool, // create the sinks, then let them go out of scope to close the handles { std::vector> sinks; - CHECK(sink_creator.make_data_sinks( + CHECK(sink_creator.make_data_file_sinks( test_dir, dimensions, zarr::chunks_along_dimension, sinks)); } @@ -94,11 +94,11 @@ sink_creator_make_chunk_sinks( char data_[] = { 0, 0 }; std::span data(reinterpret_cast(data_), sizeof(data_)); std::vector> sinks; - CHECK(sink_creator.make_data_sinks(bucket_name, - test_dir, - dimensions, - zarr::chunks_along_dimension, - sinks)); + CHECK(sink_creator.make_data_s3_sinks(bucket_name, + test_dir, + dimensions, + zarr::chunks_along_dimension, + sinks)); for (auto& sink : sinks) { CHECK(sink); @@ -142,7 +142,7 @@ sink_creator_make_shard_sinks(std::shared_ptr thread_pool, // create the sinks, then let them go out of scope to close the handles { std::vector> sinks; - CHECK(sink_creator.make_data_sinks( + CHECK(sink_creator.make_data_file_sinks( test_dir, dimensions, zarr::shards_along_dimension, sinks)); } @@ -180,11 +180,11 @@ sink_creator_make_shard_sinks( char data_[] = { 0, 0 }; std::span data(reinterpret_cast(data_), sizeof(data_)); std::vector> sinks; - CHECK(sink_creator.make_data_sinks(bucket_name, - test_dir, - dimensions, - zarr::shards_along_dimension, - sinks)); + CHECK(sink_creator.make_data_s3_sinks(bucket_name, + test_dir, + dimensions, + zarr::shards_along_dimension, + sinks)); for (auto& sink : sinks) { CHECK(sink); diff --git a/tests/unit-tests/sink-creator-make-metadata-sinks.cpp b/tests/unit-tests/sink-creator-make-metadata-sinks.cpp index c400079..00cdbff 100644 --- a/tests/unit-tests/sink-creator-make-metadata-sinks.cpp +++ b/tests/unit-tests/sink-creator-make-metadata-sinks.cpp @@ -52,7 +52,7 @@ sink_creator_make_v2_metadata_sinks( zarr::SinkCreator sink_creator(thread_pool, nullptr); std::unordered_map> metadata_sinks; - CHECK(sink_creator.make_metadata_sinks(2, test_dir, metadata_sinks)); + CHECK(sink_creator.make_metadata_file_sinks(2, test_dir, metadata_sinks)); CHECK(metadata_sinks.size() == 4); CHECK(metadata_sinks.contains(".zattrs")); @@ -82,8 +82,8 @@ sink_creator_make_v2_metadata_sinks( zarr::SinkCreator sink_creator(thread_pool, connection_pool); std::unordered_map> metadata_sinks; - CHECK( - sink_creator.make_metadata_sinks(2, bucket_name, test_dir, metadata_sinks)); + CHECK(sink_creator.make_metadata_s3_sinks( + 2, bucket_name, test_dir, metadata_sinks)); CHECK(metadata_sinks.size() == 4); CHECK(metadata_sinks.contains(".zattrs")); @@ -118,7 +118,7 @@ sink_creator_make_v3_metadata_sinks( zarr::SinkCreator sink_creator(thread_pool, nullptr); std::unordered_map> metadata_sinks; - CHECK(sink_creator.make_metadata_sinks(3, test_dir, metadata_sinks)); + CHECK(sink_creator.make_metadata_file_sinks(3, test_dir, metadata_sinks)); CHECK(metadata_sinks.size() == 3); CHECK(metadata_sinks.contains("zarr.json")); @@ -147,8 +147,8 @@ sink_creator_make_v3_metadata_sinks( zarr::SinkCreator sink_creator(thread_pool, connection_pool); std::unordered_map> metadata_sinks; - CHECK( - sink_creator.make_metadata_sinks(3, bucket_name, test_dir, metadata_sinks)); + CHECK(sink_creator.make_metadata_s3_sinks( + 3, bucket_name, test_dir, metadata_sinks)); CHECK(metadata_sinks.size() == 3); CHECK(metadata_sinks.contains("zarr.json")); From 7d7d399c83a60918395e1ee372b1cdcaa0287839 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 10 Dec 2024 16:15:50 -0500 Subject: [PATCH 20/26] Make `ArrayWriter::make_data_sinks_()` a pure virtual method. --- src/streaming/array.writer.cpp | 30 ------------------- src/streaming/array.writer.hh | 3 +- src/streaming/zarrv2.array.writer.cpp | 29 ++++++++++++++++-- src/streaming/zarrv2.array.writer.hh | 4 +-- src/streaming/zarrv3.array.writer.cpp | 6 ++-- src/streaming/zarrv3.array.writer.hh | 4 +-- .../array-writer-write-frame-to-chunks.cpp | 5 +--- 7 files changed, 35 insertions(+), 46 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 74c5cf3..5c52058 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -137,36 +137,6 @@ zarr::ArrayWriter::is_s3_array_() const return config_.bucket_name.has_value(); } -bool -zarr::ArrayWriter::make_data_sinks_() -{ - SinkCreator creator(thread_pool_, s3_connection_pool_); - - const auto data_root = data_root_(); - const auto parts_along_dimension = parts_along_dimension_(); - if (is_s3_array_()) { - if (!creator.make_data_sinks(*config_.bucket_name, - data_root, - config_.dimensions.get(), - parts_along_dimension, - data_sinks_)) { - LOG_ERROR("Failed to create data sinks in ", - data_root, - " for bucket ", - *config_.bucket_name); - return false; - } - } else if (!creator.make_data_sinks(data_root, - config_.dimensions.get(), - parts_along_dimension, - data_sinks_)) { - LOG_ERROR("Failed to create data sinks in ", data_root); - return false; - } - - return true; -} - bool zarr::ArrayWriter::make_metadata_sink_() { diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index ed9fc3a..980f1a4 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -80,11 +80,10 @@ class ArrayWriter virtual std::string data_root_() const = 0; virtual std::string metadata_path_() const = 0; - virtual PartsAlongDimensionFun parts_along_dimension_() const = 0; bool is_s3_array_() const; - [[nodiscard]] bool make_data_sinks_(); + [[nodiscard]] virtual bool make_data_sinks_() = 0; [[nodiscard]] bool make_metadata_sink_(); void make_buffers_() noexcept; diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index ee9070d..fb9d490 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -86,10 +86,33 @@ zarr::ZarrV2ArrayWriter::metadata_path_() const "/.zarray"; } -PartsAlongDimensionFun -zarr::ZarrV2ArrayWriter::parts_along_dimension_() const +bool +zarr::ZarrV2ArrayWriter::make_data_sinks_() { - return chunks_along_dimension; + SinkCreator creator(thread_pool_, s3_connection_pool_); + + const auto data_root = data_root_(); + if (is_s3_array_()) { + if (!creator.make_data_s3_sinks(*config_.bucket_name, + data_root, + config_.dimensions.get(), + chunks_along_dimension, + data_sinks_)) { + LOG_ERROR("Failed to create data sinks in ", + data_root, + " for bucket ", + *config_.bucket_name); + return false; + } + } else if (!creator.make_data_file_sinks(data_root, + config_.dimensions.get(), + chunks_along_dimension, + data_sinks_)) { + LOG_ERROR("Failed to create data sinks in ", data_root); + return false; + } + + return true; } void diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index 169d939..c7e4cee 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -16,10 +16,10 @@ class ZarrV2ArrayWriter final : public ArrayWriter private: std::string data_root_() const override; std::string metadata_path_() const override; - PartsAlongDimensionFun parts_along_dimension_() const override; + bool make_data_sinks_() override; + bool should_rollover_() const override; void compress_and_flush_() override; bool flush_impl_() override; bool write_array_metadata_() override; - bool should_rollover_() const override; }; } // namespace zarr diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index 4eacc16..18df087 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -102,10 +102,10 @@ zarr::ZarrV3ArrayWriter::metadata_path_() const std::to_string(config_.level_of_detail) + ".array.json"; } -PartsAlongDimensionFun -zarr::ZarrV3ArrayWriter::parts_along_dimension_() const +bool +zarr::ZarrV3ArrayWriter::make_data_sinks_() { - return shards_along_dimension; + return false; } void diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index aaa131c..120d5d2 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -23,10 +23,10 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::string data_root_() const override; std::string metadata_path_() const override; - PartsAlongDimensionFun parts_along_dimension_() const override; + bool make_data_sinks_() override; + bool should_rollover_() const override; void compress_and_flush_() override; bool flush_impl_() override; bool write_array_metadata_() override; - bool should_rollover_() const override; }; } // namespace zarr diff --git a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp index 92d5681..a155299 100644 --- a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp +++ b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp @@ -15,10 +15,7 @@ class TestWriter : public zarr::ArrayWriter private: std::string data_root_() const override { return ""; } std::string metadata_path_() const override { return ""; } - PartsAlongDimensionFun parts_along_dimension_() const override - { - return {}; - }; + bool make_data_sinks_() override { return true; } bool should_rollover_() const override { return false; } void compress_and_flush_() override {} bool flush_impl_() override { return true; } From bc4ff3b743e6826b8ee8730f5b963b3ce6a5bf76 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 11 Dec 2024 10:10:55 -0500 Subject: [PATCH 21/26] Add missing EXPECT error message to make-dirs.cpp. --- tests/unit-tests/make-dirs.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit-tests/make-dirs.cpp b/tests/unit-tests/make-dirs.cpp index d465b4e..f11e119 100644 --- a/tests/unit-tests/make-dirs.cpp +++ b/tests/unit-tests/make-dirs.cpp @@ -25,7 +25,8 @@ main() !fs::exists(dir_path), "Directory ", dir_path, " already exists"); } - EXPECT(zarr::make_dirs(dir_paths, thread_pool)); + EXPECT(zarr::make_dirs(dir_paths, thread_pool), + "Failed to create dirs."); for (const auto& dir_path : dir_paths) { EXPECT(fs::is_directory(temp_dir / dir_path), "Failed to create directory ", From 97ede7c0ada17611a89719d021cdb67b7653fc93 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 11 Dec 2024 11:15:19 -0500 Subject: [PATCH 22/26] Remove `ArrayWriter::flush_impl_()` --- src/streaming/array.writer.hh | 1 - src/streaming/zarrv2.array.writer.cpp | 6 ------ src/streaming/zarrv2.array.writer.hh | 1 - src/streaming/zarrv3.array.writer.cpp | 2 +- tests/unit-tests/array-writer-write-frame-to-chunks.cpp | 1 - 5 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index 980f1a4..6744b82 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -95,7 +95,6 @@ class ArrayWriter virtual void compress_and_flush_() = 0; void flush_(); - [[nodiscard]] virtual bool flush_impl_() = 0; void rollover_(); [[nodiscard]] virtual bool write_array_metadata_() = 0; diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index fb9d490..5a9120d 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -228,12 +228,6 @@ zarr::ZarrV2ArrayWriter::compress_and_flush_() latch.wait(); } -bool -zarr::ZarrV2ArrayWriter::flush_impl_() -{ - return true; -} - bool zarr::ZarrV2ArrayWriter::write_array_metadata_() { diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index c7e4cee..6d4d1ac 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -19,7 +19,6 @@ class ZarrV2ArrayWriter final : public ArrayWriter bool make_data_sinks_() override; bool should_rollover_() const override; void compress_and_flush_() override; - bool flush_impl_() override; bool write_array_metadata_() override; }; } // namespace zarr diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index 18df087..f28f258 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -192,7 +192,7 @@ zarr::ZarrV3ArrayWriter::compress_and_flush_() } bool -zarr::ZarrV3ArrayWriter::flush_impl_() +zarr::ZarrV3ArrayWriter::compress_and_flush_to_s3_() { // write out chunks to shards // std::latch latch(n_shards); diff --git a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp index a155299..cd0acb2 100644 --- a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp +++ b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp @@ -18,7 +18,6 @@ class TestWriter : public zarr::ArrayWriter bool make_data_sinks_() override { return true; } bool should_rollover_() const override { return false; } void compress_and_flush_() override {} - bool flush_impl_() override { return true; } bool write_array_metadata_() override { return true; } }; } // namespace From 72508ced135c25af06c955062de374b0e2b55298 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 11 Dec 2024 13:30:06 -0500 Subject: [PATCH 23/26] Everything except ZarrV3 compressed to filesystem working again. --- src/streaming/array.writer.cpp | 92 ++---- src/streaming/array.writer.hh | 4 +- src/streaming/shard.writer.cpp | 50 ++- src/streaming/shard.writer.hh | 20 +- src/streaming/zarrv2.array.writer.cpp | 130 +++----- src/streaming/zarrv2.array.writer.hh | 1 + src/streaming/zarrv3.array.writer.cpp | 290 ++++++++++-------- src/streaming/zarrv3.array.writer.hh | 7 +- .../array-writer-write-frame-to-chunks.cpp | 1 + .../shard-writer-add-chunk-data-to-flush.cpp | 7 +- 10 files changed, 303 insertions(+), 299 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 5c52058..8c85e6a 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -272,60 +272,47 @@ zarr::ArrayWriter::should_flush_() const return frames_written_ % frames_before_flush == 0; } -void -zarr::ArrayWriter::compress_buffers_() +bool +zarr::ArrayWriter::compress_chunk_buffer_(size_t chunk_index) { if (!config_.compression_params.has_value()) { - return; + return true; } - LOG_DEBUG("Compressing"); + EXPECT(chunk_index < chunk_buffers_.size(), + "Chunk index out of bounds: ", + chunk_index); - BloscCompressionParams params = config_.compression_params.value(); + LOG_DEBUG("Compressing chunk ", chunk_index); + + const auto params = *config_.compression_params; const auto bytes_per_px = bytes_of_type(config_.dtype); - std::scoped_lock lock(buffers_mutex_); - std::latch latch(chunk_buffers_.size()); - for (auto& chunk : chunk_buffers_) { - EXPECT(thread_pool_->push_job( - [¶ms, buf = &chunk, bytes_per_px, &latch]( - std::string& err) -> bool { - bool success = false; - const size_t bytes_of_chunk = buf->size(); - - try { - const auto tmp_size = - bytes_of_chunk + BLOSC_MAX_OVERHEAD; - ChunkBuffer tmp(tmp_size); - const auto nb = - blosc_compress_ctx(params.clevel, - params.shuffle, - bytes_per_px, - bytes_of_chunk, - buf->data(), - tmp.data(), - tmp_size, - params.codec_id.c_str(), - 0 /* blocksize - 0:automatic */, - 1); - - tmp.resize(nb); - buf->swap(tmp); - - success = true; - } catch (const std::exception& exc) { - err = "Failed to compress chunk: " + - std::string(exc.what()); - } - latch.count_down(); - - return success; - }), - "Failed to push to job queue"); + auto& chunk = chunk_buffers_[chunk_index]; + const auto bytes_of_chunk = chunk.size(); + + const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; + ChunkBuffer tmp(tmp_size); + const auto nb = blosc_compress_ctx(params.clevel, + params.shuffle, + bytes_per_px, + bytes_of_chunk, + chunk.data(), + tmp.data(), + tmp_size, + params.codec_id.c_str(), + 0 /* blocksize - 0:automatic */, + 1); + + if (nb <= 0) { + LOG_ERROR("Failed to compress chunk ", chunk_index); + return false; } - // wait for all threads to finish - latch.wait(); + tmp.resize(nb); + chunk.swap(tmp); + + return true; } void @@ -339,11 +326,9 @@ zarr::ArrayWriter::flush_() compress_and_flush_(); const auto should_rollover = should_rollover_(); - if (should_rollover) { - rollover_(); - } if (should_rollover || is_finalizing_) { + rollover_(); CHECK(write_array_metadata_()); } @@ -354,17 +339,6 @@ zarr::ArrayWriter::flush_() bytes_to_flush_ = 0; } -void -zarr::ArrayWriter::close_sinks_() -{ - for (auto i = 0; i < data_sinks_.size(); ++i) { - EXPECT(finalize_sink(std::move(data_sinks_[i])), - "Failed to finalize sink ", - i); - } - data_sinks_.clear(); -} - void zarr::ArrayWriter::rollover_() { diff --git a/src/streaming/array.writer.hh b/src/streaming/array.writer.hh index 6744b82..da66b16 100644 --- a/src/streaming/array.writer.hh +++ b/src/streaming/array.writer.hh @@ -91,7 +91,7 @@ class ArrayWriter virtual bool should_rollover_() const = 0; size_t write_frame_to_chunks_(std::span data); - void compress_buffers_(); + bool compress_chunk_buffer_(size_t chunk_index); virtual void compress_and_flush_() = 0; void flush_(); @@ -99,7 +99,7 @@ class ArrayWriter [[nodiscard]] virtual bool write_array_metadata_() = 0; - void close_sinks_(); + virtual void close_sinks_() = 0; friend bool finalize_array(std::unique_ptr&& writer); }; diff --git a/src/streaming/shard.writer.cpp b/src/streaming/shard.writer.cpp index 6eb17d6..32b05b2 100644 --- a/src/streaming/shard.writer.cpp +++ b/src/streaming/shard.writer.cpp @@ -1,6 +1,7 @@ -#include "shard.writer.hh" #include "macros.hh" +#include "shard.writer.hh" #include "vectorized.file.writer.hh" +#include "zarr.common.hh" #include #include @@ -12,16 +13,14 @@ namespace fs = std::filesystem; #undef max #endif -zarr::ShardWriter::ShardWriter(std::string_view file_path, - uint32_t chunks_before_flush, - uint32_t chunks_per_shard) - : file_path_(file_path) - , chunks_before_flush_{ chunks_before_flush } - , chunks_per_shard_{ chunks_per_shard } +zarr::ShardWriter::ShardWriter(const ShardWriterConfig& config) + : file_path_(config.file_path) + , chunks_before_flush_{ config.chunks_before_flush } + , chunks_per_shard_{ config.chunks_per_shard } , chunks_flushed_{ 0 } , cumulative_size_{ 0 } , file_offset_{ 0 } - , index_table_(2 * chunks_per_shard * sizeof(uint64_t)) + , index_table_(2 * config.chunks_per_shard * sizeof(uint64_t)) { std::fill_n(reinterpret_cast(index_table_.data()), index_table_.size() / sizeof(uint64_t), @@ -41,7 +40,7 @@ zarr::ShardWriter::add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard) chunks_.push_back(buffer); if (chunks_.size() == chunks_before_flush_) { - flush_(); + CHECK(flush_()); } } @@ -111,4 +110,35 @@ zarr::finalize_shard_writer(std::unique_ptr&& writer) writer.reset(); return true; -} \ No newline at end of file +} + +bool +zarr::make_shard_writers( + std::string_view base_path, + uint32_t chunks_before_flush, + uint32_t chunks_per_shard, + const ArrayDimensions& dimensions, + std::shared_ptr thread_pool, + std::vector>& shard_writers) +{ + auto paths = + construct_data_paths(base_path, dimensions, shards_along_dimension); + + auto parent_paths = get_parent_paths(paths); + if (!make_dirs(parent_paths, thread_pool)) { + LOG_ERROR("Failed to create dataset paths."); + return false; + } + + shard_writers.clear(); + shard_writers.reserve(paths.size()); + + for (const auto& path : paths) { + ShardWriterConfig config{ .file_path = path, + .chunks_before_flush = chunks_before_flush, + .chunks_per_shard = chunks_per_shard }; + shard_writers.emplace_back(std::make_unique(config)); + } + + return true; +} diff --git a/src/streaming/shard.writer.hh b/src/streaming/shard.writer.hh index 30593b6..0de84ba 100644 --- a/src/streaming/shard.writer.hh +++ b/src/streaming/shard.writer.hh @@ -1,6 +1,7 @@ #pragma once #include "thread.pool.hh" +#include "zarr.dimension.hh" #include #include @@ -9,14 +10,19 @@ #include namespace zarr { +struct ShardWriterConfig +{ + std::string file_path; + uint32_t chunks_before_flush; + uint32_t chunks_per_shard; +}; + class ShardWriter { public: using ChunkBufferPtr = std::vector*; - ShardWriter(std::string_view file_path, - uint32_t chunks_before_flush, - uint32_t chunks_per_shard); + explicit ShardWriter(const ShardWriterConfig& config); ~ShardWriter() = default; void add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard); @@ -46,4 +52,12 @@ class ShardWriter bool finalize_shard_writer(std::unique_ptr&& writer); + +bool +make_shard_writers(std::string_view base_path, + uint32_t chunks_before_flush, + uint32_t chunks_per_shard, + const ArrayDimensions& dimensions, + std::shared_ptr thread_pool, + std::vector>& shard_writers); } // namespace zarr \ No newline at end of file diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 5a9120d..5c40840 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -133,101 +133,53 @@ zarr::ZarrV2ArrayWriter::compress_and_flush_() std::latch latch(n_chunks); for (auto i = 0; i < n_chunks; ++i) { - auto& chunk = chunk_buffers_[i]; - - if (config_.compression_params) { - auto& params = *config_.compression_params; - auto job = [¶ms, - buf = &chunk, - bytes_per_px, - &sink = data_sinks_[i], - thread_pool = thread_pool_, - &latch](std::string& err) -> bool { - const size_t bytes_of_chunk = buf->size(); - - const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; - ChunkBuffer tmp(tmp_size); - const auto nb = - blosc_compress_ctx(params.clevel, - params.shuffle, - bytes_per_px, - bytes_of_chunk, - buf->data(), - tmp.data(), - tmp_size, - params.codec_id.c_str(), - 0 /* blocksize - 0:automatic */, - 1); - - if (nb <= 0) { - err = "Failed to compress chunk"; - latch.count_down(); - return false; - } - - tmp.resize(nb); - buf->swap(tmp); - - auto queued = thread_pool->push_job( - std::move([&sink, buf, &latch](std::string& err) -> bool { - bool success = false; - - try { - success = sink->write(0, *buf); - } catch (const std::exception& exc) { - err = - "Failed to write chunk: " + std::string(exc.what()); - } - - latch.count_down(); - return success; - })); - - if (!queued) { - err = "Failed to push job to thread pool"; - latch.count_down(); - } - - return queued; - }; - - CHECK(thread_pool_->push_job(std::move(job))); - } else { - auto job = [buf = &chunk, - &sink = data_sinks_[i], - thread_pool = thread_pool_, - &latch](std::string& err) -> bool { - auto queued = thread_pool->push_job( - std::move([&sink, buf, &latch](std::string& err) -> bool { - bool success = false; - - try { - success = sink->write(0, *buf); - } catch (const std::exception& exc) { - err = - "Failed to write chunk: " + std::string(exc.what()); - } - - latch.count_down(); - return success; - })); - - if (!queued) { - err = "Failed to push job to thread pool"; - latch.count_down(); - } - - return queued; - }; - - CHECK(thread_pool_->push_job(std::move(job))); - } + auto job = [this, i, &latch](std::string& err) -> bool { + if (!compress_chunk_buffer_(i)) { // no-op if no compression + err = "Failed to compress chunk buffer"; + return false; + } + + auto queued = thread_pool_->push_job(std::move( + [&sink = data_sinks_[i], buf = &chunk_buffers_[i], &latch]( + std::string& err) -> bool { + bool success = false; + + try { + success = sink->write(0, *buf); + } catch (const std::exception& exc) { + err = "Failed to write chunk: " + std::string(exc.what()); + } + + latch.count_down(); + return success; + })); + + if (!queued) { + err = "Failed to push job to thread pool"; + latch.count_down(); + } + + return queued; + }; + + CHECK(thread_pool_->push_job(std::move(job))); } // wait for all threads to finish latch.wait(); } +void +zarr::ZarrV2ArrayWriter::close_sinks_() +{ + for (auto i = 0; i < data_sinks_.size(); ++i) { + EXPECT(finalize_sink(std::move(data_sinks_[i])), + "Failed to finalize sink ", + i); + } + data_sinks_.clear(); +} + bool zarr::ZarrV2ArrayWriter::write_array_metadata_() { diff --git a/src/streaming/zarrv2.array.writer.hh b/src/streaming/zarrv2.array.writer.hh index 6d4d1ac..4c3619a 100644 --- a/src/streaming/zarrv2.array.writer.hh +++ b/src/streaming/zarrv2.array.writer.hh @@ -19,6 +19,7 @@ class ZarrV2ArrayWriter final : public ArrayWriter bool make_data_sinks_() override; bool should_rollover_() const override; void compress_and_flush_() override; + void close_sinks_() override; bool write_array_metadata_() override; }; } // namespace zarr diff --git a/src/streaming/zarrv3.array.writer.cpp b/src/streaming/zarrv3.array.writer.cpp index f28f258..acd1fb9 100644 --- a/src/streaming/zarrv3.array.writer.cpp +++ b/src/streaming/zarrv3.array.writer.cpp @@ -62,7 +62,6 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( const auto chunks_in_memory = config_.dimensions->number_of_chunks_in_memory(); const auto chunks_per_shard = config_.dimensions->chunks_per_shard(); - const auto chunks_before_flush = chunks_in_memory / n_shards; shard_file_offsets_.resize(n_shards, 0); shard_tables_.resize(n_shards); @@ -79,12 +78,6 @@ zarr::ZarrV3ArrayWriter::ZarrV3ArrayWriter( const auto index = config_.dimensions->shard_index_for_chunk(i); chunk_in_shards_[index].push_back(i); } - - shards_ready_.resize(n_shards); - for (auto& shard : shards_ready_) { - shard.reset( - new ShardWriter("foo", chunks_before_flush, chunks_per_shard)); - } } std::string @@ -105,7 +98,40 @@ zarr::ZarrV3ArrayWriter::metadata_path_() const bool zarr::ZarrV3ArrayWriter::make_data_sinks_() { - return false; + const auto data_root = data_root_(); + + if (is_s3_array_()) { + SinkCreator creator(thread_pool_, s3_connection_pool_); + if (!creator.make_data_s3_sinks(*config_.bucket_name, + data_root, + config_.dimensions.get(), + shards_along_dimension, + data_sinks_)) { + LOG_ERROR("Failed to create data sinks in ", + data_root, + " for bucket ", + *config_.bucket_name); + return false; + } + } else { + const auto n_shards = config_.dimensions->number_of_shards(); + const auto chunks_in_memory = + config_.dimensions->number_of_chunks_in_memory(); + const auto chunks_before_flush = chunks_in_memory / n_shards; + const auto chunks_per_shard = config_.dimensions->chunks_per_shard(); + + if (!make_shard_writers(data_root, + chunks_before_flush, + chunks_per_shard, + *config_.dimensions, + thread_pool_, + shard_writers_)) { + LOG_ERROR("Failed to create shard writers in ", data_root); + return false; + } + } + + return true; } void @@ -117,154 +143,154 @@ zarr::ZarrV3ArrayWriter::compress_and_flush_() } // create shard files if they don't exist - if (data_sinks_.empty()) { - CHECK(make_data_sinks_()); - } - - const auto n_chunks = chunk_buffers_.size(); const auto n_shards = chunk_in_shards_.size(); - CHECK(data_sinks_.size() == n_shards); - - const auto bytes_per_px = bytes_of_type(config_.dtype); - auto write_table = is_finalizing_ || should_rollover_(); + if (is_s3_array_()) { + if (data_sinks_.empty()) { + CHECK(make_data_sinks_()); + } + CHECK(data_sinks_.size() == n_shards); + CHECK(compress_and_flush_to_s3_()); + } else { + if (shard_writers_.empty()) { + CHECK(make_data_sinks_()); + } + CHECK(shard_writers_.size() == n_shards); + CHECK(compress_and_flush_to_filesystem_()); + } +} +bool +zarr::ZarrV3ArrayWriter::compress_and_flush_to_filesystem_() +{ + const auto n_chunks = chunk_buffers_.size(); std::latch latch(n_chunks); for (auto i = 0; i < n_chunks; ++i) { - auto& chunk = chunk_buffers_[i]; const auto shard_index = config_.dimensions->shard_index_for_chunk(i); - auto& shard = shards_ready_[shard_index]; + auto& shard = shard_writers_[shard_index]; const auto internal_index = config_.dimensions->shard_internal_index(i); - if (config_.compression_params) { - auto& params = *config_.compression_params; - auto job = [¶ms, - buf = &chunk, - bytes_per_px, - internal_index, - &shard, - &latch](std::string& err) -> bool { - const size_t bytes_of_chunk = buf->size(); - - const auto tmp_size = bytes_of_chunk + BLOSC_MAX_OVERHEAD; - ChunkBuffer tmp(tmp_size); - const auto nb = - blosc_compress_ctx(params.clevel, - params.shuffle, - bytes_per_px, - bytes_of_chunk, - buf->data(), - tmp.data(), - tmp_size, - params.codec_id.c_str(), - 0 /* blocksize - 0:automatic */, - 1); - - if (nb <= 0) { - err = "Failed to compress chunk"; - latch.count_down(); - return false; - } - - tmp.resize(nb); - buf->swap(tmp); - - shard->add_chunk(buf, internal_index); - - latch.count_down(); + auto job = + [this, i, internal_index, &shard, &latch](std::string& err) -> bool { + bool success = true; + if (compress_chunk_buffer_(i)) { // no-op if compression is disabled + shard->add_chunk(&chunk_buffers_[i], internal_index); + } else { + err = "Failed to compress chunk " + std::to_string(i); + success = false; + } + latch.count_down(); - return true; - }; + return success; + }; - EXPECT(thread_pool_->push_job(job), - "Failed to push job to thread pool"); - } else { - shards_ready_[shard_index]->add_chunk(&chunk, internal_index); - latch.count_down(); - } + EXPECT(thread_pool_->push_job(std::move(job)), + "Failed to push job to thread pool"); } latch.wait(); - // compress_buffers_(); - // CHECK(flush_impl_()); + + return true; } bool zarr::ZarrV3ArrayWriter::compress_and_flush_to_s3_() { + const auto n_shards = chunk_in_shards_.size(); + + const auto write_table = is_finalizing_ || should_rollover_(); + // write out chunks to shards - // std::latch latch(n_shards); - // for (auto i = 0; i < n_shards; ++i) { - // const auto& chunks = chunk_in_shards_.at(i); - // auto& chunk_table = shard_tables_.at(i); - // auto* file_offset = &shard_file_offsets_.at(i); - // - // EXPECT(thread_pool_->push_job([&sink = data_sinks_.at(i), - // &chunks, - // &chunk_table, - // file_offset, - // write_table, - // &latch, - // this](std::string& err) { - // bool success = false; - // - // try { - // for (const auto& chunk_idx : chunks) { - // auto& chunk = chunk_buffers_.at(chunk_idx); - // std::span data{ - // reinterpret_cast(chunk.data()), - // chunk.size() }; - // success = sink->write(*file_offset, data); - // if (!success) { - // break; - // } - // - // const auto internal_idx = - // config_.dimensions->shard_internal_index(chunk_idx); - // chunk_table.at(2 * internal_idx) = *file_offset; - // chunk_table.at(2 * internal_idx + 1) = chunk.size(); - // - // *file_offset += chunk.size(); - // } - // - // if (success && write_table) { - // auto* table = - // reinterpret_cast(chunk_table.data()); - // std::span data{ table, - // chunk_table.size() * sizeof(uint64_t) - // }; - // success = sink->write(*file_offset, data); - // } - // } catch (const std::exception& exc) { - // err = "Failed to write chunk: " + std::string(exc.what()); - // } - // - // latch.count_down(); - // return success; - // }), - // "Failed to push job to thread pool"); - // } - // - // // wait for all threads to finish - // latch.wait(); - // - // // reset shard tables and file offsets - // if (write_table) { - // for (auto& table : shard_tables_) { - // std::fill( - // table.begin(), table.end(), - // std::numeric_limits::max()); - // } - // - // std::fill(shard_file_offsets_.begin(), shard_file_offsets_.end(), - // 0); - // } - // + std::latch latch(n_shards); + + for (auto i = 0; i < n_shards; ++i) { + const auto& chunks = chunk_in_shards_.at(i); + auto& chunk_table = shard_tables_.at(i); + auto* file_offset = &shard_file_offsets_.at(i); + + EXPECT(thread_pool_->push_job([&sink = data_sinks_.at(i), + &chunks, + &chunk_table, + file_offset, + write_table, + &latch, + this](std::string& err) { + bool success = false; + + try { + for (const auto& chunk_idx : chunks) { + // no-op if compression is disabled + compress_chunk_buffer_(chunk_idx); + + auto& chunk = chunk_buffers_.at(chunk_idx); + std::span data{ reinterpret_cast(chunk.data()), + chunk.size() }; + success = sink->write(*file_offset, data); + if (!success) { + break; + } + + const auto internal_idx = + config_.dimensions->shard_internal_index(chunk_idx); + chunk_table.at(2 * internal_idx) = *file_offset; + chunk_table.at(2 * internal_idx + 1) = chunk.size(); + + *file_offset += chunk.size(); + } + + if (success && write_table) { + auto* table = + reinterpret_cast(chunk_table.data()); + std::span data{ table, + chunk_table.size() * sizeof(uint64_t) }; + success = sink->write(*file_offset, data); + } + } catch (const std::exception& exc) { + err = "Failed to write chunk: " + std::string(exc.what()); + } + + latch.count_down(); + return success; + }), + "Failed to push job to thread pool"); + } + + // wait for all threads to finish + latch.wait(); + + // reset shard tables and file offsets + if (write_table) { + for (auto& table : shard_tables_) { + std::fill( + table.begin(), table.end(), std::numeric_limits::max()); + } + + std::fill(shard_file_offsets_.begin(), shard_file_offsets_.end(), 0); + } + return true; } +void +zarr::ZarrV3ArrayWriter::close_sinks_() +{ + for (auto i = 0; i < data_sinks_.size(); ++i) { + EXPECT(finalize_sink(std::move(data_sinks_[i])), + "Failed to finalize sink ", + i); + } + data_sinks_.clear(); + + for (auto i = 0; i < shard_writers_.size(); ++i) { + EXPECT(finalize_shard_writer(std::move(shard_writers_[i])), + "Failed to finalize shard writer ", + i); + } + shard_writers_.clear(); +} + bool zarr::ZarrV3ArrayWriter::write_array_metadata_() { diff --git a/src/streaming/zarrv3.array.writer.hh b/src/streaming/zarrv3.array.writer.hh index 120d5d2..e9f52a5 100644 --- a/src/streaming/zarrv3.array.writer.hh +++ b/src/streaming/zarrv3.array.writer.hh @@ -15,7 +15,7 @@ struct ZarrV3ArrayWriter : public ArrayWriter std::shared_ptr s3_connection_pool); private: - std::vector> shards_ready_; + std::vector> shard_writers_; std::vector shard_file_offsets_; std::vector> shard_tables_; @@ -26,7 +26,10 @@ struct ZarrV3ArrayWriter : public ArrayWriter bool make_data_sinks_() override; bool should_rollover_() const override; void compress_and_flush_() override; - bool flush_impl_() override; + void close_sinks_() override; bool write_array_metadata_() override; + + bool compress_and_flush_to_filesystem_(); + bool compress_and_flush_to_s3_(); }; } // namespace zarr diff --git a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp index cd0acb2..7fd82f8 100644 --- a/tests/unit-tests/array-writer-write-frame-to-chunks.cpp +++ b/tests/unit-tests/array-writer-write-frame-to-chunks.cpp @@ -18,6 +18,7 @@ class TestWriter : public zarr::ArrayWriter bool make_data_sinks_() override { return true; } bool should_rollover_() const override { return false; } void compress_and_flush_() override {} + void close_sinks_() override {} bool write_array_metadata_() override { return true; } }; } // namespace diff --git a/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp index 32a2573..2bc7e4a 100644 --- a/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp +++ b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp @@ -85,8 +85,11 @@ main() const uint32_t chunks_per_shard = 8; auto shard_file_path = fs::temp_directory_path() / "shard-data.bin"; - auto writer = std::make_unique( - shard_file_path.string(), chunks_before_flush, chunks_per_shard); + zarr::ShardWriterConfig config = { .file_path = shard_file_path.string(), + .chunks_before_flush = + chunks_before_flush, + .chunks_per_shard = chunks_per_shard }; + auto writer = std::make_unique(config); const auto index_table_size = chunks_per_shard * 2 * sizeof(uint64_t); From 1fc703f6bda6e674947fb0daf0a31933a298eb49 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Fri, 13 Dec 2024 13:13:35 -0500 Subject: [PATCH 24/26] We did it. --- src/streaming/CMakeLists.txt | 4 +- src/streaming/platform.cpp | 264 ++++++++++++++++++ src/streaming/platform.hh | 46 +++ src/streaming/shard.writer.cpp | 15 +- src/streaming/shard.writer.hh | 5 +- src/streaming/vectorized.file.writer.cpp | 221 --------------- src/streaming/vectorized.file.writer.hh | 44 --- .../shard-writer-add-chunk-data-to-flush.cpp | 157 ++++++++--- tests/unit-tests/vectorized-file-write.cpp | 21 +- 9 files changed, 459 insertions(+), 318 deletions(-) create mode 100644 src/streaming/platform.cpp create mode 100644 src/streaming/platform.hh delete mode 100644 src/streaming/vectorized.file.writer.cpp delete mode 100644 src/streaming/vectorized.file.writer.hh diff --git a/src/streaming/CMakeLists.txt b/src/streaming/CMakeLists.txt index cf1dbaf..aa757a7 100644 --- a/src/streaming/CMakeLists.txt +++ b/src/streaming/CMakeLists.txt @@ -29,10 +29,10 @@ add_library(${tgt} zarrv2.array.writer.cpp zarrv3.array.writer.hh zarrv3.array.writer.cpp - vectorized.file.writer.hh - vectorized.file.writer.cpp shard.writer.hh shard.writer.cpp + platform.hh + platform.cpp ) target_include_directories(${tgt} diff --git a/src/streaming/platform.cpp b/src/streaming/platform.cpp new file mode 100644 index 0000000..b91270c --- /dev/null +++ b/src/streaming/platform.cpp @@ -0,0 +1,264 @@ +#include "platform.hh" +#include "macros.hh" + +#ifdef _WIN32 +#include +#else +#include +#include +#include + +#include +#endif + +namespace { +size_t +align_to(size_t size, size_t alignment) +{ + if (alignment == 0) { + return size; + } + + return (size + alignment - 1) & ~(alignment - 1); +} +} // namespace + +#ifdef _WIN32 +namespace { +size_t _PAGE_SIZE = 0; +size_t _SECTOR_SIZE = 0; + +[[nodiscard]] +size_t +get_sector_size() +{ + if (_SECTOR_SIZE == 0) { + DWORD bytes_per_sector; + GetDiskFreeSpace(NULL, &bytes_per_sector, NULL, NULL, NULL); + _SECTOR_SIZE = bytes_per_sector; + } + return _SECTOR_SIZE; +} + +[[nodiscard]] +size_t +get_page_size() +{ + if (_PAGE_SIZE == 0) { + SYSTEM_INFO sys_info; + GetSystemInfo(&sys_info); + _PAGE_SIZE = sys_info.dwPageSize; + } + return _PAGE_SIZE; +} + +std::string +get_last_error_as_string() +{ + DWORD errorMessageID = ::GetLastError(); + if (errorMessageID == 0) { + return std::string(); // No error message has been recorded + } + + LPSTR messageBuffer = nullptr; + + size_t size = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | + FORMAT_MESSAGE_FROM_SYSTEM | + FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, + errorMessageID, + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + (LPSTR)&messageBuffer, + 0, + NULL); + + std::string message(messageBuffer, size); + + LocalFree(messageBuffer); + + return message; +} +} // namespace + +size_t +zarr::align_to_system_boundary(size_t size) +{ + size = align_to(size, get_page_size()); + return align_to(size, get_sector_size()); +} + +zarr::VectorizedFile::VectorizedFile(std::string_view path) + : inner_(INVALID_HANDLE_VALUE) +{ + inner_ = (void*)CreateFileA(path.data(), + GENERIC_WRITE, + 0, // no sharing + nullptr, + OPEN_ALWAYS, + FILE_FLAG_OVERLAPPED | FILE_FLAG_NO_BUFFERING | + FILE_FLAG_SEQUENTIAL_SCAN, + nullptr); + + EXPECT(inner_ != INVALID_HANDLE_VALUE, "Failed to open file: ", path); +} + +zarr::VectorizedFile::~VectorizedFile() +{ + if (inner_ != INVALID_HANDLE_VALUE) { + CloseHandle((HANDLE)inner_); + } +} + +bool +zarr::file_write_vectorized(VectorizedFile& file, + const std::vector>& buffers, + size_t offset) +{ + auto fh = (HANDLE)file.inner_; + EXPECT(fh != INVALID_HANDLE_VALUE, "Invalid file handle"); + + const auto page_size = get_page_size(); + + bool success = true; + + size_t total_bytes_to_write = 0; + for (const auto& buffer : buffers) { + total_bytes_to_write += buffer.size(); + } + + const size_t nbytes_aligned = + align_to_system_boundary(total_bytes_to_write); + CHECK(nbytes_aligned >= total_bytes_to_write); + + auto* aligned = + static_cast(_aligned_malloc(nbytes_aligned, page_size)); + if (!aligned) { + return false; + } + + auto* cur = aligned; + for (const auto& buffer : buffers) { + std::copy(buffer.begin(), buffer.end(), cur); + cur += buffer.size(); + } + + std::vector segments(nbytes_aligned / page_size); + + cur = aligned; + for (auto& segment : segments) { + memset(&segment, 0, sizeof(segment)); + segment.Buffer = PtrToPtr64(cur); + cur += page_size; + } + + OVERLAPPED overlapped = { 0 }; + overlapped.Offset = static_cast(offset & 0xFFFFFFFF); + overlapped.OffsetHigh = static_cast(offset >> 32); + overlapped.hEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); + + if (!WriteFileGather( + fh, segments.data(), nbytes_aligned, nullptr, &overlapped)) { + if (GetLastError() != ERROR_IO_PENDING) { + LOG_ERROR("Failed to write file : ", get_last_error_as_string()); + success = false; + } + + // Wait for the operation to complete + DWORD bytes_written = 0; + if (success && + !GetOverlappedResult(fh, &overlapped, &bytes_written, TRUE)) { + LOG_ERROR("Failed to get overlapped result: ", + get_last_error_as_string()); + success = false; + } + EXPECT(bytes_written == nbytes_aligned, + "Expected to write ", + nbytes_aligned, + " bytes, wrote ", + bytes_written); + } + + _aligned_free(aligned); + + return success; +} +#else // _WIN32 +namespace { +size_t _PAGE_SIZE = 0; + +size_t +get_page_size() +{ + if (_PAGE_SIZE == 0) { + _PAGE_SIZE = sysconf(_SC_PAGE_SIZE); + } + return _PAGE_SIZE; +} + +std::string +get_last_error_as_string() +{ + return strerror(errno); +} +} // namespace + +size_t +zarr::align_to_system_boundary(size_t size) +{ + return align_to(size, get_page_size()); +} + +zarr::VectorizedFile::VectorizedFile(std::string_view path) + : inner_(nullptr) +{ + inner_ = new int(open(path.data(), O_WRONLY | O_CREAT, 0644)); + EXPECT(inner_ != nullptr, "Failed to open file: ", path); + EXPECT(*(int*)inner_ != -1, "Failed to open file: ", path); +} + +zarr::VectorizedFile::~VectorizedFile() +{ + if (inner_ != nullptr) { + if (*(int*)inner_ != -1) { + close(*(int*)inner_); + } + delete (int*)inner_; + } +} + +bool +zarr::file_write_vectorized(zarr::VectorizedFile& file, + const std::vector>& buffers, + size_t offset) +{ + auto fd = *(int*)file.inner_; + EXPECT(fd != -1, "Invalid file descriptor"); + + std::vector iovecs(buffers.size()); + + for (auto i = 0; i < buffers.size(); ++i) { + auto* iov = &iovecs[i]; + memset(iov, 0, sizeof(struct iovec)); + iov->iov_base = + const_cast(static_cast(buffers[i].data())); + iov->iov_len = buffers[i].size(); + } + + ssize_t total_bytes = 0; + for (const auto& buffer : buffers) { + total_bytes += static_cast(buffer.size()); + } + + ssize_t bytes_written = pwritev(fd, + iovecs.data(), + static_cast(iovecs.size()), + static_cast(offset)); + + if (bytes_written != total_bytes) { + LOG_ERROR("Failed to write file: ", get_last_error_as_string()); + return false; + } + + return true; +} +#endif \ No newline at end of file diff --git a/src/streaming/platform.hh b/src/streaming/platform.hh new file mode 100644 index 0000000..cd974a8 --- /dev/null +++ b/src/streaming/platform.hh @@ -0,0 +1,46 @@ +#pragma once + +#include +#include +#include + +namespace zarr { +class VectorizedFile +{ + public: + explicit VectorizedFile(std::string_view path); + ~VectorizedFile(); + + private: + void* inner_; + + friend bool file_write_vectorized( + VectorizedFile& file, + const std::vector>& buffers, + size_t offset); +}; + +/** + * @brief Align and offset or a size to the nearest system boundary. + * @note Aligns to sector size on Windows, page size on UNIX. + * @param size The offset or size to align. + * @return The aligned offset or size. + */ +size_t +align_to_system_boundary(size_t size); + +/** + * @brief Write a vector of buffers to the file at the given path. + * @param[in] file The VectorizedFile to write to. + * @param[in] buffers The buffers to write. + * @param[in] offset The offset in the file to write to. This value must be + * aligned to the system boundary. + * @throws std::runtime_error if the file cannot be opened for writing, or if + * the offset is not aligned to the system boundary. + * @return True if the write was successful, false otherwise. + */ +bool +file_write_vectorized(VectorizedFile& file, + const std::vector>& buffers, + size_t offset); +} \ No newline at end of file diff --git a/src/streaming/shard.writer.cpp b/src/streaming/shard.writer.cpp index 32b05b2..525c32e 100644 --- a/src/streaming/shard.writer.cpp +++ b/src/streaming/shard.writer.cpp @@ -1,6 +1,5 @@ #include "macros.hh" #include "shard.writer.hh" -#include "vectorized.file.writer.hh" #include "zarr.common.hh" #include @@ -15,6 +14,7 @@ namespace fs = std::filesystem; zarr::ShardWriter::ShardWriter(const ShardWriterConfig& config) : file_path_(config.file_path) + , file_{ std::make_unique(config.file_path) } , chunks_before_flush_{ config.chunks_before_flush } , chunks_per_shard_{ config.chunks_per_shard } , chunks_flushed_{ 0 } @@ -35,6 +35,11 @@ zarr::ShardWriter::add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard) std::unique_lock lock(mutex_); cv_.wait(lock, [this] { return chunks_.size() < chunks_before_flush_; }); + // chunks have been flushed and the new offset is aligned to the sector size + if (chunks_.empty()) { + cumulative_size_ = file_offset_; + } + set_offset_extent_(index_in_shard, cumulative_size_, buffer->size()); cumulative_size_ += buffer->size(); @@ -55,8 +60,7 @@ zarr::ShardWriter::flush_() buffers.emplace_back(index_table_); try { - VectorizedFileWriter writer(file_path_); - writer.write_vectors(buffers, file_offset_); + file_write_vectorized(*file_, buffers, file_offset_); } catch (const std::exception& exc) { LOG_ERROR("Failed to write chunk: ", std::string(exc.what())); return false; @@ -65,7 +69,7 @@ zarr::ShardWriter::flush_() chunks_flushed_ += chunks_.size(); chunks_.clear(); chunks_.reserve(chunks_before_flush_); - file_offset_ = cumulative_size_; + file_offset_ = zarr::align_to_system_boundary(cumulative_size_); cv_.notify_all(); @@ -96,9 +100,12 @@ zarr::finalize_shard_writer(std::unique_ptr&& writer) return true; } + // flush remaining chunks and close file if (!writer->flush_()) { + LOG_ERROR("Failed to flush remaining chunks."); return false; } + writer->file_.reset(); // release file handle // resize file if necessary const auto file_size = fs::file_size(writer->file_path_); diff --git a/src/streaming/shard.writer.hh b/src/streaming/shard.writer.hh index 0de84ba..4b43953 100644 --- a/src/streaming/shard.writer.hh +++ b/src/streaming/shard.writer.hh @@ -1,5 +1,6 @@ #pragma once +#include "platform.hh" #include "thread.pool.hh" #include "zarr.dimension.hh" @@ -28,10 +29,12 @@ class ShardWriter void add_chunk(ChunkBufferPtr buffer, uint32_t index_in_shard); private: + std::string file_path_; + std::unique_ptr file_; + uint32_t chunks_before_flush_; uint32_t chunks_per_shard_; uint32_t chunks_flushed_; - std::string file_path_; std::vector index_table_; diff --git a/src/streaming/vectorized.file.writer.cpp b/src/streaming/vectorized.file.writer.cpp deleted file mode 100644 index 8753247..0000000 --- a/src/streaming/vectorized.file.writer.cpp +++ /dev/null @@ -1,221 +0,0 @@ -#include "vectorized.file.writer.hh" -#include "macros.hh" - -#include - -namespace { -#ifdef _WIN32 -std::string -get_last_error_as_string() -{ - DWORD errorMessageID = ::GetLastError(); - if (errorMessageID == 0) { - return std::string(); // No error message has been recorded - } - - LPSTR messageBuffer = nullptr; - - size_t size = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | - FORMAT_MESSAGE_FROM_SYSTEM | - FORMAT_MESSAGE_IGNORE_INSERTS, - NULL, - errorMessageID, - MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), - (LPSTR)&messageBuffer, - 0, - NULL); - - std::string message(messageBuffer, size); - - LocalFree(messageBuffer); - - return message; -} - -size_t -get_sector_size(std::string_view path) -{ - // Get volume root path - char volume_path[MAX_PATH]; - if (!GetVolumePathNameA(path.data(), volume_path, MAX_PATH)) { - return 0; - } - - DWORD sectors_per_cluster; - DWORD bytes_per_sector; - DWORD number_of_free_clusters; - DWORD total_number_of_clusters; - - if (!GetDiskFreeSpaceA(volume_path, - §ors_per_cluster, - &bytes_per_sector, - &number_of_free_clusters, - &total_number_of_clusters)) { - return 0; - } - - return bytes_per_sector; -} -#else -std::string -get_last_error_as_string() -{ - return strerror(errno); -} -#endif -} // namespace - -zarr::VectorizedFileWriter::VectorizedFileWriter(std::string_view path) -{ -#ifdef _WIN32 - SYSTEM_INFO si; - GetSystemInfo(&si); - page_size_ = si.dwPageSize; - - sector_size_ = get_sector_size(path); - if (sector_size_ == 0) { - throw std::runtime_error("Failed to get sector size"); - } - - handle_ = CreateFileA(path.data(), - GENERIC_WRITE, - 0, // No sharing - nullptr, - OPEN_ALWAYS, - FILE_FLAG_OVERLAPPED | FILE_FLAG_NO_BUFFERING | - FILE_FLAG_SEQUENTIAL_SCAN, - nullptr); - if (handle_ == INVALID_HANDLE_VALUE) { - auto err = get_last_error_as_string(); - throw std::runtime_error("Failed to open file '" + std::string(path) + - "': " + err); - } -#else - page_size_ = sysconf(_SC_PAGESIZE); - fd_ = open(path.data(), O_WRONLY | O_CREAT, 0644); - if (fd_ < 0) { - throw std::runtime_error("Failed to open file: " + std::string(path)); - } -#endif -} - -zarr::VectorizedFileWriter::~VectorizedFileWriter() -{ -#ifdef _WIN32 - if (handle_ != INVALID_HANDLE_VALUE) { - CloseHandle(handle_); - } -#else - if (fd_ >= 0) { - close(fd_); - } -#endif -} - -bool -zarr::VectorizedFileWriter::write_vectors( - const std::vector>& buffers, - size_t offset) -{ - std::lock_guard lock(mutex_); - bool retval{ true }; - -#ifdef _WIN32 - size_t total_bytes_to_write = 0; - for (const auto& buffer : buffers) { - total_bytes_to_write += buffer.size(); - } - - const size_t nbytes_aligned = align_size_(total_bytes_to_write); - CHECK(nbytes_aligned >= total_bytes_to_write); - - auto* aligned_ptr = - static_cast(_aligned_malloc(nbytes_aligned, page_size_)); - if (!aligned_ptr) { - return false; - } - - auto* cur = aligned_ptr; - for (const auto& buffer : buffers) { - std::copy(buffer.begin(), buffer.end(), cur); - cur += buffer.size(); - } - - std::vector segments(nbytes_aligned / page_size_); - - cur = aligned_ptr; - for (auto& segment : segments) { - memset(&segment, 0, sizeof(segment)); - segment.Buffer = PtrToPtr64(cur); - cur += page_size_; - } - - OVERLAPPED overlapped = { 0 }; - overlapped.Offset = static_cast(offset & 0xFFFFFFFF); - overlapped.OffsetHigh = static_cast(offset >> 32); - overlapped.hEvent = CreateEvent(nullptr, TRUE, FALSE, nullptr); - - DWORD bytes_written; - - if (!WriteFileGather( - handle_, segments.data(), nbytes_aligned, nullptr, &overlapped)) { - if (GetLastError() != ERROR_IO_PENDING) { - LOG_ERROR("Failed to write file: ", get_last_error_as_string()); - retval = false; - } - - // Wait for the operation to complete - if (!GetOverlappedResult(handle_, &overlapped, &bytes_written, TRUE)) { - LOG_ERROR("Failed to get overlapped result: ", - get_last_error_as_string()); - retval = false; - } - } - - _aligned_free(aligned_ptr); -#else - std::vector iovecs(buffers.size()); - - for (auto i = 0; i < buffers.size(); ++i) { - auto* iov = &iovecs[i]; - memset(iov, 0, sizeof(struct iovec)); - iov->iov_base = - const_cast(static_cast(buffers[i].data())); - iov->iov_len = buffers[i].size(); - } - - ssize_t total_bytes = 0; - for (const auto& buffer : buffers) { - total_bytes += static_cast(buffer.size()); - } - - ssize_t bytes_written = pwritev(fd_, - iovecs.data(), - static_cast(iovecs.size()), - static_cast(offset)); - - if (bytes_written != total_bytes) { - auto error = get_last_error_as_string(); - LOG_ERROR("Failed to write file: ", error); - retval = false; - } -#endif - return retval; -} - -size_t -zarr::VectorizedFileWriter::align_size_(size_t size) const -{ - size = align_to_page_(size); -#ifdef _WIN32 - return (size + sector_size_ - 1) & ~(sector_size_ - 1); -#else - return size; -#endif -} - -size_t -zarr::VectorizedFileWriter::align_to_page_(size_t size) const -{ - return (size + page_size_ - 1) & ~(page_size_ - 1); -} \ No newline at end of file diff --git a/src/streaming/vectorized.file.writer.hh b/src/streaming/vectorized.file.writer.hh deleted file mode 100644 index 0e15c8d..0000000 --- a/src/streaming/vectorized.file.writer.hh +++ /dev/null @@ -1,44 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include - -#ifdef _WIN32 -#include -#undef min -#undef max -#else -#include -#include -#include -#endif - -namespace zarr { -class VectorizedFileWriter -{ - public: - explicit VectorizedFileWriter(std::string_view path); - ~VectorizedFileWriter(); - - bool write_vectors(const std::vector>& buffers, - size_t offset); - - std::mutex& mutex() { return mutex_; } - - private: - std::mutex mutex_; - size_t page_size_; -#ifdef _WIN32 - HANDLE handle_; - size_t sector_size_; -#else - int fd_; -#endif - - size_t align_size_(size_t size) const; - size_t align_to_page_(size_t size) const; -}; -} // namespace zarr \ No newline at end of file diff --git a/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp index 2bc7e4a..897740b 100644 --- a/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp +++ b/tests/unit-tests/shard-writer-add-chunk-data-to-flush.cpp @@ -9,10 +9,13 @@ namespace fs = std::filesystem; +static constexpr size_t chunk_size = 967; +static constexpr uint32_t chunks_before_flush = 2; +static constexpr uint32_t chunks_per_shard = 8; + void -verify_file_data(const std::string& filename) +verify_file_data(const std::string& filename, size_t file_size) { - const size_t file_size = 1024 * 8; std::ifstream file(filename, std::ios::binary); std::vector read_buffer(file_size); @@ -21,9 +24,13 @@ verify_file_data(const std::string& filename) int values[] = { 0, 7, 3, 1, 5, 6, 2, 4 }; - size_t offset = 0; - for (size_t i = 0; i < 8; ++i) { - for (size_t j = offset; j < offset + 1024; ++j) { + size_t buf_offset = 0; + for (auto i = 0; i < 8; ++i) { + if (i % chunks_before_flush == 0) { + buf_offset = zarr::align_to_system_boundary(buf_offset); + } + + for (size_t j = buf_offset; j < buf_offset + chunk_size; ++j) { auto byte = (int)read_buffer[j]; EXPECT(byte == values[i], "Data mismatch at offset ", @@ -34,46 +41,121 @@ verify_file_data(const std::string& filename) byte, "."); } - offset += 1024; + buf_offset += chunk_size; } // check the index table - std::vector index_table(16); - file.read(reinterpret_cast(index_table.data()), - 2 * 8 * sizeof(uint64_t)); - CHECK(file.good() && file.gcount() == 2 * 8 * sizeof(uint64_t)); + const auto index_table_size = chunks_per_shard * 2 * sizeof(uint64_t); + CHECK(buf_offset == file_size - index_table_size); + auto* index_table = + reinterpret_cast(read_buffer.data() + buf_offset); - // chunk 0 - EXPECT(index_table[0] == 0, "Expected 0, got ", index_table[0]); - EXPECT(index_table[1] == 1024, "Expected 1024, got ", index_table[1]); + size_t file_offset = 0; - // chunk 1 - EXPECT(index_table[2] == 3 * 1024, "Expected 3072, got ", index_table[2]); - EXPECT(index_table[3] == 1024, "Expected 1024, got ", index_table[3]); + // chunk 0 + EXPECT(index_table[0] == file_offset, + "Expected ", + file_offset, + ", got ", + index_table[0]); + EXPECT(index_table[1] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[1]); - // chunk 2 - EXPECT(index_table[4] == 6 * 1024, "Expected 6144, got ", index_table[4]); - EXPECT(index_table[5] == 1024, "Expected 1024, got ", index_table[5]); + // chunk 7 + EXPECT(index_table[14] == file_offset + chunk_size, + "Expected ", + file_offset + chunk_size, + ", got ", + index_table[14]); + EXPECT(index_table[15] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[15]); + + file_offset = + zarr::align_to_system_boundary(chunks_before_flush * chunk_size); // chunk 3 - EXPECT(index_table[6] == 2 * 1024, "Expected 2048, got ", index_table[6]); - EXPECT(index_table[7] == 1024, "Expected 1024, got ", index_table[7]); + EXPECT(index_table[6] == file_offset, + "Expected ", + file_offset, + " got ", + index_table[6]); + EXPECT(index_table[7] == chunk_size, + "Expected ", + chunk_size, + ", got ", + index_table[7]); - // chunk 4 - EXPECT(index_table[8] == 7 * 1024, "Expected 7168, got ", index_table[8]); - EXPECT(index_table[9] == 1024, "Expected 1024, got ", index_table[9]); + // chunk 1 + EXPECT(index_table[2] == file_offset + chunk_size, + "Expected ", + file_offset + chunk_size, + ", got ", + index_table[2]); + EXPECT(index_table[3] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[3]); + + file_offset += + zarr::align_to_system_boundary(chunks_before_flush * chunk_size); // chunk 5 - EXPECT(index_table[10] == 4 * 1024, "Expected 4096, got ", index_table[10]); - EXPECT(index_table[11] == 1024, "Expected 1024, got ", index_table[11]); + EXPECT(index_table[10] == file_offset, + "Expected ", + file_offset, + ", got ", + index_table[10]); + EXPECT(index_table[11] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[11]); // chunk 6 - EXPECT(index_table[12] == 5 * 1024, "Expected 5120, got ", index_table[12]); - EXPECT(index_table[13] == 1024, "Expected 1024, got ", index_table[13]); + EXPECT(index_table[12] == file_offset + chunk_size, + "Expected ", + file_offset + chunk_size, + ", got ", + index_table[12]); + EXPECT(index_table[13] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[13]); + + file_offset += + zarr::align_to_system_boundary(chunks_before_flush * chunk_size); - // chunk 7 - EXPECT(index_table[14] == 1 * 1024, "Expected 1024, got ", index_table[14]); - EXPECT(index_table[15] == 1024, "Expected 1024, got ", index_table[15]); + // chunk 2 + EXPECT(index_table[4] == file_offset, + "Expected ", + file_offset, + ", got ", + index_table[4]); + EXPECT(index_table[5] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[5]); + + // chunk 4 + EXPECT(index_table[8] == file_offset + chunk_size, + "Expected ", + file_offset + chunk_size, + ", got ", + index_table[8]); + EXPECT(index_table[9] == chunk_size, + "Expected ", + chunk_size, + " got ", + index_table[9]); } int @@ -81,9 +163,6 @@ main() { int retval = 1; - const uint32_t chunks_before_flush = 2; - const uint32_t chunks_per_shard = 8; - auto shard_file_path = fs::temp_directory_path() / "shard-data.bin"; zarr::ShardWriterConfig config = { .file_path = shard_file_path.string(), .chunks_before_flush = @@ -95,7 +174,7 @@ main() std::vector> chunk_data(chunks_per_shard); for (auto i = 0; i < chunks_per_shard; ++i) { - chunk_data[i].resize(1024); + chunk_data[i].resize(chunk_size); std::fill(chunk_data[i].begin(), chunk_data[i].end(), std::byte(i)); } @@ -110,7 +189,11 @@ main() writer->add_chunk(&chunk_data[4], 4); zarr::finalize_shard_writer(std::move(writer)); - auto expected_file_size = 1024 * 8 + index_table_size; + auto expected_file_size = + 3 * zarr::align_to_system_boundary( + chunks_before_flush * chunk_size) + // 3 aligned sets of chunks + chunks_before_flush * chunk_size + // final, unaligned set of chunks + index_table_size; // index table auto actual_file_size = fs::file_size(shard_file_path); EXPECT(actual_file_size == expected_file_size, "Expected a file size of ", @@ -118,7 +201,7 @@ main() ", got ", actual_file_size); - verify_file_data(shard_file_path.string()); + verify_file_data(shard_file_path.string(), actual_file_size); retval = 0; } catch (const std::exception& exc) { diff --git a/tests/unit-tests/vectorized-file-write.cpp b/tests/unit-tests/vectorized-file-write.cpp index 79ebe6d..20066d5 100644 --- a/tests/unit-tests/vectorized-file-write.cpp +++ b/tests/unit-tests/vectorized-file-write.cpp @@ -1,4 +1,4 @@ -#include "vectorized.file.writer.hh" +#include "platform.hh" #include "unit.test.macros.hh" #include @@ -11,18 +11,19 @@ size_t write_to_file(const std::string& filename) { size_t file_size = 0; - zarr::VectorizedFileWriter writer(filename); + zarr::VectorizedFile file(filename); std::vector> data(10); std::vector> spans(10); for (auto i = 0; i < data.size(); ++i) { - data[i].resize((i + 1) * 1024); + data[i].resize((i + 1) * 967); std::fill(data[i].begin(), data[i].end(), std::byte(i)); file_size += data[i].size(); spans[i] = data[i]; } - CHECK(writer.write_vectors(spans, 0)); + size_t offset = 0; + CHECK(file_write_vectorized(file, spans, offset)); // write more data for (auto i = 0; i < 10; ++i) { @@ -30,9 +31,10 @@ write_to_file(const std::string& filename) std::fill(vec.begin(), vec.end(), std::byte(i + 10)); spans[i] = vec; } - CHECK(writer.write_vectors(spans, file_size)); + offset = zarr::align_to_system_boundary(file_size); + CHECK(file_write_vectorized(file, spans, offset)); - return 2 * file_size; + return offset + file_size; } void @@ -47,7 +49,7 @@ verify_file_data(const std::string& filename, size_t file_size) // Verify data pattern size_t offset = 0; for (size_t i = 0; i < 10; ++i) { - size_t size = (i + 1) * 1024; + size_t size = (i + 1) * 967; for (size_t j = offset; j < offset + size; ++j) { auto byte = (int)read_buffer[j]; @@ -63,8 +65,9 @@ verify_file_data(const std::string& filename, size_t file_size) offset += size; } + offset = zarr::align_to_system_boundary(offset); for (size_t i = 0; i < 10; ++i) { - size_t size = (i + 1) * 1024; + size_t size = (i + 1) * 967; for (size_t j = offset; j < offset + size; ++j) { auto byte = (int)read_buffer[j]; @@ -98,7 +101,7 @@ main() EXPECT(fs::exists(filename), "File not found: ", filename); auto file_size_on_disk = fs::file_size(filename); - EXPECT(file_size_on_disk >= file_size, // sum(1:10) * 1024 * 2 + EXPECT(file_size_on_disk >= file_size, "Expected file size of at least ", file_size, " bytes, got ", From 99a553fdaf9123ebd476b17de0516e54ea05c26f Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Fri, 13 Dec 2024 14:19:13 -0500 Subject: [PATCH 25/26] Fix ragged internal dim size check on OSX. --- tests/unit-tests/unit.test.macros.hh | 7 +++++++ .../zarrv3-writer-write-ragged-internal-dim.cpp | 15 ++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/unit-tests/unit.test.macros.hh b/tests/unit-tests/unit.test.macros.hh index 302a033..dcb61b7 100644 --- a/tests/unit-tests/unit.test.macros.hh +++ b/tests/unit-tests/unit.test.macros.hh @@ -21,6 +21,13 @@ a_ == b_, "Expected ", #a, " == ", #b, " but ", a_, " != ", b_); \ } while (0) +#define EXPECT_GTE(T, a, b) \ + do { \ + T a_ = (T)(a); \ + T b_ = (T)(b); \ + EXPECT(a_ >= b_, "Expected ", #a, " < ", #b, " but ", a_, " >= ", b_); \ + } while (0) + #define EXPECT_STR_EQ(a, b) \ do { \ std::string a_ = (a) ? (a) : ""; \ diff --git a/tests/unit-tests/zarrv3-writer-write-ragged-internal-dim.cpp b/tests/unit-tests/zarrv3-writer-write-ragged-internal-dim.cpp index 0affa4e..a9690b2 100644 --- a/tests/unit-tests/zarrv3-writer-write-ragged-internal-dim.cpp +++ b/tests/unit-tests/zarrv3-writer-write-ragged-internal-dim.cpp @@ -117,7 +117,8 @@ main() "x", ZarrDimensionType_Space, array_width, chunk_width, shard_width); zarr::ArrayWriterConfig config = { - .dimensions = std::make_shared(std::move(dims), dtype), + .dimensions = + std::make_shared(std::move(dims), dtype), .dtype = dtype, .level_of_detail = 5, .bucket_name = std::nullopt, @@ -130,7 +131,8 @@ main() std::move(config), thread_pool); const size_t frame_size = array_width * array_height * nbytes_px; - std::vector data(frame_size, std::byte(0));; + std::vector data(frame_size, std::byte(0)); + ; for (auto i = 0; i < n_frames; ++i) { // 2 time points CHECK(writer->write_frame(data)); @@ -144,10 +146,9 @@ main() const auto index_size = chunks_per_shard * sizeof(uint64_t) * // indices are 64 bits 2; // 2 indices per chunk - const auto expected_file_size = shard_width * shard_height * - shard_planes * shard_timepoints * - chunk_size + - index_size; + const auto min_file_size = shard_width * shard_height * shard_planes * + shard_timepoints * chunk_size + + index_size; const fs::path data_root = base_dir / "data/root" / std::to_string(config.level_of_detail); @@ -168,7 +169,7 @@ main() const auto x_file = y_dir / std::to_string(x); CHECK(fs::is_regular_file(x_file)); const auto file_size = fs::file_size(x_file); - EXPECT_EQ(int, file_size, expected_file_size); + EXPECT_GTE(int, file_size, min_file_size); } CHECK(!fs::is_regular_file(y_dir / From b6fd05d46a4645d1968fdc4d4345cff0ab8cd30d Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 18 Dec 2024 12:47:34 -0500 Subject: [PATCH 26/26] Respond to PR comments. --- src/streaming/array.writer.cpp | 2 +- src/streaming/zarrv2.array.writer.cpp | 30 ++++++++++----------------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/src/streaming/array.writer.cpp b/src/streaming/array.writer.cpp index 8c85e6a..4c06183 100644 --- a/src/streaming/array.writer.cpp +++ b/src/streaming/array.writer.cpp @@ -151,7 +151,7 @@ zarr::ArrayWriter::make_metadata_sink_() metadata_sink_ = creator.make_s3_sink(*config_.bucket_name, metadata_path); } else { - metadata_sink_ = zarr::SinkCreator::make_file_sink(metadata_path); + metadata_sink_ = SinkCreator::make_file_sink(metadata_path); } if (!metadata_sink_) { diff --git a/src/streaming/zarrv2.array.writer.cpp b/src/streaming/zarrv2.array.writer.cpp index 5c40840..9d4d6e4 100644 --- a/src/streaming/zarrv2.array.writer.cpp +++ b/src/streaming/zarrv2.array.writer.cpp @@ -139,27 +139,19 @@ zarr::ZarrV2ArrayWriter::compress_and_flush_() return false; } - auto queued = thread_pool_->push_job(std::move( - [&sink = data_sinks_[i], buf = &chunk_buffers_[i], &latch]( - std::string& err) -> bool { - bool success = false; - - try { - success = sink->write(0, *buf); - } catch (const std::exception& exc) { - err = "Failed to write chunk: " + std::string(exc.what()); - } - - latch.count_down(); - return success; - })); - - if (!queued) { - err = "Failed to push job to thread pool"; - latch.count_down(); + auto& sink = data_sinks_[i]; + auto& buf = chunk_buffers_[i]; + + bool success = false; + + try { + success = sink->write(0, buf); + } catch (const std::exception& exc) { + err = "Failed to write chunk: " + std::string(exc.what()); } - return queued; + latch.count_down(); + return success; }; CHECK(thread_pool_->push_job(std::move(job)));