Repository navigation
[format][python] Compress video index pages and reduce seek I/O - #10905
XiaoHongbo-Hope wants to merge 8 commits into
Conversation
|
Thanks for the PR! The page directory looks like the right direction to me, and from my local tests most of the improvement comes from it. But I have some concerns about the format design, mainly the versioning and the compression part. 1. The v1 layout changes again without bumping the version This is the third v1 layout within one month (#9831: On the read path I know none of these commits is released yet, but people building from master may already have such data. Since we already have a version byte, I think we should just use version 2 here (it costs nothing), and let readers report "unsupported keyframe index version" instead of misparsing. Maybe also reserve a flags or header-length field, so next time we don't need to break the layout again. 2. Most of the gain comes from the directory, not compression I counted GETs with an instrumented
Compression reduces the bytes by ~3x, but the GET count is the same. On OSS the latency is mostly decided by the number of requests; with a simple model (15 ms per GET + 100 MB/s, my assumption, not measured) it's about 7.8 s vs 7.4 s. For storage, the index is 24 bytes per keyframe while one keyframe usually covers tens of KB of video, so the saving is very small. Also, once a codec is in the spec it's hard to change, and I'm not sure zlib over row-major int64 triples is a good choice. On the same data (GOP 2):
So my suggestion is to split this PR: land the directory with codec 0 first, and discuss compression separately. If we keep compression, maybe define the codec as per-column delta encoding. It would also be great if the benchmark could show the directory and compression separately. 3. Unsupported version / codec cannot be distinguished from corruption If we add codec 2 later, old readers will fail with
4. Directory caches can be simpler (minor) Python keeps three caches: directory blocks (512 records, LRU of 2), descriptors (LRU of 512) and pages. The directory for 1M keyframes is only 16.6 KB and is usually already in the 64 KB prefix. If we read the whole directory at open, the binary search needs no I/O and the first two caches can be removed. I also left a few smaller comments inline (page size wording in the spec, the eager-loading threshold, and the temp file in the Java validator). WDYT? |
| Each page holds 4,096 uncompressed `(frame ordinal, PTS, packet position)` int64 triples | ||
| and their 4-byte CRC32; the last page may be shorter. Page `p` starts at | ||
| `21 + 16 * metadata_count + p * 98308`. | ||
| Each page holds up to 4,096 `(frame ordinal, PTS, packet position)` int64 triples, |
There was a problem hiding this comment.
This says "up to 4,096", but both readers assume every page except the last has exactly 4,096 entries: they compute the entry count as min(4096, count - p * 4096) and locate the page by divmod(index, 4096). So a writer which follows the spec and writes variable-size pages (e.g. a future Rust writer) will be rejected.
Maybe say "exactly 4,096 except the last page" here, or store page_entries in the header. The directory already has the first ordinal of each page, so variable-size pages are not hard to support.
| prefix = read_at(0, min(length, cls.HEADER.size + _PagedKeyframes.PAGE_BYTES + 4)) | ||
| if len(prefix) != min(length, cls.HEADER.size + _PagedKeyframes.PAGE_BYTES + 4): | ||
| # Small indexes use one request; larger indexes retain only a bounded prefix. | ||
| prefix_size = length if length <= _IndexedKeyframes.EAGER_BYTES else 64 * 1024 |
There was a problem hiding this comment.
Here the whole index is read when the encoded length <= 2 MiB, but load_small() only goes eager when the decoded size count * 24 <= 2 MiB. For compressed indexes in between, the whole encoded index stays in prefix, and pages are still decoded into the 1 MiB page cache. With 200K keyframes I see 1.58 MB held + 0.97 MB page cache, and it's not eager.
So the memory per open index goes from ~1.1 MB to ~2.5 MB, multiplied by max_open_videos (16 by default) and the number of DataLoader workers. Maybe use one threshold on the decoded size, or document this case.
| crc.update(page, 0, bytes); | ||
| if (crc.getValue() != expected) { | ||
| throw invalid("page checksum"); | ||
| Path path = Files.createTempFile("paimon-video-directory-", ".tmp"); |
There was a problem hiding this comment.
This creates a temp file for every index only to read the directory twice. The caller's spool in VideoFormatWriter is already a RandomAccessFile, so maybe we can pass a seekable input here, or just keep the directory in memory. It is bounded by the index length, and only about 16.6 KB for 1M keyframes.
XiaoHongbo-Hope
changed the title
XiaoHongbo-Hope
changed the title
Purpose
Reduce video-index I/O with a page directory and optional compression.
video.index.compressiondefaults todelta-zlib;nonedisables compression. Readers auto-detect encoding; rewrites preserve it.Measurements
Three-run medians. Local index-only test: 1M keyframes; writes include fsync, reads include open + 64 random lookups.
Compression saves I/O at a CPU cost. In three local DROID DataLoader runs (Python/PyAV, batch 32, 4 workers, 10 warmup + 64 measured batches), median throughput was 97.75 → 97.24 samples/s and CPU 5.76 → 5.75 cores; indexes shrank 90%. These indexes fit in cache; this does not measure large-index cache churn.
Separately, the directory-only OSS test reduced 64-lookup latency from 10.21 s to 4.02 s and GETs from 163 to 65.
Validation
Python: 190 passed, 1 skipped, 147 subtests; 6 additional decoder tests passed. Seven Ray tests excluded due to an incomplete local installation. Java: 26 passed.
Coverage includes cross-language fixtures, real video reads, mixed codecs, corruption, memory bounds, rolling and compaction. RAT, Maven checks, flake8, Python 3.6 syntax and diff checks passed locally.