execute_plan: don't build temporary vec of rows#2918
Merged
Conversation
Centril
force-pushed
the
centril/execute-plan-no-temp-vecs
branch
from
54e7434 to
b45b193
Compare
gefjon
approved these changes
Jul 7, 2025
Contributor
gefjon
left a comment
gefjon
left a comment
There was a problem hiding this comment.
Good code, good comments, good benchmarks. A lot of this RowListBuilder stuff could be moved out of the code-ownered websocket.rs, IMO, since we're trying to restrict changes to the format of the messages, not the procedures by which they are constructed. But I don't think it needs to get moved, I'm just flagging it as an option.
Collaborator
|
I would like to review this before it merges.
In particular I may ask that we do this. |
bfops
added
release-any
Contributor
Author
I think we should split |
joshua-spacetime
previously requested changes
Jul 11, 2025
Collaborator
joshua-spacetime
left a comment
joshua-spacetime
left a comment
There was a problem hiding this comment.
I do think it is an opportune time to move the non-spacetime types out of websocket.rs. This file should really be reserved for the public message schemas only.
Centril
force-pushed
the
centril/execute-plan-no-temp-vecs
branch
from
6271604 to
c134d82
Compare
Centril
dismissed
joshua-spacetime’s stale review
Addressed core of review. Further refactorings aren't related to PR's goals and can be done in follow ups.
mamcx
pushed a commit
that referenced
this pull request
Aug 26, 2025
# Description of Changes
Avoid building a temporary `Vec` in `execute_plan` by exposing a
list-building interface instead.
- The old `fn encode_list` is rewritten in terms of this list-building
interface.
- The `BsatnRowList` and `BsatnRowListBuilder` types are split into two
entirely separate types. The latter now tries to recognize the case
where there isn't a known static layout, but where the BSATN lengths
happen to be the same for all rows anyways. In those cases, the
allocation of `RowSizeHint::RowOffsets` is avoided in favor of just
storing the found length in bytes. This is in particular useful for
small table updates as statistically, the fewer rows, the more chance of
the lengths being all equal. In the case of a single row, the chance is
notably 100%. It is also good for the case of when we don't have
`RelValue::Row` or `Row::Ptr` but where the underlying table that
actually has a static layout.
In the future, we might want to avoid these lists in incremental as
well.
# Benchmarks
Benchmark numbers vs. master using `cargo bench --bench subscription --
--baseline subs` on i7-7700K, 64GB RAM:
```
footprint-scan time: [28.731 ms 28.924 ms 29.171 ms]
change: [-49.728% -49.006% -48.388%] (p = 0.00 < 0.05)
Performance has improved.
```
Performance goes from roughly 56.721 ms to 28.795 ms.
# API and ABI breaking changes
None
# Expected complexity level and risk
2, fairly local change to just subscriptions.
# Testing
Covered by existing tests.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 18, 2025
# Description of Changes Fixes #2824. Defines a global pool `BsatnRowListBuilderPool` which reclaims the buffers of a `ServerMessage<BsatnFormat>` and which is then used when building new `ServerMessage<BsatnFormat>`s. Notes: 1. The new pool `BsatnRowListBuilderPool` reports the same kind of metrics to prometheus as `PagePool` does. 2. `BsatnRowListBuilder` now works in terms of `BytesMut`. 3. The trait method `fn to_bsatn_extend` is redefined to be capable of dealing with `BytesMut` as well as `Vec<u8>`. 4. A trait `ConsumeEachBuffer` is defined from `ServerMessage<BsatnFormat>` and down to extract buffers. `<ServerMessage<_> as ConsumeEachBuffer>::consume_each_buffer(...)` is then called in `messages::serialize(...)` just after bsatn-encoding the entire message and before any compression is done. This is the place where the pool reclaims buffers. # Benchmarks Benchmark numbers vs. master using `cargo bench --bench subscription -- --baseline subs` on i7-7700K, 64GB RAM: ``` footprint-scan time: [21.607 ms 21.873 ms 22.187 ms] change: [-62.090% -61.438% -60.787%] (p = 0.00 < 0.05) Performance has improved. full-scan time: [22.185 ms 22.245 ms 22.324 ms] change: [-36.884% -36.497% -36.166%] (p = 0.00 < 0.05) Performance has improved. ``` The improvements in `footprint-scan` are mostly thanks to #2918, but 7 ms of the improvements here are thanks to the pool. The improvements to `full-scan` should be only thanks to the pool. # API and ABI breaking changes None # Expected complexity level and risk 2? # Testing - Tests for `Pool<T>` also apply to `BsatnRowListBuilderPool`.
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
4 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.
Description of Changes
Avoid building a temporary
Vecinexecute_planby exposing a list-building interface instead.fn encode_listis rewritten in terms of this list-building interface.BsatnRowListandBsatnRowListBuildertypes are split into two entirely separate types. The latter now tries to recognize the case where there isn't a known static layout, but where the BSATN lengths happen to be the same for all rows anyways. In those cases, the allocation ofRowSizeHint::RowOffsetsis avoided in favor of just storing the found length in bytes. This is in particular useful for small table updates as statistically, the fewer rows, the more chance of the lengths being all equal. In the case of a single row, the chance is notably 100%. It is also good for the case of when we don't haveRelValue::RoworRow::Ptrbut where the underlying table that actually has a static layout.In the future, we might want to avoid these lists in incremental as well.
Benchmarks
Benchmark numbers vs. master using
cargo bench --bench subscription -- --baseline subson i7-7700K, 64GB RAM:Performance goes from roughly 56.721 ms to 28.795 ms.
API and ABI breaking changes
None
Expected complexity level and risk
2, fairly local change to just subscriptions.
Testing
Covered by existing tests.