Fix rehash(0) producing a 100%-full table - #303
Merged
Merged
Conversation
rehash(n) computed the new capacity as NormalizeCapacity(max(n, size())), which omits the growth->capacity conversion. When size() is exactly 2^k-1, NormalizeCapacity(size()) == size(), so rehash(0) resizes to a capacity equal to the element count -- a table at 100% load. Two consequences: - ctrl_ is left with no kEmpty byte, so the probe loop in find_impl() has no reachable exit and any lookup of an absent key spins forever at 100% CPU. - reset_growth_left() computes CapacityToGrowth(capacity_) - size_ < 0, which underflows, so prepare_insert()'s `growth_left() == 0` test never fires again and the table can never grow. Besides rehash(0), the same path is reached by reserve(0) (because GrowthToLowerboundCapacity(0) == 0), by the resize(0) compatibility alias on flat/node hash map/set, and by parallel_hash_set::rehash(n) with n < num_tables (n / num_tables truncates to 0). Restore the Abseil form, which converts size() to the capacity needed to hold it under the 7/8 max load before taking the maximum. This reverts the rehash() half of 1aeeff1; the parallel_hash_set::reserve() half of that commit is left intact, and is what actually reduces over-allocation. The change only affects the n == 0 path: for reserve(n) the GrowthToLowerboundCapacity(n) argument already dominates, and for n != 0 the `m > capacity_` guard means the table only ever grows. bucket_count() after reserve(n) is unchanged across flat and parallel maps. Add two regression tests. They assert on capacities rather than calling find(), so a regressed build fails cleanly instead of hanging. Fixes greg7mdp#302
wenyekui
force-pushed
the
fix-rehash-zero-full-table
branch
from
September 10, 2026 06:25
f253c29 to
f861fca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #302.
rehash(n)computes the new capacity asNormalizeCapacity(std::max(n, size())), which omits the growth→capacity conversion. Whensize()is exactly2^k-1,NormalizeCapacity(size()) == size(), sorehash(0)resizes to a capacity equal to the element count — a table at 100% load. Two consequences:ctrl_is left with nokEmptybyte, so the probe loop infind_impl()has no reachable exit: any lookup of an absent key spins forever at 100% CPU.reset_growth_left()computesCapacityToGrowth(capacity_) - size_ < 0, which underflows, soprepare_insert()'sgrowth_left() == 0test never fires again and the table can no longer grow.Besides
rehash(0), the same path is reached byreserve(0)(becauseGrowthToLowerboundCapacity(0) == 0), by theresize(0)compatibility alias on the flat/node map/set wrappers, and byparallel_hash_set::rehash(n)withn < num_tables(integer division truncates to 0).The change
This restores the Abseil form, i.e. it reverts the
rehash()half of1aeeff1("reserve and capacity not matching #18"). Theparallel_hash_set::reserve()half of that commit is left intact — that half is what actually reduces over-allocation, and it also happens to keepnn >= 1, which is whyparallel_hash_set::reserve()never hit this bug.Why this is safe
bucket_count()afterreserve(n), 12 cases ×flat_hash_mapandparallel_flat_hash_map— identical before and after:Structurally:
reserve(n)passesGrowthToLowerboundCapacity(n)asrehash'sn, andGrowthToLowerboundCapacity(n) >= n >= size(), so that argument dominates either expression. TheGrowthToLowerboundCapacity(size())term can only matter whenn < GrowthToLowerboundCapacity(size()), and there them > capacity_guard prevents any resize — except forn == 0, which is the unconditional path this fixes.The loop from #18 (
reserve(std::max(capacity(), 800000))) also behaves identically with and without this change — the capacity still doubles per iteration in both, matching your closing comment on that issue.The only behaviour that changes:
Sizes that were already safe shrink exactly as before; the ones that would have overflowed stop one tier earlier.
Tests
Two regression tests in
tests/raw_hash_set_test.cc, next to the existingRehashZero*tests:RehashZeroPreservesMaxLoadFactorandReserveZeroPreservesMaxLoadFactor. They sweepsize()from 1 to 256 and assertsize() <= CapacityToGrowth(bucket_count())plusIsValidCapacity(bucket_count()).They deliberately assert on capacities rather than calling
find(): on a regressed build a lookup would spin forever, so afind()-based test would hang CI instead of failing. Verified both ways — with the fix reverted, both fail cleanly atn = 7(on aGroup::kWidth == 8build;n = 15wherekWidth == 16).Full suite, macOS / clang,
-std=c++11:I did not add a separate test for the
parallel_hash_set::rehash(n < num_tables)entry point: it funnels intoraw_hash_set::rehash(0), so it is covered by construction, and a direct test would have to either reach into private state or risk hanging. The standalone repro for it is in #302 if you want it as a test as well — happy to add it.