Repository navigation
Hold attribute files open across tile reads and read compressed tiles with pread - #9
Conversation
With the mmap read method, every tile read opened and closed its attribute file; open() was about 15% of a GATK GnarlyGenotyper profile over a 1,000-sample GenomicsDB workspace. ReadState now keeps each attribute and variable-sized attribute file open until it is destroyed, within a process-wide budget: TILEDB_MAX_CACHED_READ_FILE_HANDLES if set, otherwise half the soft RLIMIT_NOFILE. Past the budget, files are opened for each tile as before. Compressed tiles are read with pread instead of mmap. They are always decompressed into a separate buffer, so mapping them saved no copy but cost an mmap/munmap pair and the page faults of a fresh mapping per tile. They are now read from the held descriptor into the reused tile_compressed_ buffer. Uncompressed tiles are still mapped. Adds tests that reads return every cell whether or not files are held open, that held files are closed when the array is finalized, and that no more are held than the budget allows.
| * | ||
| * The MIT License | ||
| * | ||
| * @copyright Copyright (c) 2026 Tim Fennell |
There was a problem hiding this comment.
Not sure what we want the copyright to be. Should it be Copyright (c) 2026 Broad Institute of MIT and Harvard?
There was a problem hiding this comment.
I'd leave it as is, the same as test_invalid_handles.cc and test_error_messages.cc: as things stand I can only put myself or Fulcrum Genomics as the copyright holder for content I write, and since the code is MIT licensed it makes little practical difference who holds it. If you'd like the Broad to hold it, I'm happy to assign ownership to the Broad if it has a process for that.
| * Bounded so that workspaces with many fragments cannot exhaust the process | ||
| * descriptor limit; see open_attribute_file(). | ||
| */ | ||
| static std::atomic<int64_t> num_held_file_descriptors(0); |
There was a problem hiding this comment.
This is process-wide counter. Would this be a contention if workloads spawn many concurrent array reads, possibly from different threads? We should document this at least.
I also don't see test coverage for concurrency, maybe that will help settle this issue.
There was a problem hiding this comment.
I've added cd324e4, which documents the limit as shared by reads on every thread, in read_state.cc and EnvironmentVariables.md, and adds two tests in which 8 threads, each with its own context, read the array at once: every reader gets every cell, and between them they hold exactly TILEDB_MAX_CACHED_READ_FILE_HANDLES files.
On contention: the counter changes only when a read opens or closes an attribute file, never when it reads a tile from a file it already holds, and each change sits beside the open() or close() it accounts for. To measure it, I benchmarked threads that each repeatedly open, read and finalize a sparse array of 200 small fragments, so that every read opens 1,000 attribute files to read 2,000 tiles, the most counter updates for the work done (benchmark code). Reads per second, median of 5 runs, on an M4 Max under macOS, and under Linux aarch64 in a Docker VM on the same machine (16 vCPUs, GCC 15, workspace on tmpfs):
| Threads | Linux main | Linux #9 | macOS main | macOS #9 |
|---|---|---|---|---|
| 1 | 80.5 | 122.7 | 18.4 | 26.2 |
| 2 | 136.5 | 219.1 | 26.7 | 38.7 |
| 4 | 188.0 | 403.5 | 40.2 | 57.0 |
| 8 | 150.1 | 496.5 | 25.3 | 35.4 |
On Linux, #9 scales to 4.1x at 8 threads where main peaks at 2.3x and falls back to 1.9x. On macOS both scale the same, and poorly, because open() itself doesn't scale there: profiling #9 at 8 threads, 82% of samples are in open(), mostly from the file size lookup in ReadState's constructor, which main has too, and the code that updates the counter doesn't register.
With the default mmap read method, every tile read opens and closes its attribute file, and
open()was about 15% of a GATK GnarlyGenotyper profile over a 1,000-sample GenomicsDB workspace.ReadStatenow keeps each attribute file (and variable-sized attribute file) it reads open until it is destroyed, within a process-wide budget:TILEDB_MAX_CACHED_READ_FILE_HANDLESif set, otherwise half the softRLIMIT_NOFILE, beyond which files are opened per tile as before. This is separate fromTILEDB_KEEP_FILE_HANDLES_OPEN, which only covers files being written, and only applies to local files read with mmap, since cloud and HDFS reads don't take this path.Compressed tiles are now read with
preadfrom the held descriptor into a reused buffer instead of being mapped. They are always decompressed into a separate buffer, so mapping them saved no copy but cost anmmap/munmappair and fresh page faults per tile. Uncompressed tiles are still mapped.Measured one after the other on GnarlyGenotyper, 1,000 samples, chr20:60,001-16,000,000, LZ4-compressed workspace: holding files open took the runtime from 1,222 s to 911 s (−25%), then
preadfrom 1,001 s to 889 s (−11%, paired runs). New tests check that reads return the same cells whether or not files are held open, that held files are closed when the array is finalized, and that no more are held than the budget allows.TILEDB_MAX_CACHED_READ_FILE_HANDLESis documented incore/EnvironmentVariables.md.