[Perf] Support fixed encoding for vqueue and other common types#4749
Conversation
AhmedSoliman
force-pushed
the
pr4749
branch
3 times, most recently
from
70e958b to
39c8862
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39c88627ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| &entry_id.to_invocation_id(qid.partition_key()) | ||
| .expect("This version does not support yielding vqueues entries other than invocations"), |
There was a problem hiding this comment.
This expect assumes every yielded vqueue entry is an invocation, but scheduler yields are emitted for any running head entry on leader change, and running entries can be EntryKind::StateMutation as well. In that case to_invocation_id(...) returns None and this path panics, taking down command application for an otherwise recoverable leader-change yield. Please branch by entry kind (or keep a generic yield path) instead of hard-failing here.
Useful? React with 👍 / 👎.
| }; | ||
| for update in batch.updates.iter() { | ||
| vqueue_meta.apply_update(update); | ||
| if let Err(err) = update.replace_from_slice(op) { |
There was a problem hiding this comment.
The merge operator now decodes every operand as a single Update, but prior code wrote operands wrapped in VQueueMetaUpdates. During restart/rolling upgrade, existing RocksDB merge operands in the old format can still be present; replace_from_slice(op) will fail on those bytes and full_merge returns None, turning normal merge/read/compaction into merge failures. Keep a compatibility decode path for the old wrapper format until legacy operands are drained.
Useful? React with 👍 / 👎.
AhmedSoliman
force-pushed
the
pr4749
branch
2 times, most recently
from
b7b88be to
c9c7644
Compare
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Great improvements @AhmedSoliman 🚀 LGTM. +1 for merging :-)
| #[bilrost(tag(12))] | ||
| pub(crate) avg_inbox_duration_ms: u64, |
There was a problem hiding this comment.
For the averages we are keeping the varint encodings because we assume that this value is usually small and that varint encoding pays off?
There was a problem hiding this comment.
I opted to make the stats cheaper to store to allow us to grow them without worrying too much about the number of them.
| Succeeded, | ||
| } | ||
|
|
||
| mod status_bilrost_encoding { |
There was a problem hiding this comment.
Why not just bilrost_encoding like the other places?
There was a problem hiding this comment.
I want to make the enum encoding fixed (not varint) for decode efficiency purposes.
muhamadazmy
left a comment
muhamadazmy
left a comment
There was a problem hiding this comment.
LGTM! I left couple of nits.
…l merges Partial merges didn't add value and wasted precious CPU cycles since the updates cannot be merged This change removes it and increase the limit of successive merges from 100 (way too low for vqueus) to 5000100 (way too low for vqueus) to 5000 which experimentally seems to be enough, but we may expose this as an advanced config option in the future if really needed.
Improves decoding throughput of successive vqueue meta merge updates by ~45% as shows in the benchmark:
```console
vqueue_meta_merge/full_merge_10000_operands
time: [200.41 µs 201.33 µs 202.22 µs]
thrpt: [49.451 Melem/s 49.670 Melem/s 49.897 Melem/s]
change:
time: [-29.998% -29.504% -28.958%] (p = 0.00 < 0.05)
thrpt: [+40.761% +41.852% +42.853%]
Performance has improved.
```
3 participants
Improves decoding throughput of successive vqueue meta merge updates by ~45% as shows in the benchmark:
Stack created with Sapling. Best reviewed with ReviewStack.