perf(bigtable): delete periodic Tick loop; sizing is event-driven - #20285
Merged
sushanb merged 2 commits intoJul 31, 2026
Merged
Conversation
…loop Both were running every Tick (1-second cadence), but they're periodic-snapshot metrics — nothing changes meaningfully at 1 s granularity. sampleActiveUptimes calls sessionUptime.Record on every Ready session per Tick, so at 100 sessions this was ~6000 OTel histogram writes / min + the accompanying toOtelMetricAttrs allocations (visible in heap profiles as multi-MB residence). Matches java-bigtable's session.uptime cadence at MetricsImpl.java: 193 — scheduleAtFixedRate(this::recordAsyncSessionMetrics, 1, 1, TimeUnit.MINUTES). Changes: - Drop the two calls from Tick's body (session_pool_scaling.go). - Add slowMetricsInterval = 1 minute and startSlowMetricsLoop, mirroring the existing startSweepStuckSessionsLoop / start AfePruneLoop patterns (dedicated goroutine per concern). - Wire into Start() alongside the other periodic loops. Tick keeps its 1-s cadence for sizing decisions (still needed for reactive scale-up on burst arrivals — Java handles that via event-driven hooks, but Go's current shape uses Tick).
Tick's remaining body (after the recordTimeSeries / sampleActiveUptimes move to the 1-min slow-metrics loop on the base of this stack) is pure sizing: sizer.Decide + createSession fan-out. Every meaningful pool- state transition already fires spawnTickOnce so a background timer is redundant: onActive → signalFree (session became Ready) onClosing → spawnTickOnce (Ready→closing transition) CheckoutSession → spawnTickOnce (waiter park on empty pool) The one path that wouldn't otherwise fire an event is a server-driven UpdateConfig that raises MinSessionCount on an idle pool — add an explicit spawnTickOnce at the end of UpdateConfig to cover it. spawnTickOnce is CAS-guarded via tickPending so a burst of listener fires coalesces to one Tick body. Matches java-bigtable's fully event-driven poolSizer — no periodic timer for sizing. On an idle pool the pool now consumes zero CPU between events instead of the previous 1-second Tick baseline. Changes: - Delete startTickLoop + tickInterval const. - Drop p.startTickLoop(ctx) from Start(); keep the pre-start spawnTickOnce that seeds MinSessions. - Add p.spawnTickOnce(p.poolCtx) at end of UpdateConfig. Verified: 5× stress-run of TestSessionPool_* / TestCheckoutSession* / TestPool_* under -race passes; internal/session suite passes.
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the session pool to use an event-driven sizing model, removing the periodic 1-second tick watchdog. Instead, a slower 1-minute background loop is introduced to record metrics, reducing CPU and OTel histogram write overhead. Feedback was provided to add panic recovery to this new background loop to prevent potential application crashes during metric collection.
Comment on lines
+481
to
495
| func (p *SessionPoolImpl) startSlowMetricsLoop(ctx context.Context) { | ||
| go func() { | ||
| ticker := time.NewTicker(tickInterval) | ||
| ticker := time.NewTicker(slowMetricsInterval) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| p.tickOnce(ctx) | ||
| p.recordTimeSeries() | ||
| p.sampleActiveUptimes(ctx) | ||
| } | ||
| } | ||
| }() | ||
| } |
Contributor
There was a problem hiding this comment.
The new background goroutine startSlowMetricsLoop runs p.recordTimeSeries() and p.sampleActiveUptimes(ctx) without any panic recovery. Previously, these methods were executed within tickOnce, which was protected by a panic recovery block. If either of these methods panics (for example, due to a nil pointer or concurrent map access during pool shutdown/re-initialization), it will crash the entire application.
Consider wrapping the ticker execution block in a helper function with a recover() block to restore the previous level of safety and prevent background panics from crashing the process.
Suggested change
| func (p *SessionPoolImpl) startSlowMetricsLoop(ctx context.Context) { | |
| go func() { | |
| ticker := time.NewTicker(tickInterval) | |
| ticker := time.NewTicker(slowMetricsInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| p.tickOnce(ctx) | |
| p.recordTimeSeries() | |
| p.sampleActiveUptimes(ctx) | |
| } | |
| } | |
| }() | |
| } | |
| func (p *SessionPoolImpl) startSlowMetricsLoop(ctx context.Context) { | |
| go func() { | |
| ticker := time.NewTicker(slowMetricsInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| func() { | |
| defer func() { | |
| _ = recover() | |
| }() | |
| p.recordTimeSeries() | |
| p.sampleActiveUptimes(ctx) | |
| }() | |
| } | |
| } | |
| }() | |
| } |
Contributor
Author
There was a problem hiding this comment.
mutianf
approved these changes
Jul 31, 2026
3 tasks
sushanb
pushed a commit
that referenced
this pull request
Aug 3, 2026
🤖 I have created a release *beep* *boop* --- ## [1.52.0](bigtable/v1.51.0...bigtable/v1.52.0) (2026-08-03) ### Features * **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency) ([#20204](#20204)) ([bcbf714](bcbf714)) * **bigtable:** Add ClientConfig.DisableSession to opt out of session backend ([#20297](#20297)) ([7ee5e44](7ee5e44)) * **bigtable:** Add getClientConfigDirectAccessChecker for session pools ([#20209](#20209)) ([3b8d30a](3b8d30a)) * **bigtable:** Add NoOpChannelPrimer for session channel pools ([#20208](#20208)) ([d055a8a](d055a8a)) * **bigtable:** Add per-AFE sessionList for the two-tier session pool ([#20224](#20224)) ([dbf0c3f](dbf0c3f)) * **bigtable:** Add protoRowToRow conversion helper for TableShim ([#20257](#20257)) ([1297143](1297143)) * **bigtable:** Add Session debug surface (observability fields + methods) ([#20211](#20211)) ([d8d3e16](d8d3e16)) * **bigtable:** Add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) ([#20215](#20215)) ([b9e53c6](b9e53c6)) * **bigtable:** Add Session struct + state machine ([#20117](#20117)) ([09acbb3](09acbb3)) * **bigtable:** Add session.Config.EnableDebug to gate sessionz debug state ([#20247](#20247)) ([ce74c31](ce74c31)) * **bigtable:** Add SessionClient + SessionTable + lazyPool ([#20228](#20228)) ([ab2c96c](ab2c96c)) * **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug) ([#20225](#20225)) ([683eda8](683eda8)) * **bigtable:** Rename session pool display to <resource-id>-<PERM> ([#20248](#20248)) ([35e146e](35e146e)) * **bigtable:** Route Client.Open()-returned *Table through the Diverter ([#20273](#20273)) ([2b81c7d](2b81c7d)) * **bigtable:** State-based classification for abnormal session close ([#20243](#20243)) ([f2905b7](f2905b7)) * **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED ([#20269](#20269)) ([36540af](36540af)) * **bigtable:** TTL-on-idle cache for per-resource session.TableAPI ([#20263](#20263)) ([00b2a49](00b2a49)) * **bigtable:** Wire Diverter on Client and route Open* via TableShim ([#20256](#20256)) ([b32fbd7](b32fbd7)) ### Bug Fixes * **bigtable:** AFE picker latency signal — subtract poolWait and compute TransportLatency = wire − backend at source ([#20281](#20281)) ([bb8c4d5](bb8c4d5)) * **bigtable:** Guard NewStream OnFinish against grpc-go double-fire ([#20295](#20295)) ([b51da29](b51da29)) * **bigtable:** Real per-resource pool teardown on sessionTable.Close + cache close-race gate ([#20264](#20264)) ([599aea9](599aea9)) * **bigtable:** Session.durations / session.uptime — set explicit histogram bucket boundaries ([#20276](#20276)) ([97eee22](97eee22)) * **bigtable:** SessionTableHandle self-heals across cache eviction ([#20296](#20296)) ([0dd98cd](0dd98cd)) * **bigtable:** Translate ctx errors to gRPC status on session vRPC ([#20299](#20299)) ([0f3b2a5](0f3b2a5)) * **bigtable:** Treat PingAndWarm NotFound as a successful prime ([#20219](#20219)) ([a1557ad](a1557ad)) ### Performance Improvements * **bigtable:** Delete periodic Tick loop; sizing is event-driven ([#20285](#20285)) ([2c096bd](2c096bd)) * **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot path ([#20280](#20280)) ([bd0e400](bd0e400)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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
2 participants
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.
Summary
After #20283 moved `recordTimeSeries` and `sampleActiveUptimes` to a dedicated 1-min loop, `Tick`'s body is pure sizing (`sizer.Decide` + `createSession` fan-out). Every meaningful pool-state transition already fires `spawnTickOnce` so a background timer is redundant.
Event coverage audit
`spawnTickOnce` is CAS-guarded via `tickPending`, so a burst of listener fires coalesces to one Tick body.
Java parity
Matches java-bigtable's fully event-driven `poolSizer`: no periodic timer for sizing. Sizing decisions ride on the events that would trigger them anyway.
Impact
Changes
Net: +9 / -25 lines.
Test plan
What this does NOT change