statistics: simplify FMSketch by removing swiss map and pool#66590
statistics: simplify FMSketch by removing swiss map and pool#66590ti-chi-bot[bot] merged 13 commits intopingcap:masterfrom
Conversation
Go 1.24+ uses swiss tables natively, so the dolthub/swiss dependency
is no longer needed. Replace it with native map[uint64]struct{} and
use standard library functions (maps.Clone, maps.DeleteFunc). Remove
the sync.Pool for FMSketch as its benefit is marginal — nil the
references instead to allow prompt GC.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ti-chi-bot
bot
added
do-not-merge/needs-tests-checked
release-note-none
|
Review Complete Findings: 2 issues |
ti-chi-bot
bot
added
the
size/L
|
Hi @mjonss. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughReplaced dolthub/swiss/maphash usage with Go native maps for FMSketch, removed the FMSketch sync.Pool lifecycle, and changed code to nil FMSketch references (letting GC reclaim) instead of returning them to a pool; corresponding dependency and Bazel entries were removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/statistics/fmsketch.go (1)
232-238: Consider native map memory overhead inMemoryUsage.The current calculation
8 * len(s.hashset)accounts only for the key storage. Go's nativemaphas additional overhead (bucket headers, overflow pointers, etc.) that varies by map size. For large maps, actual memory usage could be 1.5-2x the calculated value.This may be acceptable if the estimation is intended to be conservative, but consider documenting the approximation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/fmsketch.go` around lines 232 - 238, The MemoryUsage implementation for FMSketch underestimates map overhead by counting only key sizes; update the calculation in MemoryUsage (and its comment) to either apply a conservative map-overhead factor (e.g., multiply 8*len(s.hashset) by ~1.5–2.0) or switch to a more accurate estimation using runtime/unsafe inspection, and explicitly document that this is an approximation; refer to the FMSketch type, its MemoryUsage method, and fields mask, maxSize and s.hashset when making the change so the comment and returned value both reflect the adjusted estimate or the rationale for keeping an approximation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/statistics/fmsketch.go`:
- Around line 232-238: The MemoryUsage implementation for FMSketch
underestimates map overhead by counting only key sizes; update the calculation
in MemoryUsage (and its comment) to either apply a conservative map-overhead
factor (e.g., multiply 8*len(s.hashset) by ~1.5–2.0) or switch to a more
accurate estimation using runtime/unsafe inspection, and explicitly document
that this is an approximation; refer to the FMSketch type, its MemoryUsage
method, and fields mask, maxSize and s.hashset when making the change so the
comment and returned value both reflect the adjusted estimate or the rationale
for keeping an approximation.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
go.modpkg/statistics/analyze.gopkg/statistics/fmsketch.gopkg/statistics/fmsketch_test.gopkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/storage/json.gopkg/statistics/row_sampler.gopkg/statistics/table.go
💤 Files with no reviewable changes (2)
- go.mod
- pkg/statistics/table.go
There was a problem hiding this comment.
Pull request overview
Removes the archived dolthub/swiss dependency from FMSketch and simplifies lifecycle management by switching to native Go maps and dropping the sync.Pool-based reuse pattern.
Changes:
- Replaced
*swiss.Map[uint64,bool]withmap[uint64]struct{}and updated related operations (Clone, pruning deletes, NDV calc). - Removed
FMSketchpooling/reset APIs and updated call sites to nil references instead of returning sketches to a pool. - Dropped
dolthub/swiss(and its indirect dependency) fromgo.mod/go.sumand updated unit tests accordingly.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/statistics/fmsketch.go | Switches hashset implementation to native map and removes pool-based lifecycle methods. |
| pkg/statistics/fmsketch_test.go | Updates tests to reflect native map operations (len, range). |
| pkg/statistics/table.go | Removes per-column/index sketch “destroy to pool” calls during table release. |
| pkg/statistics/row_sampler.go | Replaces sketch pool-destruction with nil-ing the sketch slice reference. |
| pkg/statistics/analyze.go | Replaces per-result sketch destruction with nil-ing Fms. |
| pkg/statistics/handle/storage/json.go | Nils column FMSketch after JSON dump instead of returning it to a pool. |
| pkg/statistics/handle/globalstats/global_stats.go | Nils merged partition FMSketch references after computing global NDV. |
| pkg/statistics/handle/globalstats/global_stats_async.go | Nils merged async FMSketch references after computing global NDV. |
| go.mod | Removes github.com/dolthub/swiss and indirect github.com/dolthub/maphash. |
| go.sum | Removes checksums for the removed dolthub dependencies. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/statistics/fmsketch.go (1)
238-244: Consider noting that this is a lower-bound estimate.The calculation accounts for key storage (8 bytes per
uint64) but not Go map's structural overhead (header, buckets, load factor slack). This results in an underestimate of actual memory.For estimation purposes this is likely acceptable, but if precise memory tracking becomes important, the calculation could include a rough overhead factor (e.g.,
~1.5xto2xthe key-only size for typical map sizes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/fmsketch.go` around lines 238 - 244, The MemoryUsage method on FMSketch currently computes only the raw bytes for mask/maxSize and the uint64 keys in s.hashset, which underestimates real Go map memory; update the implementation of FMSketch.MemoryUsage (and its comment) to either document that the returned value is a lower-bound estimate or include an approximate map overhead multiplier (e.g., 1.5–2x) when computing sum from len(s.hashset), and mention the omission of Go map headers/buckets in the comment so callers know this is not the full heap usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/statistics/fmsketch.go`:
- Around line 238-244: The MemoryUsage method on FMSketch currently computes
only the raw bytes for mask/maxSize and the uint64 keys in s.hashset, which
underestimates real Go map memory; update the implementation of
FMSketch.MemoryUsage (and its comment) to either document that the returned
value is a lower-bound estimate or include an approximate map overhead
multiplier (e.g., 1.5–2x) when computing sum from len(s.hashset), and mention
the omission of Go map headers/buckets in the comment so callers know this is
not the full heap usage.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DEPS.bzlpkg/statistics/BUILD.bazelpkg/statistics/fmsketch.go
💤 Files with no reviewable changes (2)
- pkg/statistics/BUILD.bazel
- DEPS.bzl
|
@mjonss Please fix the linter. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66590 +/- ##
================================================
+ Coverage 77.6886% 78.2516% +0.5630%
================================================
Files 2007 1939 -68
Lines 549532 539835 -9697
================================================
- Hits 426924 422430 -4494
+ Misses 120947 116958 -3989
+ Partials 1661 447 -1214
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I suggest attaching benchmark data for scenarios such as partitioned tables and concurrent ANALYZE to prove that removing the pool won't cause performance issues. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/statistics/fmsketch.go (1)
238-244: Memory calculation is an approximation; consider documenting this.The comment states each map entry consumes 8 bytes, but Go maps have additional overhead (bucket structure, tophash bytes, load factor). The actual memory per entry is typically higher (~15-25 bytes depending on load factor).
If this is intentionally a rough estimate for memory tracking, consider updating the comment to clarify it's an approximation, e.g., "approximately 8 bytes per key (excluding map overhead)".
📝 Suggested comment clarification
// MemoryUsage returns the total memory usage of a FMSketch. func (s *FMSketch) MemoryUsage() (sum int64) { // As for the variables mask(uint64) and maxSize(int) each will consume 8 bytes. This is the origin of the constant 16. - // And for the variables hashset(map[uint64]struct{}), each element in map will consume 8 bytes(uint64 key). + // And for the variables hashset(map[uint64]struct{}), we estimate 8 bytes per entry (key size only, excluding Go map overhead). sum = int64(16 + 8*len(s.hashset)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/fmsketch.go` around lines 238 - 244, Update the MemoryUsage comment for method FMSketch.MemoryUsage to state that the returned value is an approximation (e.g., "approximate: counts 8 bytes per uint64 key and 16 bytes for mask/maxSize, excluding Go map overhead such as bucket/tophash structures"), and mention that actual per-entry memory is typically higher depending on map load factor; keep the calculation code (sum = int64(16 + 8*len(s.hashset))) unchanged but clarify in the comment that the 8 bytes per entry excludes map overhead and is a rough estimate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/statistics/fmsketch.go`:
- Around line 238-244: Update the MemoryUsage comment for method
FMSketch.MemoryUsage to state that the returned value is an approximation (e.g.,
"approximate: counts 8 bytes per uint64 key and 16 bytes for mask/maxSize,
excluding Go map overhead such as bucket/tophash structures"), and mention that
actual per-entry memory is typically higher depending on map load factor; keep
the calculation code (sum = int64(16 + 8*len(s.hashset))) unchanged but clarify
in the comment that the 8 bytes per entry excludes map overhead and is a rough
estimate.
|
/retest |
|
@mjonss: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@hawkingrei Found this during my testing: |
|
A flame graph showing the bad performance: And the history + comment from Claude Code: |
ti-chi-bot
bot
added
do-not-merge/hold
[LGTM Timeline notifier]Timeline:
|
- Clarify MemoryUsage comment (CodeRabbit) - Add "Release for GC." comments at nil-assignment sites (0xPoe) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/statistics/fmsketch.go (1)
238-243: Memory estimation is an approximation.The 8 bytes per entry estimate covers only the key size. Go maps have additional overhead (buckets, overflow pointers, etc.) that scales with capacity, not just element count. This is acknowledged in the comment with "excluding Go map overhead." For a more accurate estimate, consider using a multiplier (e.g., ~2x) or documenting that this is a lower bound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/statistics/fmsketch.go` around lines 238 - 243, The MemoryUsage() estimate currently counts 8 bytes per map entry (key only) and notes it excludes Go map overhead; update the implementation and/or comment in FMSketch.MemoryUsage to reflect a more realistic estimate by either applying a multiplier (e.g., 2x) to account for map bucket/overflow overhead or explicitly state it is a lower bound; reference the variables mask, maxSize and hashset and ensure the comment and returned calculation consistently indicate whether the value is an approximate lower bound or a multiplied approximation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/statistics/handle/storage/json.go`:
- Around line 125-126: Inside the ForEachColumnImmutable callback you're
mutating the passed column by setting col.FMSketch = nil which violates the
"Don't change the content when calling this function" contract; remove that
assignment from the callback and instead either rely on GC or collect the column
identifiers/indices (or pointers) into a local slice and after
ForEachColumnImmutable returns iterate that slice to call cleanup/clear on
FMSketch (or set to nil) outside the iteration; keep the existing
hist.DestroyAndPutToPool() call in the callback but move any modification of
col.FMSketch to the post-iteration cleanup step.
---
Nitpick comments:
In `@pkg/statistics/fmsketch.go`:
- Around line 238-243: The MemoryUsage() estimate currently counts 8 bytes per
map entry (key only) and notes it excludes Go map overhead; update the
implementation and/or comment in FMSketch.MemoryUsage to reflect a more
realistic estimate by either applying a multiplier (e.g., 2x) to account for map
bucket/overflow overhead or explicitly state it is a lower bound; reference the
variables mask, maxSize and hashset and ensure the comment and returned
calculation consistently indicate whether the value is an approximate lower
bound or a multiplied approximation.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/statistics/analyze.gopkg/statistics/fmsketch.gopkg/statistics/handle/globalstats/global_stats.gopkg/statistics/handle/globalstats/global_stats_async.gopkg/statistics/handle/storage/json.gopkg/statistics/row_sampler.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/statistics/row_sampler.go
Add a standalone benchmark tool that measures FMSketch allocation, merge, serialization, and GC behavior across three phases matching the production data flow (region->partition, save/load, partition->global). Unlike testing.B this gives full control over GC timing and heap measurement. Build-tag gated destroy functions allow A/B comparison between master (pool+Clear) and PR (nil) on the same benchmark code. Includes benchmark results for 6 configurations (master/stash/PR x destroy/no-destroy) showing the PR is fastest overall at ~12s vs 14-20s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Re-ran PR benchmark after changing NewFMSketch to pre-allocate map to maxSize capacity. Phase 1 improved from 9.0s to 7.3s (-19%) by avoiding incremental map rehashing during merge. Total improved from 12.0s to 10.3s. Updated report with new numbers and consistent table ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add package-level testing.B benchmarks for FMSketch merge, encode/decode, and the old destroy path, plus a benchmark report summarizing results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eport" This reverts commit 4e8e934. This was just to share some previous results and benchmarks, not relevant!
…ocation" This reverts commit 4a249c6. Removing benchmark data and report, since only for reference, not for PR
This reverts commit 3729b07. Benchmarks for PR, not to be merged
|
Performance report / Benchmark of wall clock and allocations After playing around a lot and trying to collect profiles etc. during ANALYZE TABLE or having micro benchmarks tests, without resulting in any useful data for this PR, I created a simulation program to simulate the use of FMSketch during the three phases of ANALYZE TABLE for partitioned tables:
So in total: The old implementation is 89% slower than this PR! For repeatability, check the reverted commits in the commit history of this PR. |
Great work！ 👍 |
mjonss
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/statistics/fmsketch.go`:
- Around line 197-200: The FMSketch is being constructed without initializing
its maxSize, leaving it zero and breaking threshold-based pruning; in the
FMSketchFromProto construction set the FMSketch.maxSize from the proto (use
protoSketch.MaxSize, falling back to len(protoSketch.Hashset) if MaxSize isn't
available) so that the invariant is local to FMSketch and pruning/merge logic
(methods on FMSketch) behave correctly.
|
@mjonss: You cannot manually add or delete the reviewing state labels, only I and the tursted members have permission to do so. DetailsIn response to adding label named approved. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xPoe, bb7133, hawkingrei, terry1purcell The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
@terry1purcell: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Go 1.24+ uses swiss tables natively, so the dolthub/swiss dependency
is no longer needed, and is in fact deprecated. Replace it with native map[uint64]struct{} and
use standard library functions (maps.Clone, maps.DeleteFunc). Remove
the sync.Pool for FMSketch as its benefit is marginal — nil the
references instead to allow prompt GC.
What problem does this PR solve?
Issue Number: close #66589
Problem Summary:
FMSketch uses
github.com/dolthub/swissfor its internal hashset. Since Go 1.24, the standard runtime map uses swiss tables natively, making the third-party dependency redundant. Thesync.Poolfor FMSketch adds complexity across many call sites for marginal benefit.What changed and how does it work?
*swiss.Map[uint64, bool]with nativemap[uint64]struct{}(zero-sized values save 1 byte per entry)dolthub/swissdependency fromgo.modfmSketchPool(sync.Pool),reset(), andDestroyAndPutToPool()from FMSketchDestroyAndPutToPoolcall sites to allow prompt GCmaps.CloneforCopy()andmaps.DeleteFuncfor mask-based pruningMemoryUsage()to reflect 8 bytes per entry (was 9 with bool)maxSizecapacity inNewFMSketchto avoid rehashing during mergeBenchmark results
Standalone benchmark simulating the production ANALYZE flow (region→partition merge, save/load round-trip, partition→global merge) with 8000 partitions × 50 regions on Go 1.25.6 darwin/arm64. Median of 5 iterations:
47% faster overall. The
swiss.Map.Clear()called byDestroyAndPutToPool()on every sketch release costs far more than the GC it avoids. GC pauses are negligible (29 runs, 1.4ms total across 7.3s of work).Full benchmark report and raw data in
data/fmsketch-bench-results/. Standalone benchmark tool incmd/fmsketch-bench/.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit