Refactor commit encoding / decoding #495
Merged
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
Use `sats::{BufReader, BufWriter}` for decoding / encoding of `Commit`
and associated types. This makes `decode` fallible (which is quite
desirable, instead of panicking).
As the `DecodeError` from sats is fairly sparse, also add some context
about where exactly decoding failed.
Lastly, add some documentation.
kim
commented
Oct 30, 2023
kulakowski
approved these changes
Oct 30, 2023
|
|
||
| pub fn datakey() -> impl Strategy<Value = DataKey> { | ||
| prop_oneof![ | ||
| prop::collection::vec(any::<u8>(), 0..31).prop_map(DataKey::from_data), |
There was a problem hiding this comment.
I'd comment on the reason for the two ranges.
| pub struct Write { | ||
| pub operation: Operation, | ||
| pub set_id: u32, | ||
| pub set_id: u32, // aka table id | ||
| #[cfg_attr(test, proptest(strategy = "arbitrary::datakey()"))] |
There was a problem hiding this comment.
I'd also comment about directing this to a hand-written strategy, or in general anywhere that the derive macro doesn't do the right thing on its own.
There was a problem hiding this comment.
Is it fine to have those on the mod arbitrary intentionally kept close the to type definition? See ea62cf8
| #[cfg(test)] | ||
| use proptest_derive::Arbitrary; | ||
|
|
||
| /// A commit is one record in the write-ahead log. |
There was a problem hiding this comment.
A drive by thought: similarly to how we have a bunch of code that says set_id but commented "this is the table id", I would love to settle on saying "write ahead log" everywhere, including data type names etc. Or some other name. But let's pick one!
There was a problem hiding this comment.
Yeah, I'd love to settle on WAL / write ahead log, maybe even collapsing CommitLog and MessageLog into a single type. Possibly not for this PR, though?
There was a problem hiding this comment.
Yeah I'd love to have us try to take things that way. I agree on the naming, I think the commit log and message log names are implementation details and that "write ahead log" is the name that reflects the semantics the datastore needs to care about. But yeah not for this PR.
There was a problem hiding this comment.
Just adding my two cents here. I agree on coalescing on the name. I avoided WAL because historically that's referred to a log that is eventually compressed/deleted, but I think on balance WAL is probably the best name. We can include a comment about the fact that the WAL should never be deleted.
We should also note that it will include information which is not normally in a WAL, including reducer call event info and that it forms a Merkle DAG between commits.
There was a problem hiding this comment.
The term MessageLog was borrowed from Kafka.
There was a problem hiding this comment.
Noted to include all of these in the upcoming formalization / design changes doc.
Hm, would this entail landing a separate patch with a benchmark before this, so we can track the difference? |
I don't think so. I meant regression at the level of in benchmarks / game performance, not in any new functionality or anything. Like if worldgen goes from N seconds to 1.1N seconds, I'd want to look at this again, but if it stays at N seconds, send it. |
4 tasks
|
benchmarks please |
|
Let's try this :) |
Benchmark resultsBenchmark ReportLegend:
All throughputs are single-threaded. Empty transaction
Single-row insertions
Multi-row insertions
Full table iterate
Find unique key
Filter
Serialize
Module: invoke with large arguments
Module: print bulk
Remaining benchmarks
|
Looks like it was a flake. |
4 tasks
Centril
reviewed
Oct 30, 2023
Comment on lines
114
to
116
| let mut count = 0; | ||
|
|
||
| if self.parent_commit_hash.is_none() { | ||
| count += 1; | ||
| } else { | ||
| count += 1; | ||
| count += self.parent_commit_hash.unwrap().data.len(); | ||
| count += 1; // tag for option |
Comment on lines
+32
to
+33
| // [`DataKey`] is defined in `lib`, so we can't have an [`Arbitrary`] impl | ||
| // for it just yet due to orphan rules. |
There was a problem hiding this comment.
What I'd do in this situation is to move the impl to that crate and expose the impl under a proptest feature that can be enabled in dev-dependencies.
There was a problem hiding this comment.
Sure. I tried to keep changes local for this one, but happy to send a follow-up defining Arbitrary for Hash and DataKey. Perhaps that'd even increase probability for folks to write property tests ;)
cloutiertyler
previously requested changes
Oct 30, 2023
| // 1 for flags, 4 for set_id | ||
| let mut count = 1 + 4; | ||
| let mut count = self.operation.encoded_len(); | ||
| count += 4; // set_id |
There was a problem hiding this comment.
This seems wrong. Shouldn't it be 5 based on the operation encoding?
There was a problem hiding this comment.
It’s operation.len + 4 = 1+4 = 5, like before no?
| @@ -91,21 +131,36 @@ impl Commit { | |||
| count | |||
| } | |||
|
|
|||
| pub fn encode(&self, bytes: &mut Vec<u8>) { | |||
| bytes.reserve(self.encoded_len()); | |||
There was a problem hiding this comment.
It's a little unfortunate that we're losing this optimization, but maybe we can improve things by not using a Vec at all but a buffer pool or something in the future.
There was a problem hiding this comment.
Good point. I meant to make sure the caller takes care of this, will add.
Also comment on the scope of proptest_config.
4 tasks
kim
dismissed
cloutiertyler’s stale review
It was not a bug, and I added a test to assure it's not.
kulakowski
pushed a commit
that referenced
this pull request
Nov 7, 2023
* core: Refactor commit encoding / decoding
Use `sats::{BufReader, BufWriter}` for decoding / encoding of `Commit`
and associated types. This makes `decode` fallible (which is quite
desirable, instead of panicking).
As the `DecodeError` from sats is fairly sparse, also add some context
about where exactly decoding failed.
Lastly, add some documentation and (property) tests.
kulakowski
pushed a commit
that referenced
this pull request
Nov 7, 2023
* core: Refactor commit encoding / decoding
Use `sats::{BufReader, BufWriter}` for decoding / encoding of `Commit`
and associated types. This makes `decode` fallible (which is quite
desirable, instead of panicking).
As the `DecodeError` from sats is fairly sparse, also add some context
about where exactly decoding failed.
Lastly, add some documentation and (property) tests.
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.
Description of Changes
Use
sats::{BufReader, BufWriter}for decoding / encoding ofCommitand associated types. This makes
decodefallible (which is quitedesirable, instead of panicking).
As the
DecodeErrorfrom sats is fairly sparse, also add some contextabout where exactly decoding failed.
Also add documentation and property test.
API and ABI
If the API is breaking, please state below what will break
Expected complexity level and risk
1