persist: "packed" encodings for Interval and Time
#27336
Merged
+279
−4
Conversation
This file contains 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
5 tasks
antiguru
reviewed
May 29, 2024
src/repr/benches/packed.rs
Outdated
| const INTERVAL: Interval = Interval::new(1, 1, 0); | ||
| group.bench_function("encode", |b| { | ||
| b.iter(|| { | ||
| let packed = PackedInterval::from(INTERVAL); |
There was a problem hiding this comment.
I might be totally off, but don't you need to black_box INTERVAL, too? Otherwise the optimizer might just constant-fold the expresion?
There was a problem hiding this comment.
That totally makes sense, the benchmarks also seemed too fast and the numbers look more reasonable now, thanks!
bkirwi
approved these changes
May 29, 2024
| /// Interprets a slice of bytes as a [`PackedNaiveTime`]. | ||
| /// | ||
| /// Returns an error if the size of the slice is incorrect. | ||
| pub fn from_bytes(slice: &[u8]) -> Result<Self, String> { |
There was a problem hiding this comment.
This could be implemented a bit more directly via try_into: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b3c429d443727dbe68bc28817f9aea9d
I wasn't sure how I felt about this leaving data only partially validated, but I think it's fine to panic at read time. Hard to imagine we'd ever do anything else in production...
There was a problem hiding this comment.
Ahhh nice! updated to use try_into
5 tasks
ParkMyCar
added a commit
that referenced
this pull request
May 30, 2024
…e` and `MzAclType` (#27360) This PR does a few things: 1. Introduces a `ColumnarCodec` trait that is defines how to encode a type `T` so it can be durably persisted in an `arrow::array::FixedSizeBinaryArray`. 2. Refactors the impls of `PackedInterval` and `PackedNaiveTime` (introduced in #27336) to use `ColumnarCodec`. 3. Add `PackedAclItem` and `PackedMzAclItem` which implement `ColumnarCodec` 4. Refactor existing and add more benchmarks to measure the throughput of encoding and decoding for all of the "packed" types vs the existing `ProtoDatum` types. #### Benchmarks The existing benchmarks have been reworked a bit to include encoding into an existing buffer and reading from a slice. Also included are benchmarks for the same workflow but using the existing protobuf types, since the motivation for this work is to improve throughput compared to protobuf. type | encode | decode -------------------|------------------------|-------- interval/packed | ~1,172 mil/second | ~1,000 mil/second interval/proto | ~239.3 mil/second | ~120.4 mil/second time/packed | ~722.1 mil/second | ~1,038 mil/second time/proto | ~336.9 mil/second | ~187.3 mil/second acl_item/packed | ~1,050 mil/second | ~1,033 mil/second acl_item/proto | ~117.7 mil/second | ~56.5 mil/second mz_acl_item/packed | ~385.6 mil/second | ~590.0 mil/second mz_acl_item/proto | ~58.9 mil/second | ~28.5 mil/second In general, the "packed" representations have a 5x - 20x higher throughput than their protobuf alternatives. FWIW I believe the packed representations are so much faster because protobuf encodes integers with LEB128 and encodes fields one at a time. Whereas the packed representations use a bit more memory but encode integers in their true size and encodes and entire type "all at once". ### Motivation Progress towards https://github.com/MaterializeInc/materialize/issues/24830 ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [x] This PR includes the following [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note): - N/a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR implements "packed" encodings for
IntervalandTimethat will be used for writing structured data in Persist.The packed encodings are designed to be as fast as possible and have the same sort order as the original types. They're based on discussion from #26175. I added benchmarks that measure throughput and locally I get the following results:
I believe the implementations can be optimized further with SIMD, but right now that requires
inline_asm!or theNightlycompiler.Note: The code placement feels a bit weird since these impls should only be used by Persist, so I was thinking of putting the impl behind a trait like
mz_persist_types::ColumnarCodec, but was curious what other folks thought.Motivation
Progress towards https://github.com/MaterializeInc/database-issues/issues/7411
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.