Optimization for request result - #429
Conversation
Before this change, each call to CassResultIterator::next dropped previous CassRow, and created new one. CassRow contains a Vec, so this means a free + malloc call. My optimization reuses the allocation of previous Vec. This makes sense - CassValue stored in this Vec has constant size, and each row has the same amount of columns, so there should never be a need for realloc. I did (claude did) write a benchmark to measure impact. It confirmed that before my change, iterating over 10000 rows required 10000 mallocs and 10000 frees, and after the change just 1 malloc and 1 free. There was no visible performance difference, most likely due to optimizations in libc about re-acquiring just freed allocation of the same size. I still think my change makes sense. Allocator opts may degrade in multi-threaded scenario. Users may also use different allocators, with different optimisations.
d7d9c3a to
c59fde5
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements two performance optimizations for query result handling: removing the Arc allocation for CassRowsResult by implementing manual self-referential borrowing, and reusing Vec allocations when iterating through result rows.
Changes:
- Replaced Yoke-based self-referential struct with manual unsafe implementation using lifetime erasure, two-phase Arc construction, and custom Drop ordering
- Modified CassRow::from_raw_row_and_metadata to accept and reuse an old_row's Vec allocation, eliminating per-row malloc/free cycles during iteration
- Added result_iteration_bench benchmark with malloc interposition to measure allocation improvements
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scylla-rust-wrapper/src/query_result.rs | Implements manual self-referential struct for first_row using ManuallyDrop and lifetime transmutation; adds Vec reuse parameter to CassRow construction; changes from_result_payload to return Arc directly |
| scylla-rust-wrapper/src/iterator.rs | Passes old_row to from_raw_row_and_metadata to enable Vec reuse during iteration |
| scylla-rust-wrapper/src/session.rs | Updates to handle Arc returned directly from from_result_payload |
| scylla-rust-wrapper/Cargo.toml | Removes yoke dependency |
| scylla-rust-wrapper/Cargo.lock | Updates lockfile to reflect yoke removal from direct dependencies |
| examples/result_iteration_bench/result_iteration_bench.c | Adds benchmark with malloc/realloc/free interposition to measure allocation behavior during result iteration |
| examples/result_iteration_bench/CMakeLists.txt | Build configuration for benchmark |
| examples/result_iteration_bench/.gitignore | Ignores benchmark binary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
It was useful to me to analyze my improvements to the result iterator, so I thought it would be nice to keep it here.
Next commit will introduce manual self-reference inside this struct. For that, we need to have a guarantee that it is at a stable memory address. Using Arc here guarantees it. Nit: theoretically, caller of from_result_payload could create 2 such objects, use Arc::get_mut on both, swap insides, and drop one. We could fix that by introducing another indirection level, but then it would not play nice with our FFI (which requires Arc / Box to be the outermost type).
After making the first row manually borrowing from non-Arced shared data, we need a safe way to access it, without field access. This method will be it.
This commit gets rid of Arc from Arc<CassRowsResultSharedData> by manually creating a self-reference in CassRowsResult. This is safe only when CassRowsResult is at its final address, in other words pinned. In our case this is true, because CassResult that is holding it is stored in Arc. This could theoretically be abused from outside the module, by swapping contents of 2 `Arc<CassResult>` and dropping one of them. It is extremely hard to design a fully safe abstraction here :/ I don't think this is a problem here, there is very little chance of abusing this by accident.
c59fde5 to
e32e160
Compare
|
Addressed review comments. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| size_t aligned = (size + 7) & ~(size_t)7; /* align to 8 */ | ||
| if (bootstrap_used + aligned > sizeof(bootstrap_buf)) { |
There was a problem hiding this comment.
In malloc()'s bootstrap allocation path the manual alignment is hard-coded to 8 bytes. That can under-align allocations on platforms where max_align_t requires 16 (or larger) alignment, which can cause undefined behavior if dlsym() (or other early allocations) requests such alignment. Consider aligning to alignof(max_align_t) (C11) or sizeof(void*)/a union that enforces max_align_t alignment, rather than a fixed 8-byte boundary.
| if (ptr != NULL && ptr >= (void*)bootstrap_buf && | ||
| ptr < (void*)(bootstrap_buf + sizeof(bootstrap_buf))) { | ||
| size_t old_offset = (char*)ptr - bootstrap_buf; |
There was a problem hiding this comment.
The range check ptr >= (void*)bootstrap_buf && ptr < ... performs relational comparisons between pointers that may not point into the same object/array, which is undefined behavior in C. To make this well-defined, compare integer addresses instead (e.g., cast both to uintptr_t) before doing the bounds check.
| if (ptr >= (void*)bootstrap_buf && ptr < (void*)(bootstrap_buf + sizeof(bootstrap_buf))) { | ||
| return; |
There was a problem hiding this comment.
Same undefined-behavior issue here: ptr >= (void*)bootstrap_buf && ptr < ... relies on relational comparison of unrelated pointers. Please switch to comparing uintptr_t (or similar) addresses so the bootstrap-buffer check is well-defined for any pointer value, including NULL and heap pointers.
| file(GLOB EXAMPLE_SRC_FILES *.c) | ||
| include_directories(${INCLUDES}) | ||
| add_executable(${PROJECT_EXAMPLE_NAME} ${EXAMPLE_SRC_FILES}) | ||
| target_link_libraries(${PROJECT_EXAMPLE_NAME} ${PROJECT_LIB_NAME_TARGET} ${CASS_LIBS}) |
There was a problem hiding this comment.
This example uses dlsym()/RTLD_NEXT (libdl) but the target is only linked against ${PROJECT_LIB_NAME_TARGET} and ${CASS_LIBS}. On many Linux toolchains this will fail to link unless dl (or ${CMAKE_DL_LIBS}) is added to target_link_libraries for this executable.
| target_link_libraries(${PROJECT_EXAMPLE_NAME} ${PROJECT_LIB_NAME_TARGET} ${CASS_LIBS}) | |
| target_link_libraries(${PROJECT_EXAMPLE_NAME} ${PROJECT_LIB_NAME_TARGET} ${CASS_LIBS} ${CMAKE_DL_LIBS}) |
|
I don't see the point of spending my time on perfecting code of this benchmark. It works, it shows the fix reduces the allocations. |
It's good enough for a benchmark. |
This PR performs 2 optimizations related to request results.
CassRow allocation
Allocation for
VecinCassRowis reused when advancingCassResultIterator. Before we always dropped oldCassRowand created new one, resulting in 1free+ 1malloccall.Now we take the previous
Vec, clear it, and reuse it.I verified with the benchmark (second commit) that allocations are really gone, and they are! Now we have no obligatory allocations per-row, only per-request.
There was no visible performance improvement in my simple test. This is most likely because libc probably optimizes a pattern of
free+mallocwith the same size, making them really cheap.This may not be the case for other allocators, or in more complex scenarios (like multi-threaded programs), so I still think this opt makes sense
Removed Arc for shared data in CassRowsResult
See the issue about this: #244
This is solved by making the self-referential struct manually, without Yoke.
Unfortunately I see no way to apply it. I also see no good way to make the resulting code much more secure, or easy to understand.
I guess this is a tradeoff - allocation gone, in exchange for more dangerous and complex code.
I'll gladly accept suggestions for how to make it safer, better contained, simpler etc.
If reviewers think this tradeoff makes no sense, I can also drop this part of PR, and close the issue as
wontfix.Fixes: #244
Pre-review checklist
I have implemented Rust unit tests for the features/changes introduced.I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.Fixes:annotations to PR description.