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.
2 participants
Results Summary
spanner-fast-path-mux-session-acquisition)main)The performance gain from this optimization is minimal in an end-to-end test.