Make concurrent PackData reads thread-safe - #2356
Conversation
|
@jelmer The pack _offset_cache is still unsynchronized. I couldn't get that fix to pass tests yet, so I've excluded it from the PR. I can add it back if you think that the PR shouldn't land without that. |
Give each PackData reader its own buffered cursor. Use pread for files PackData opens, and serialize seek-based reads for supplied file objects and systems without pread. Coordinate reads and close so active reads finish safely, concurrent closes do not race, and cursors fail once the pack is closed. Record the opened file's size for checksum reads and cover concurrency and cursor edge cases. Fixes jelmer#2350
Calculate checksums through cursors so reads no longer monopolize the shared file position. Path-backed packs use pread; the seek-based fallback locks each seek/read operation individually, allowing other readers between chunks. Derive checksum offsets from the opened descriptor and reject packs with missing or truncated checksums.
There was a problem hiding this comment.
The whole point of pread is to avoid cursors, so adding a cursor wrapper for pread seems counterintuitive. This also adds a bunch of overhead in a performance-critical part of the code as well as a lot of complexity. In some quick and dirty benchmarking, I see a 10-20% performance degradation from this PR.
Maybe we can reevaluate using mmap for packs instead?
|
I am not thrilled by the amount of complexity either. FWIW the current work is about 20 commits squashed together, the first one was nice and simple, but getting the semantics right (e.g. close() races) took both code and effort. That said, having a cursor abstraction on top of pread feels unavoidable, unpack_object() expects a stateful read(size) callback. Though I might be running into my limits of understanding the current architecture. A 10-20% performance degradation is completely unacceptable, but I'd like to see if there's anything I can do to optimize it before jumping into an alternative approach with mmap, so that at least we have a fair comparison. Can you share your quick&dirty benchmark? |
Pack scans ask for 64KB chunks. With a fixed 512-byte readahead, almost every object triggered another pread and another lock acquisition. Scanning an 18MB test pack made about 34,000 pread calls and was roughly 30% slower than BufferedReader. Double the readahead size after each buffer miss, up to 256KB. Short random reads still start at 512 bytes, while sequential scans quickly start reading larger chunks. In the same benchmark, the slowdown drops to about 4%. Random reads and checksum performance are unchanged.
|
I've pushed an optimization. Will focus on mmap in the next two days, then we can compare. |
If we're going to use a cursor, then we might as well just use file descriptors - perhaps just one per thread? That said, I think mmap /or/ direct use of os.pread (rather than a cursor!) are going to keep the code much simpler.
It was a quick one-off that I didn't save unfortunately. We do also have https://github.com/dulwich/benchmarks but I haven't run that in a while. |
2 participants
Use os.pread where available so concurrent pack readers no longer share a file position. Provide synchronized fallback reads for supplied file objects and systems without pread, coordinate reads with close operations, and protect the pack offset cache.
Add targeted coverage for concurrent reads, cursor behavior, checksum reads, and close races.
Issue: #2350