Repository navigation
perf(spanner): add fast-path for multiplexed session acquisition - #18317
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors multiplexed session management in both the synchronous and asynchronous database session managers. Key changes include introducing a fast path for retrieving already-initialized multiplexed sessions, replacing time.time() with time.monotonic() for interval calculations, implementing a safer session rotation mechanism (_rotate_multiplexed_session) that swaps sessions before deletion, and using event-based waiting for cleaner termination. Extensive unit tests have been added to cover these changes. The review feedback highlights multiple violations of the Repository Style Guide (Section 4: Unit Testing and Mock Hygiene) where time.monotonic is patched globally in the tests. To ensure test isolation, these mocks should be localized to the target module's import path.
- Bypass initialization and multiplexed session locks on steady-state queries. - Build replacement sessions outside the lock during maintenance rotation, holding the lock only for the pointer swap. - Replace maintenance sleep loops with event wait for immediate shutdown termination. - Use monotonic time for maintenance intervals and clear local manager references before waiting.
1b8b33e to
7bf0dcb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the multiplexed session management in both the synchronous and asynchronous database session managers. Key changes include adding a fast path in _get_multiplexed_session to return an existing session without locking, improving thread safety by assigning sessions and maintenance threads to local variables before updating instance state, introducing a _rotate_multiplexed_session helper to safely swap and delete sessions, and replacing the polling sleep in _maintain_multiplexed_session with an event-wait mechanism using time.monotonic(). Additionally, comprehensive unit tests have been added to verify these new behaviors and edge cases. No review comments were provided, so there is no feedback to address.
| await self._multiplexed_session.delete() | ||
| session_to_delete = self._multiplexed_session | ||
| self._multiplexed_session = None | ||
| await session_to_delete.delete() |
There was a problem hiding this comment.
Note: This is strictly speaking a bug. Multiplexed sessions cannot be deleted. I will fix this in a separate follow-up pull request. I kept it for now to keep the change as small as possible.
| self._multiplexed_session_thread.start() | ||
| maintenance_thread.start() | ||
| self._multiplexed_session_thread = maintenance_thread | ||
| self._multiplexed_session = session |
There was a problem hiding this comment.
Assume if maintenance_thread.start() fails, now the local session gets orphaned on the Spanner backend because self._multiplexed_session isn't updated until like 149. I believe this opens a scope for session leaks.
Old code assigned it immediately after creation, ensuring close() can always clean it up if things go wrong.
There was a problem hiding this comment.
Just noticed from your other comment that multiplexed sessions cannot be deleted. We should be good here then.
🤖 I have created a release *beep* *boop* --- ## [3.72.0](google-cloud-spanner-v3.71.0...google-cloud-spanner-v3.72.0) (2026-11-12) ### Features * **dbapi:** wire timeout parameter through Connection to execute_sql ([#18484](#18484)) ([98fc304](98fc304)) * declare Python3.15 support ([8775794](8775794)) * **spanner:** auth login support for Spanner Omni endpoints ([#18273](#18273)) ([9e0548d](9e0548d)) * update API sources and regenerate ([#18797](#18797)) ([be4d954](be4d954)) ### Bug Fixes * **google-cloud-spanner:** clear buffered DDL statements on rollback and flush on autocommit ([#18636](#18636)) ([15d6a2c](15d6a2c)) * **google-cloud-spanner:** do not replay DDL statements on transaction retry ([#18625](#18625)) ([78b9da2](78b9da2)) * **spanner:** correctly wrap async streaming calls in metrics interceptor ([#18581](#18581)) ([2d19b0a](2d19b0a)) * **spanner:** eliminate duplicate metrics capture and measure full streaming latency ([#18605](#18605)) ([a7678fd](a7678fd)) * **spanner:** record streaming operation completion when stream finishes ([#18704](#18704)) ([ddeae47](ddeae47)) * **spanner:** release transaction lock if inline begin fails ([#18409](#18409)) ([dd24029](dd24029)) * **spanner:** stop deleting multiplexed sessions on rotation and close ([#18332](#18332)) ([c38e960](c38e960)) ### Performance Improvements * **spanner:** add fast-path for multiplexed session acquisition ([#18317](#18317)) ([9569b39](9569b39)) * **spanner:** avoid allocating empty RequestOptions on read and query paths ([#18422](#18422)) ([3743ff0](3743ff0)) * **spanner:** build ExecuteSqlRequest on the raw protobuf message ([#18379](#18379)) ([6d91fec](6d91fec)) * **spanner:** eliminate redundant tracing span in run_in_transaction ([#18357](#18357)) ([181e26a](181e26a)) * **spanner:** optimize built-in metrics hot path and harden concurrency ([#18329](#18329)) ([f2a791f](f2a791f)) * **spanner:** optimize query option merging and prevent in-place mutation ([#18358](#18358)) ([8cb3c6a](8cb3c6a)) * **spanner:** optimize query parameter encoding with direct type dispatch ([#18359](#18359)) ([f06497c](f06497c)) * **spanner:** optimize query result decoding and type resolution ([#18330](#18330)) ([ce72986](ce72986)) * **spanner:** optimize request ID header generation and retry closures ([#18408](#18408)) ([ad03b95](ad03b95)) * **spanner:** optimize row popping in StreamedResultSet ([#18316](#18316)) ([e66103d](e66103d)) * **spanner:** optimize single-chunk results and single-row lookups ([#18602](#18602)) ([05b3775](05b3775)) * **spanner:** prune lock and begin event allocations on single-use snapshots ([#18420](#18420)) ([ab56473](ab56473)) * **spanner:** short-circuit trace_call when tracing is inactive ([#18356](#18356)) ([58031be](58031be)) * **spanner:** support PartialResultSet.last with background stream draining ([#18320](#18320)) ([80efd63](80efd63)) ### Documentation * fix typos ([#18979](#18979)) ([5c8baeb](5c8baeb)) --- 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>
Results Summary
spanner-fast-path-mux-session-acquisition)main)The performance gain from this optimization is minimal in an end-to-end test.