perf(spanner): store buffered_values in a VecDeque#6037
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 updates ResultSet in src/spanner/src/result_set.rs by changing the type of buffered_values from Vec to VecDeque to optimize queue operations. Additionally, it introduces a capacity reservation on ready_rows to pre-allocate memory based on the buffered values and column count. There are no review comments, and I have no feedback to provide.
… googleapis main (#277) Repoint all eight google-cloud-* git dependencies (spanner, auth, lro, wkt, gax, both admin crates, spanner-grpc-mock) from fornwall/google-cloud-rust @ 3872d288 to googleapis/google-cloud-rust @ a69f6d5771ea454cadb6aace92a0f114b1b27e50 (current upstream main, a strict 36-commit fast-forward — the old pin was itself an upstream commit, so the fork never diverged at the pin). The O(n^2) row-drain perf fix (fork PR fornwall/google-cloud-rust#2, closed) was never in the old pin and is now pending upstream as googleapis/google-cloud-rust#6037; this bump is perf-neutral. No driver code changes were needed. Also update deny.toml allow-git, the CLAUDE.md revert-checklist rev, and the benchmarks README PR reference. Claude-Session: https://claude.ai/code/session_01Ao7MRWDTdb3rdgN9kWBf3e Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
fornwall
changed the title
262f334 to
7ac6cb6
Compare
fornwall
changed the title
process_partial_result_set drained values off the front of a `Vec` once per complete row, causing a performance issue to due excessive `memmove`, especially on larger result sets. Fix this by changing `buffered_values` to a `VecDeque`. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
7ac6cb6 to
fe188ee
Compare
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6037 +/- ##
==========================================
- Coverage 97.02% 97.01% -0.01%
==========================================
Files 253 253
Lines 63547 63549 +2
==========================================
- Hits 61655 61654 -1
- Misses 1892 1895 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@fornwall Thanks for this fix. This does indeed make a significant difference.
I was not able to reproduce this, though. At least not with real Spanner. It could be that the Emulator uses a different chunking strategy that real Spanner, making the comparison different. What I saw using real Spanner was this:
|
3 participants
process_partial_result_set()drained values off the front of aVeconce per complete row, causing a performance issue (being an order of magnitude slower than e.g. the python spanner client) to due excessivememmoveon larger result sets.Fix this by changing
buffered_valuesto aVecDeque.See https://github.com/fornwall/spanner-client-perf-issue for a benchmark that contains before and after flamegraphs and concrete numbers.
Additionally, as a related minor enhancement, introduce a capacity reservation on
ready_rowsto pre-allocate memory based on the buffered values and column count.