execute_plan: don't build temporary vec of rows
#2918
Conversation
Centril
force-pushed
the
centril/execute-plan-no-temp-vecs
branch
from
54e7434 to
b45b193
Compare
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.
|
I would like to review this before it merges.
In particular I may ask that we do this. |
bfops
added
release-any
I think we should split |
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.
# 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.
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.