chore: add mimalloc tests to assert size functions#23399
Open
comphead wants to merge 3 commits into
Open
Conversation
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.
Rationale for this change
Related to #23393
Rationale for this change
MemoryPoolreservations trustfn size()on every accumulator/group-values/hash-map — but those bodies are hand-written arithmetic over the struct's fields and rot silently whenever the backing typeschange (a
Vecbecomes aHashMap, a scratch buffer is added, aScalarValuevariant grows). Nothing catches the drift today. DataFusion needs an independent, deterministic ground truth to pin theseformulas down; a mimalloc-wrapping tracking
GlobalAllocgives it (exactlayout.size()sum, no size-class rounding or thread-cache lag).What changes are included in this PR?
SlidingDistinctCountAccumulator::size()— was returning a flatsize_of_val(self)(64 B) regardless of contents. Now mirrorsDistinctCountAccumulator::full_size: self + HashMap bucket capacityDataTypeinner heap.test_utils::memory_profiler— reusable module:mimalloc_tracking::TrackingAlloc— aGlobalAllocovermimalloc::MiMalloctracking livelayout.size()bytes.size_matrix— a datatype × size grid:distinct_key_cases()(5 flat leaves) +nested_key_cases()(5List<T>combinations) ×SIZES(small → xxlarge). EachCasedeclares its ownmax_deviation_pctbudget.count.rs(flat + nested), gated on the newmimallocfeature (off by default; forwards fromtest-utils→datafusion-functions-aggregate→datafusion).mimallocadded to--featuresin.github/workflows/extended.yml.Memory usage before / after
SlidingDistinctCountAccumulator::size()reported vs. real heap bytes (mimalloc tracking allocator,Int64keys):Full matrix across
Int64/Decimal128(38, 10)/Utf8/Binary/Timestamp(ns, UTC)stays within a 30% budget; nestedList<T>cases (all 5 leaves) fence at 4 000% — a documented ceiling on theknown upstream
ScalarValue::List::size()overcount, kept as a regression fence.Are these changes tested?
Two new
#[test]s under themimallocfeature:sliding_distinct_count_size_matches_allocator_flatand..._nested. Each iterates the shared matrix and asserts against the per-casemax_deviation_pct.Existing
sliding_distinct_count_accumulator_{basic,retract,merge_states}tests continue to pass.Are there any user-facing changes?
No API changes.
SlidingDistinctCountAccumulatorreservations are now accurate — queries that previously slipped pastMemoryPoollimits may now spill or fail earlier (intended).