summaryrefslogtreecommitdiff
path: root/reftable/block.c
AgeCommit message (Collapse)AuthorFilesLines
2026-08-24Merge branch 'js/coverity-unchecked-returns-fix'Junio C Hamano1-1/+4
A handful of code paths have been corrected to check return values from functions like curl_easy_duphandle(), deflateInit(), lseek(), dup(), and strbuf_getline_lf(), resolving several Coverity warnings about unchecked returns. * js/coverity-unchecked-returns-fix: bisect: handle dup() failure when redirecting stdout bisect: check get_terms return at all call sites bisect: check strbuf_getline_lf return when reading terms transport-helper: warn when export-marks file cannot be finalized transport-helper: check dup() return in get_exporter compat/pread: check initial lseek for errors last-modified: handle repo_parse_commit() failures reftable tests: check reftable_table_init_ref_iterator() return reftable/block: check deflateInit() return value reftable: handle block-writer initialization errors config: propagate launch_editor() failure in show_editor() http: die on curl_easy_duphandle failure in get_active_slot
2026-08-12reftable/block: check deflateInit() return valueJohannes Schindelin1-1/+4
block_writer_init() allocates a z_stream and calls deflateInit() to prepare it for compressing log records. The return value of deflateInit() is silently discarded. If zlib initialization fails (e.g., Z_MEM_ERROR when the system is under memory pressure), the z_stream is left in an undefined state. Subsequent deflate() calls in block_writer_finish() then operate on this uninitialized stream. Current zlib/zlib-ng versions handle such a stream gracefully, by returning `Z_STREAM_ERROR`, so in practice it would likely not result in catastrophic error. The function already uses REFTABLE_ZLIB_ERROR for deflate() failures later in the code path, so returning the same error code for deflateInit() failure is consistent. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-07-03reftable/block: fix OOB read with bogus restart offsetPatrick Steinhardt1-0/+9
Restart points encode records in a given block that do not use prefix compression and that can thus immediately be seeked to. These offsets are encoded in the restart table, where each offset needs to point at one of the records of the block. We do not verify this though, so a bogus restart offset may cause an out-of-bounds read: ==1472280==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de5f7f (pc 0x55555599502b bp 0x7fffffff4df0 sp 0x7fffffff4d40 T0) ==1472280==The signal is caused by a READ memory access. #0 0x55555599502b in get_var_int ./git/build/../reftable/record.c:30:6 #1 0x555555995c2a in reftable_decode_keylen ./git/build/../reftable/record.c:177:6 #2 0x55555598e85c in restart_needle_less ./git/build/../reftable/block.c:455:6 #3 0x55555598895f in binsearch ./git/build/../reftable/basics.c:175:9 #4 0x55555598e189 in block_iter_seek_key ./git/build/../reftable/block.c:543:6 #5 0x555555814aee in test_reftable_block__corrupt_restart_offset ./git/build/../t/unit-tests/u-reftable-block.c:636:20 #6 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3 #7 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3 #8 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4 #9 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11 #10 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8 #11 0x55555584c25a in main ./git/build/../common-main.c:9:11 #12 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #13 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #14 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24) ==1472280==Register values: rax = 0x00007d8ff7de5f7f rbx = 0x00007fffffff4e00 rcx = 0x00007d8ff7de5f80 rdx = 0x00007bfff5b6af60 rdi = 0x00007bfff5b6af40 rsi = 0x00007bfff592dfa0 rbp = 0x00007fffffff4df0 rsp = 0x00007fffffff4d40 r8 = 0x00000000ff00002b r9 = 0x00007d8ff7de5f7f r10 = 0x00000f7ffeb25bf0 r11 = 0xf3f30000f1f1f1f1 r12 = 0x00007fffffff58f8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556055fd0 AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/record.c:30:6 in get_var_int Guard against such restart offsets and signal an error to the caller via `args.error`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-07-03reftable/block: fix use of uninitialized memory when binsearch failsPatrick Steinhardt1-4/+4
When doing the binary search through our restart offsets we may hit an error in case `restart_needle_less()` fails to decode the record at the given offset. While we correctly detect this case and error out, it will cause us to call `reftable_record_release()` on the yet-uninitialized record. Fix this by initializing the record earlier. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-07-03reftable/block: fix OOB read with bogus restart countPatrick Steinhardt1-0/+4
The restart count is stored in the last two bytes of a block. We use it without verification to compute the offset of the restart table. With a bogus restart count that is large enough this computation underflows, and the subsequent reads via the restart table access out-of-bounds memory: ==129439==ERROR: AddressSanitizer: SEGV on unknown address 0x7d90f6dcd0ad (pc 0x55555598ce89 bp 0x7fffffff4ed0 sp 0x7fffffff4e80 T0) ==129439==The signal is caused by a READ memory access. #0 0x55555598ce89 in reftable_get_be24 ./git/build/../reftable/basics.h:125:9 #1 0x55555598eabf in block_restart_offset ./git/build/../reftable/block.c:407:9 #2 0x55555598e5d5 in restart_needle_less ./git/build/../reftable/block.c:431:17 #3 0x5555559887e2 in binsearch ./git/build/../reftable/basics.c:165:13 #4 0x55555598dfec in block_iter_seek_key ./git/build/../reftable/block.c:529:6 #5 0x555555814517 in test_reftable_block__corrupt_restart_count ./git/build/../t/unit-tests/u-reftable-block.c:593:15 #6 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3 #7 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3 #8 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4 #9 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11 #10 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8 #11 0x55555584c12a in main ./git/build/../common-main.c:9:11 #12 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #13 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #14 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24) ==129439==Register values: rax = 0x00007d90f6dcd0ad rbx = 0x00007fffffff4f20 rcx = 0xf2f2f2f8f2f2f2f8 rdx = 0x0000000000000000 rdi = 0x00007d90f6dcd0ad rsi = 0x0000000000007fff rbp = 0x00007fffffff4ed0 rsp = 0x00007fffffff4e80 r8 = 0x0000000000000000 r9 = 0x0000000000000000 r10 = 0x0000000000000000 r11 = 0x0000000000000017 r12 = 0x00007fffffff58e8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x00005555560550b0 AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/basics.h:125:9 in reftable_get_be24 Verify that the restart table actually fits into the block. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-07-03reftable/block: fix OOB read with bogus block sizePatrick Steinhardt1-0/+9
The block size is read from the block header, which is untrusted data. We use it without verification to access the restart count at the end of the block as well as to compute the restart table offset. With a bogus block size that exceeds the data we have actually read this can lead to an out-of-bounds read: ==2274138==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c3ff6de2e3f at pc 0x55555598c6ea bp 0x7fffffff4ee0 sp 0x7fffffff4ed8 READ of size 1 at 0x7c3ff6de2e3f thread T0 #0 0x55555598c6e9 in reftable_get_be16 /home/pks/Development/git/build/../reftable/basics.h:119:20 #1 0x55555598c252 in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:343:18 #2 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20 #3 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3 #4 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3 #5 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4 #6 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11 #7 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8 #8 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11 #9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d) #10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d) #11 0x555555694c24 in _start (/home/pks/Development/git/build/t/unit-tests+0x140c24) 0x7c3ff6de2e3f is located 0 bytes after 47-byte region [0x7c3ff6de2e10,0x7c3ff6de2e3f) allocated by thread T0 here: #0 0x55555579e95b in malloc (/home/pks/Development/git/build/t/unit-tests+0x24a95b) #1 0x5555559871c2 in reftable_malloc /home/pks/Development/git/build/../reftable/basics.c:24:9 #2 0x5555559872e8 in reftable_calloc /home/pks/Development/git/build/../reftable/basics.c:54:6 #3 0x55555598f0d3 in reftable_buf_read_data /home/pks/Development/git/build/../reftable/blocksource.c:67:2 #4 0x55555598ea7e in block_source_read_data /home/pks/Development/git/build/../reftable/blocksource.c:41:19 #5 0x55555598c555 in read_block /home/pks/Development/git/build/../reftable/block.c:224:9 #6 0x55555598b69e in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:258:9 #7 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20 #8 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3 #9 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3 #10 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4 #11 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11 #12 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8 #13 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11 #14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d) #15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d) #16 0x555555694c24 in _start (/home/pks/Development/git/build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow /home/pks/Development/git/build/../reftable/basics.h:119:20 in reftable_get_be16 Shadow bytes around the buggy address: 0x7c3ff6de2b80: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa 0x7c3ff6de2c00: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa 0x7c3ff6de2c80: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa 0x7c3ff6de2d00: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa 0x7c3ff6de2d80: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fd =>0x7c3ff6de2e00: fa fa 00 00 00 00 00[07]fa fa fa fa fa fa fa fa 0x7c3ff6de2e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c3ff6de2f00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c3ff6de2f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c3ff6de3000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c3ff6de3080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Verify that the claimed block size fits into the block data before using it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-07-03reftable/block: fix OOB write with bogus inflated log sizePatrick Steinhardt1-0/+9
The "log" reftable block stores reflog information. This information is compressed using zlib. The inflated size is stored in the block header so that callers can easily learn ahead of time how large of a buffer they have to allocate to inflate the data in a single pass. So to reconstruct the full inflated block we: - Copy over the header as-is, as it's not deflated. - Append the inflated data to the buffer. The inflated block size stored in the header also includes the length of the header itself. So to figure out the bytes that should be inflated by zlib we need to subtract the header size, which is trusted data, from the block size, which is untrusted data derived from the block header. While we do verify that we were able to inflate all data as expected, we don't verify ahead of time that the encoded block length is larger than the header length. This can lead to an underflow, which makes zlib assume that it can write more data into the target buffer than we have allocated. The result is an out-of-bounds write: ==1422297==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1ff6de5231 at pc 0x55555579a628 bp 0x7fffffff4f10 sp 0x7fffffff46d0 WRITE of size 4 at 0x7c1ff6de5231 thread T0 #0 0x55555579a627 in __asan_memcpy (./build/t/unit-tests+0x246627) #1 0x55555598b093 in reftable_block_init ./build/../reftable/block.c:277:3 #2 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20 #3 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3 #4 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3 #5 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4 #6 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11 #7 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8 #8 0x55555584af4a in main ./build/../common-main.c:9:11 #9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #11 0x555555694c24 in _start (./build/t/unit-tests+0x140c24) 0x7c1ff6de5231 is located 0 bytes after 1-byte region [0x7c1ff6de5230,0x7c1ff6de5231) allocated by thread T0 here: #0 0x55555579db1b in realloc.part.0 asan_malloc_linux.cpp.o #1 0x5555559868d7 in reftable_realloc ./build/../reftable/basics.c:36:9 #2 0x55555598a98f in reftable_alloc_grow ./build/../reftable/basics.h:229:10 #3 0x55555598ae58 in reftable_block_init ./build/../reftable/block.c:269:3 #4 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20 #5 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3 #6 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3 #7 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4 #8 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11 #9 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8 #10 0x55555584af4a in main ./build/../common-main.c:9:11 #11 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #12 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077) #13 0x555555694c24 in _start (./build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow (./build/t/unit-tests+0x246627) in __asan_memcpy Shadow bytes around the buggy address: 0x7c1ff6de4f80: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd 0x7c1ff6de5000: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd 0x7c1ff6de5080: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd 0x7c1ff6de5100: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd 0x7c1ff6de5180: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd =>0x7c1ff6de5200: fa fa 04 fa fa fa[01]fa fa fa fa fa fa fa fa fa 0x7c1ff6de5280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c1ff6de5300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c1ff6de5380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c1ff6de5400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x7c1ff6de5480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Fix the bug by adding a sanity check and add a unit test. Reported-by: oxsignal <awo@kakao.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-05-19Merge branch 'ps/reftable-read-block-perffix'Junio C Hamano1-1/+6
Performance regression in not-yet-released code has been corrected. * ps/reftable-read-block-perffix: reftable: fix perf regression when reading blocks of unwanted type
2025-05-12reftable: fix perf regression when reading blocks of unwanted typePatrick Steinhardt1-1/+6
In fd888311fbc (reftable/table: move reading block into block reader, 2025-04-07), we have refactored how reftable blocks are read so that most of the logic is contained in the "block.c" subsystem itself. Most importantly, the whole logic to read the data itself is now contained in that subsystem. This change caused a significant performance regression though when reading blocks that aren't of the specific type one is searching for: Benchmark 1: update-ref: create 100k refs (revision = fd888311fbc~) Time (mean ± σ): 2.171 s ± 0.028 s [User: 1.189 s, System: 0.977 s] Range (min … max): 2.117 s … 2.206 s 10 runs Benchmark 2: update-ref: create 100k refs (revision = fd888311fbc) Time (mean ± σ): 3.418 s ± 0.030 s [User: 2.371 s, System: 1.037 s] Range (min … max): 3.377 s … 3.473 s 10 runs Summary update-ref: create 100k refs (revision = fd888311fbc~) ran 1.57 ± 0.02 times faster than update-ref: create 100k refs (revision = fd888311fbc) The root caute of the performance regression is that we changed when exactly blocks of an uninteresting type are being discarded. Previous to the refactoring in the mentioned commit we'd load the block data, read its type, notice that it's not the wanted type and discard the block. After the commit though we don't discard the block immediately, but we fully decode it only to realize that it's not the desired type. We then discard the block again, but have already performed a bunch of pointless work. Fix the regression by making `reftable_block_init()` return early in case the block is not of the desired type. This fixes the performance hit: Benchmark 1: update-ref: create 100k refs (revision = HEAD~) Time (mean ± σ): 2.712 s ± 0.018 s [User: 1.990 s, System: 0.716 s] Range (min … max): 2.682 s … 2.741 s 10 runs Benchmark 2: update-ref: create 100k refs (revision = HEAD) Time (mean ± σ): 1.670 s ± 0.012 s [User: 0.991 s, System: 0.676 s] Range (min … max): 1.652 s … 1.693 s 10 runs Summary update-ref: create 100k refs (revision = HEAD) ran 1.62 ± 0.02 times faster than update-ref: create 100k refs (revision = HEAD~) Note that the baseline performance is lower than in the original due to a couple of unrelated performance improvements that have landed since the original commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-29Merge branch 'ps/reftable-api-revamp'Junio C Hamano1-109/+175
Overhaul of the reftable API. * ps/reftable-api-revamp: reftable/table: move printing logic into test helper reftable/constants: make block types part of the public interface reftable/table: introduce iterator for table blocks reftable/table: add `reftable_table` to the public interface reftable/block: expose a generic iterator over reftable records reftable/block: make block iterators reseekable reftable/block: store block pointer in the block iterator reftable/block: create public interface for reading blocks git-zlib: use `struct z_stream_s` instead of typedef reftable/block: rename `block_reader` to `reftable_block` reftable/block: rename `block` to `block_data` reftable/table: move reading block into block reader reftable/block: simplify how we track restart points reftable/blocksource: consolidate code into a single file reftable/reader: rename data structure to "table" reftable: fix formatting of the license header
2025-04-08Merge branch 'ps/reftable-sans-compat-util'Junio C Hamano1-7/+9
Make the code in reftable library less reliant on the service routines it used to borrow from Git proper, to make it easier to use by external users of the library. * ps/reftable-sans-compat-util: Makefile: skip reftable library for Coccinelle reftable: decouple from Git codebase by pulling in "compat/posix.h" git-compat-util.h: split out POSIX-emulating bits compat/mingw: split out POSIX-related bits reftable/basics: introduce `REFTABLE_UNUSED` annotation reftable/basics: stop using `SWAP()` macro reftable/stack: stop using `sleep_millisec()` reftable/system: introduce `reftable_rand()` reftable/reader: stop using `ARRAY_SIZE()` macro reftable/basics: provide wrappers for big endian conversion reftable/basics: stop using `st_mult()` in array allocators reftable: stop using `BUG()` in trivial cases reftable/record: don't `BUG()` in `reftable_record_cmp()` reftable/record: stop using `BUG()` in `reftable_record_init()` reftable/record: stop using `COPY_ARRAY()` reftable/blocksource: stop using `xmmap()` reftable/stack: stop using `write_in_full()` reftable/stack: stop using `read_in_full()`
2025-04-07reftable/constants: make block types part of the public interfacePatrick Steinhardt1-2/+2
Now that reftable blocks can be read individually via the public interface it becomes necessary for callers to be able to distinguish the different types of blocks. Expose the relevant constants. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: expose a generic iterator over reftable recordsPatrick Steinhardt1-0/+56
Expose a generic iterator over reftable records and expose it via the public interface. Together with an upcoming iterator for reftable blocks contained in a table this will allow users to trivially iterate through blocks and their respective records individually. This functionality will be used to implement consistency checks for the reftable backend, which requires more fine-grained control over how we read data. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: make block iterators reseekablePatrick Steinhardt1-10/+13
Refactor the block iterators so that initialization and seeking are different from one another. This makes the iterator trivially reseekable by storing the pointer to the block at initialization time, which we can then reuse on every seek. This refactoring prepares the code for exposing a `reftable_iterator` interface for blocks in a subsequent commit. Callsites are adjusted accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: store block pointer in the block iteratorPatrick Steinhardt1-14/+8
The block iterator requires access to a bunch of data from the underlying `reftable_block` that it is iterating over. This data is stored by copying over relevant data into a separate set of variables. This has multiple downsides: - We require more storage space than necessary. This is more of a theoretical issue as we shouldn't ever have many blocks. - We have to perform more bookkeeping, and the variable names are inconsistent across the two data structures. This can lead to some confusion. - The lifetime of the block iterator is tied to the block anyway, but we hide that a bit by only storing pointers pointing into the block. There isn't really any good reason why we rip out parts of the block instead of storing a pointer to the block itself. Refactor the code to do so. Despite being simpler, it also allows us to decouple the lifetime of the block iterator from seeking in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: rename `block_reader` to `reftable_block`Patrick Steinhardt1-71/+71
The `block_reader` structure is used to access parsed data of a reftable block. The structure is currently treated as an internal implementation detail and not exposed via our public interfaces. The functionality provided by the structure is useful to external users of the reftable library though, for example when implementing consistency checks that need to scan through the blocks manually. Rename the structure to `reftable_block` now that the name has been made available in the preceding commit. This name is in line with the naming schema used for other data structures like `reftable_table` in that it describes the underlying entity that it provides access to. The new data structure isn't yet exposed via the public interface, which is left for a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: rename `block` to `block_data`Patrick Steinhardt1-23/+23
The `reftable_block` structure associates a byte slice with a block source. As such it only holds the data of a reftable block without actually encoding any of the details for how to access that data. Rename the structure to instead be called `reftable_block_data`. Besides clarifying that this really only holds data, it also allows us to rename the `reftable_block_reader` to `reftable_block` in the next commit, as this is the structure that actually encapsulates access to the reftable blocks. Rename the `struct reftable_block_reader::block` member accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/table: move reading block into block readerPatrick Steinhardt1-31/+56
The logic to read blocks from a reftable is scattered across both the table and the block subsystems. Besides causing somewhat fuzzy responsibilities, it also means that we have to awkwardly pass around the ownership of blocks between the subsystems. Refactor the code so that we stop passing the block when initializing a reader, but instead by passing in the block source plus the offset at which we're supposed to read a block. Like this, the ownership of the block itself doesn't need to get handed over as the block reader is the one owning the block right from the start. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/block: simplify how we track restart pointsPatrick Steinhardt1-13/+12
Restart points record the location of reftable records that do not use prefix compression and are used to perform a binary search inside of a block. These restart points are encoded at the end of a block, between the record data and the footer of a table. The block structure contains three different variables related to these restart points: - The block length contains the length of the reftable block up to the restart points. - The restart count contains the number of restart points contained in the block. - The restart bytes variable tracks where the restart point data begins. Tracking all three of these variables is unnecessary though as the data can be derived from one another: the block length without restart points is the exact same as the offset of the restart count data, which we already track via the `restart_bytes` data. Refactor the code so that we track the location of restart bytes not as a pointer, but instead as an offset. This allows us to trivially get rid of the `block_len` variable as described above. This avoids having the confusing `block_len` variable and allows us to do less bookkeeping overall. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable/blocksource: consolidate code into a single filePatrick Steinhardt1-14/+3
The code that implements block sources is distributed across a couple of files. Consolidate all of it into "reftable/blocksource.c" and its accompanying header so that it is easier to locate and more self contained. While at it, rename some of the functions to have properly scoped names. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-04-07reftable: fix formatting of the license headerPatrick Steinhardt1-6/+6
The license headers used across the reftable library doesn't follow our typical coding style for multi-line comments. Fix it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-03-21reftable: propagate specific error codes in block_writer_add()Meet Soni1-6/+7
Previously, functions block_writer_add() and related functions returned -1 when the record did not fit, forcing the caller to assume that any failure meant the entry was too big. Replace these generic -1 returns with defined error codes. This prepares the codebase for finer-grained error handling so that callers can distinguish between a block-full condition and other errors. Signed-off-by: Meet Soni <meetsoni3017@gmail.com> Acked-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-02-18reftable/basics: provide wrappers for big endian conversionPatrick Steinhardt1-6/+6
We're using a mixture of big endian conversion functions provided by both the reftable library, but also by the Git codebase. Refactor the code so that we exclusively use reftable-provided wrappers in order to untangle us from the Git codebase. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-02-18reftable/record: stop using `BUG()` in `reftable_record_init()`Patrick Steinhardt1-1/+3
We're aborting the program via `BUG()` in case `reftable_record_init()` was invoked with an unknown record type. This is bad because we may now die in library code, and because it makes us depend on the Git codebase. Refactor the code such that `reftable_record_init()` can return an error code to the caller. Adapt any callers accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-02-06Merge branch 'ps/zlib-ng'Junio C Hamano1-1/+0
The code paths to interact with zlib has been cleaned up in preparation for building with zlib-ng. * ps/zlib-ng: ci: make "linux-musl" job use zlib-ng ci: switch linux-musl to use Meson compat/zlib: allow use of zlib-ng as backend git-zlib: cast away potential constness of `next_in` pointer compat/zlib: provide stubs for `deflateSetHeader()` compat/zlib: provide `deflateBound()` shim centrally git-compat-util: move include of "compat/zlib.h" into "git-zlib.h" compat: introduce new "zlib.h" header git-compat-util: drop `z_const` define compat: drop `uncompress2()` compatibility shim
2025-01-28git-compat-util: move include of "compat/zlib.h" into "git-zlib.h"Patrick Steinhardt1-1/+0
We include "compat/zlib.h" in "git-compat-util.h", which is unnecessarily broad given that we only have a small handful of files that use the zlib library. Move the header into "git-zlib.h" instead and adapt users of zlib to include that header. One exception is the reftable library, as we don't want to use the Git-specific wrapper of zlib there, so we include "compat/zlib.h" instead. Furthermore, we move the include into "reftable/system.h" so that users of the library other than Git can wire up zlib themselves. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-01-28compat: introduce new "zlib.h" headerPatrick Steinhardt1-1/+1
Introduce a new "compat/zlib-compat.h" header that we include instead of including <zlib.h> directly. This will allow us to wire up zlib-ng as an alternative backend for zlib compression in a subsequent commit. Note that we cannot just call the file "compat/zlib.h", as that may otherwise cause us to include that file instead of <zlib.h>. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-01-21reftable/block: adjust type of the restart lengthPatrick Steinhardt1-7/+5
The restart length is tracked as a positive integer even though it cannot ever be negative. Furthermore, it is effectively capped via the MAX_RESTARTS variable. Adjust the type of the variable to be `uint32_t`. While this type is excessive given that MAX_RESTARTS fits into an `uint16_t`, other places already use 32 bit integers for restarts, so this type is being more consistent. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-01-21reftable/block: adapt header and footer size to return a `size_t`Patrick Steinhardt1-2/+2
The functions `header_size()` and `footer_size()` return a positive integer representing the size of the header and footer, respectively, dependent on the version of the reftable format. Similar to the preceding commit, these functions return a signed integer though, which is nonsensical given that there is no way for these functions to return negative. Adapt the functions to return a `size_t` instead to fix a couple of sign comparison warnings. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2025-01-21reftable/basics: adjust `hash_size()` to return `uint32_t`Patrick Steinhardt1-2/+2
The `hash_size()` function returns the number of bytes used by the hash function. Weirdly enough though, it returns a signed integer for its size even though the size obviously cannot ever be negative. The only case where it could be negative is if the function returned an error when asked for an unknown hash, but we assert(3p) instead. Adjust the type of `hash_size()` to be `uint32_t` and adapt all places that use signed integers for the hash size to follow suit. This also allows us to get rid of a couple asserts that we had which verified that the size was indeed positive, which further stresses the point that this refactoring makes sense. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-12-28reftable: avoid leaks on realloc errorRené Scharfe1-4/+6
When realloc(3) fails, it returns NULL and keeps the original allocation intact. REFTABLE_ALLOC_GROW overwrites both the original pointer and the allocation count variable in that case, simultaneously leaking the original allocation and misrepresenting the number of storable items. parse_names() and reftable_buf_add() avoid leaking by restoring the original pointer value on failure, but all other callers seem to be OK with losing the old allocation. Add a new variant of the macro, REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the allocation counter. Use it for those callers. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-11-26reftable: rename scratch bufferPatrick Steinhardt1-5/+5
Both `struct block_writer` and `struct reftable_writer` have a `buf` member that is being reused to optimize the number of allocations. Rename the variable to `scratch` to clarify its intend and provide a comment explaining why it exists. Suggested-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-11-21reftable/block: optimize allocations by using scratch bufferPatrick Steinhardt1-8/+5
The block writer needs to compute the key for every record that one adds to the writer. The buffer for this key is stored on the stack and thus reallocated on every call to `block_writer_add()`, which is inefficient. Refactor the code so that we store the buffer in the `block_writer` struct itself so that we can reuse it. This reduces the number of allocations when writing many refs, e.g. when migrating one million refs from the "files" backend to the "reftable backend. Before this change: HEAP SUMMARY: in use at exit: 80,048 bytes in 49 blocks total heap usage: 3,025,864 allocs, 3,025,815 frees, 372,746,291 bytes allocated After this change: HEAP SUMMARY: in use at exit: 80,048 bytes in 49 blocks total heap usage: 2,013,250 allocs, 2,013,201 frees, 347,543,583 bytes allocated Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-11-21reftable/block: rename `block_writer::buf` variablePatrick Steinhardt1-10/+10
Adapt the name of the `block_writer::buf` variable to instead be called `block`. This aligns it with the existing `block_len` variable, which tracks the length of this buffer, and is generally a bit more tied to the actual context where this variable gets used. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-10-17reftable: handle trivial `reftable_buf` errorsPatrick Steinhardt1-2/+7
Convert the reftable library such that we handle failures with the new `reftable_buf` interfaces. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2024-10-17reftable/record: adapt `reftable_record_key()` to handle allocation failuresPatrick Steinhardt1-5/+15
The `reftable_record_key()` function cannot pass any errors to the caller as it has a `void` return type. Adapt it and its callers such that we can handle errors and start handling allocation failures. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2024-10-17reftable: convert from `strbuf` to `reftable_buf`Patrick Steinhardt1-17/+17
Convert the reftable library to use the `reftable_buf` interface instead of the `strbuf` interface. This is mostly a mechanical change via sed(1) with some manual fixes where functions for `strbuf` and `reftable_buf` differ. The converted code does not yet handle allocation failures. This will be handled in subsequent commits. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2024-10-17reftable: stop using `strbuf_addbuf()`Patrick Steinhardt1-1/+1
We're about to introduce our own `reftable_buf` type to replace `strbuf`. Get rid of the seldomly-used `strbuf_addbuf()` function such that we have to reimplement one less function. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>
2024-10-02reftable: introduce `REFTABLE_FREE_AND_NULL()`Patrick Steinhardt1-3/+3
We have several calls to `FREE_AND_NULL()` in the reftable library, which of course uses free(3P). As the reftable allocators are pluggable we should rather call the reftable specific function, which is `reftable_free()`. Introduce a new macro `REFTABLE_FREE_AND_NULL()` and adapt the callsites accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-10-02reftable/block: handle allocation failuresPatrick Steinhardt1-2/+21
Handle allocation failures in `block_writer_init()` and `block_reader_init()`. This requires us to bubble up error codes into `writer_reinit_block_writer()`. Adapt call sites accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-05-13reftable/block: use `size_t` to track restart point indexPatrick Steinhardt1-2/+2
The function `block_reader_restart_offset()` gets the offset of the `i`th restart point. `i` is a signed integer though, which is certainly not the correct type to track indices like this. Furthermore, both callers end up passing a `size_t`. Refactor the code to use a `size_t` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-05-08Merge branch 'ps/reftable-write-optim'Junio C Hamano1-26/+46
Code to write out reftable has seen some optimization and simplification. * ps/reftable-write-optim: reftable/block: reuse compressed array reftable/block: reuse zstream when writing log blocks reftable/writer: reset `last_key` instead of releasing it reftable/writer: unify releasing memory reftable/writer: refactorings for `writer_flush_nonempty_block()` reftable/writer: refactorings for `writer_add_record()` refs/reftable: don't recompute committer ident reftable: remove name checks refs/reftable: skip duplicate name checks refs/reftable: perform explicit D/F check when writing symrefs refs/reftable: fix D/F conflict error message on ref copy
2024-04-15reftable/block: avoid copying block iterators on seekPatrick Steinhardt1-18/+14
When seeking a reftable record in a block we need to position the iterator _before_ the sought-after record so that the next call to `block_iter_next()` would yield that record. To achieve this, the loop that performs the linear seeks to restore the previous position once it has found the record. This is done by advancing two `block_iter`s: one to check whether the next record is our sought-after record, and one that we update after every iteration. This of course involves quite a lot of copying and also leads to needless memory allocations. Refactor the code to get rid of the `next` iterator and the copying this involves. Instead, we can restore the previous offset such that the call to `next` will return the correct record. Next to being simpler conceptually this also leads to a nice speedup. The following benchmark parser 10k refs out of 100k existing refs via `git-rev-list --no-walk`: Benchmark 1: rev-list: print many refs (HEAD~) Time (mean ± σ): 170.2 ms ± 1.7 ms [User: 86.1 ms, System: 83.6 ms] Range (min … max): 166.4 ms … 180.3 ms 500 runs Benchmark 2: rev-list: print many refs (HEAD~) Time (mean ± σ): 161.6 ms ± 1.6 ms [User: 78.1 ms, System: 83.0 ms] Range (min … max): 158.4 ms … 172.3 ms 500 runs Summary rev-list: print many refs (HEAD) ran 1.05 ± 0.01 times faster than rev-list: print many refs (HEAD~) Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: reuse `zstream` state on inflationPatrick Steinhardt1-10/+15
When calling `inflateInit()` and `inflate()`, the zlib library will allocate several data structures for the underlying `zstream` to keep track of various information. Thus, when inflating repeatedly, it is possible to optimize memory allocation patterns by reusing the `zstream` and then calling `inflateReset()` on it to prepare it for the next chunk of data to inflate. This is exactly what the reftable code is doing: when iterating through reflogs we need to potentially inflate many log blocks, but we discard the `zstream` every single time. Instead, as we reuse the `block_reader` for each of the blocks anyway, we can initialize the `zstream` once and then reuse it for subsequent inflations. Refactor the code to do so, which leads to a significant reduction in the number of allocations. The following measurements were done when iterating through 1 million reflog entries. Before: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 23,028 allocs, 22,906 frees, 162,813,552 bytes allocated After: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 302 allocs, 180 frees, 88,352 bytes allocated Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: open-code call to `uncompress2()`Patrick Steinhardt1-10/+28
The reftable format stores log blocks in a compressed format. Thus, whenever we want to read such a block we first need to decompress it. This is done by calling the convenience function `uncompress2()` of the zlib library, which is a simple wrapper that manages the lifecycle of the `zstream` structure for us. While nice for one-off inflation of data, when iterating through reflogs we will likely end up inflating many such log blocks. This requires us to reallocate the state of the `zstream` every single time, which adds up over time. It would thus be great to reuse the `zstream` instead of discarding it after every inflation. Open-code the call to `uncompress2()` such that we can start reusing the `zstream` in the subsequent commit. Note that our open-coded variant is different from `uncompress2()` in two ways: - We do not loop around `inflate()` until we have processed all input. As our input is limited by the maximum block size, which is 16MB, we should not hit limits of `inflate()`. - We use `Z_FINISH` instead of `Z_NO_FLUSH`. Quoting the `inflate()` documentation: "inflate() should normally be called until it returns Z_STREAM_END or an error. However if all decompression is to be performed in a single step (a single call of inflate), the parameter flush should be set to Z_FINISH." Furthermore, "Z_FINISH also informs inflate to not maintain a sliding window if the stream completes, which reduces inflate's memory footprint." Other than that this commit is expected to be functionally equivalent and does not yet reuse the `zstream`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: reuse uncompressed blocksPatrick Steinhardt1-8/+6
The reftable backend stores reflog entries in a compressed format and thus needs to uncompress blocks before one can read records from it. For each reflog block we thus have to allocate an array that we can decompress the block contents into. This block is being discarded whenever the table iterator moves to the next block. Consequently, we reallocate a new array on every block, which is quite wasteful. Refactor the code to reuse the uncompressed block data when moving the block reader to a new block. This significantly reduces the number of allocations when iterating through many compressed blocks. The following measurements are done with `git reflog list` when listing 100k reflogs. Before: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 45,755 allocs, 45,633 frees, 254,779,456 bytes allocated After: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 23,028 allocs, 22,906 frees, 162,813,547 bytes allocated Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/reader: iterate to next block in placePatrick Steinhardt1-0/+2
The table iterator has to iterate towards the next block once it has yielded all records of the current block. This is done by creating a new table iterator, initializing it to the next block, releasing the old iterator and then copying over the data. Refactor the code to instead advance the table iterator in place. This is simpler and unlocks some optimizations in subsequent patches. Also, it allows us to avoid some allocations. The following measurements show a single matching ref out of 1 million refs. Before this change: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 7,235 allocs, 7,110 frees, 301,481 bytes allocated After: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 315 allocs, 190 frees, 107,027 bytes allocated Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: move ownership of block reader into `struct table_iter`Patrick Steinhardt1-14/+29
The table iterator allows the caller to iterate through all records in a reftable table. To do so it iterates through all blocks of the desired type one by one, where for each block it creates a new block iterator and yields all its entries. One of the things that is somewhat confusing in this context is who owns the block reader that is being used to read the blocks and pass them to the block iterator. Intuitively, as the table iterator is responsible for iterating through the blocks, one would assume that this iterator is also responsible for managing the lifecycle of the reader. And while it somewhat is, the block reader is ultimately stored inside of the block iterator. Refactor the code such that the block reader is instead fully managed by the table iterator. Instead of passing the reader to the block iterator, we now only end up passing the block data to it. Despite clearing up the lifecycle of the reader, it will also allow for better reuse of the reader in subsequent patches. The following benchmark prints a single matching ref out of 1 million refs. Before: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 6,607 allocs, 6,482 frees, 509,635 bytes allocated After: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 7,235 allocs, 7,110 frees, 301,481 bytes allocated Note that while there are more allocation and free calls now, the overall number of bytes allocated is significantly lower. The number of allocations will be reduced significantly by the next patch though. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: introduce `block_reader_release()`Patrick Steinhardt1-0/+5
Introduce a new function `block_reader_release()` that releases resources acquired by the block reader. This function will be extended in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2024-04-15reftable/block: better grouping of functionsPatrick Steinhardt1-25/+25
Function definitions and declaration of `struct block_reader` and `struct block_iter` are somewhat mixed up, making it hard to see which functions belong together. Rearrange them. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>