Skip to content

[BUG FIX] fix shrink to fit shared containers and realloc - #959

Merged
lemire merged 1 commit into
masterfrom
fix-shrink-to-fit-shared-containers-and-realloc
Nov 4, 2026
Merged

lemire merged 1 commit into
masterfrom
fix-shrink-to-fit-shared-containers-and-realloc

Conversation

@SalvatorePreviti

Copy link
Copy Markdown
Contributor

shrink_to_fit: leave shared containers alone, answer bytes, never lose an array to realloc

scope net lines lines touched hunks files changed
source +14 105 (+80 -66) 19 11 edited
tests +479 534 (+509 -30) 22 7 (+1, 6 edited)

roaring_bitmap_shrink_to_fit and roaring64_bitmap_shrink_to_fit could reallocate memory that another bitmap was
reading, reported a mix of units, and could free or drop an array when realloc misbehaved.

(bug fix) Shrinking a copy-on-write copy no longer moves the arrays its source reads

container_shrink_to_fit unwrapped a shared container and reallocated its array in place, though every bitmap
sharing it reads that array. The README's thread-safety section allows passing copy-on-write copies to different
threads, so a thread reading the source could read freed memory while another shrank the copy. A shared container is
now shrunk only when this bitmap is its sole holder (reference count 1), so a copy whose partners were freed still
gives its slack back on its next shrink.

(bug fix) shrink_to_fit answers bytes

array_container_shrink_to_fit answered the values saved and run_container_shrink_to_fit the runs saved, though
their headers, both bitmap functions and the C++ shrinkToFit promise bytes. Both now answer bytes, so the 32-bit and
64-bit totals no longer mix units.

(bug fix) Shrinking to nothing frees instead of calling realloc(p, 0)

  • An emptied run container was shrunk through realloc(p, 0). glibc frees the block and answers NULL, and the
    NULL branch then freed it a second time. It is now freed, as an empty array container already was.
  • An emptied 64-bit bitmap reallocated its container arrays to 0 bytes, which keeps a block each on macOS. They are
    now freed, as the tree's node arrays already were.

(bug fix) A failed shrinking realloc keeps the array

The array and run containers freed their array and kept their count, and the 64-bit container arrays and the tree's
node arrays were overwritten with NULL. Each now keeps its larger block and answers no bytes for it. The 64-bit
bitmap and the tree still take the smaller capacity, which the larger block holds, so a later growth reallocates from
the block. The container, tree and 64-bit array shrinks and bitset_trim go through one internal helper,
croaring_realloc_shrink in memory.h, which frees at 0 bytes and keeps the block when realloc fails.

(bug fix) Roaring64Map::shrinkToFit counts the arrays of the empty bitmaps it erases

It added a fixed 88 bytes for each empty 32-bit bitmap it erased, though an emptied bitmap can still hold its key and
container arrays. It now adds what that bitmap's own shrink gives back, plus the size of the map's value type.

(perf) Shrinking a 32-bit bitmap that already fits allocates nothing

ra_shrink_to_fit allocated, copied and freed the key and container arrays even when they already fit, so every
shrink of a fitted 32-bit bitmap did that work to save no bytes. It now returns at once.

(test)

  • tests/threads_unit.cpp: a copy-on-write copy shrunk on one thread while another reads the source, for the
    thread sanitizer job.
  • tests/test_memory_hook.h: helpers installing a memory hook with the C library's aligned functions, reset in each
    test's cmocka teardown, and an allocator counting the blocks and bytes it holds and failing chosen allocations: the
    shrink tests compare each answer with the bytes the allocator got back.

🤖 Written and verified with the aid of Claude Code; I have reviewed every line.

@SalvatorePreviti SalvatorePreviti changed the title fix-shrink-to-fit-shared-containers-and-realloc [BUG FIX] fix shrink to fit shared containers and realloc Nov 4, 2026
@lemire
lemire merged commit 03af38a into master Nov 4, 2026
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants