[VQueues] Remove use of vobj_status on invoke path#4740
Conversation
AhmedSoliman
force-pushed
the
pr4740
branch
3 times, most recently
from
b419140 to
e557a1b
Compare
AhmedSoliman
changed the title
AhmedSoliman
force-pushed
the
pr4740
branch
4 times, most recently
from
c05822f to
31b234a
Compare
…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.
```
Throughput of "invoke" workload improves 9k/s -> ~11k/s (the relative numbers are what matters here) The assumption is that we will perform a one-shot migration of data in this table that has a value as part of the VQ migration pack.
![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: 86a4dbe67c
ℹ️ 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".
| let mut update = <vqueue_table::metadata::Update as bilrost::encoding::RawMessage>::empty(); | ||
| for op in operands { | ||
| let batch = match VQueueMetaUpdates::decode(op) { | ||
| Err(err) => { | ||
| let key = MetaKey::deserialize_from(&mut key); | ||
| error!( | ||
| ?err, | ||
| ?key, | ||
| "[full merge] Failed to decode vqueue meta batched updates ({} bytes)", | ||
| op.len(), | ||
| ); | ||
| return None; | ||
| } | ||
| Ok(batch) => batch, | ||
| }; | ||
| for update in batch.updates.iter() { | ||
| vqueue_meta.apply_update(update); | ||
| if let Err(err) = update.replace_from_slice(op) { | ||
| let key = MetaKey::deserialize_from(&mut key); |
There was a problem hiding this comment.
full_merge_slices now decodes every merge operand as a single Update, but pre-upgrade data was written as VQueueMetaUpdates batches. During a rolling upgrade (or after restart with existing SST/memtable merge records), those legacy operands will not be interpreted correctly by this path, so queued metadata updates can be dropped or the merge can fail, leaving vqueue counters/stats inconsistent. Please add a compatibility decode path for the old operand format before applying updates.
Useful? React with 👍 / 👎.
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
LGTM. +1 for merging if the failing tests are unrelated.
2 participants
Throughput of "invoke" workload improves 9k/s -> ~11k/s (the relative numbers are what matters here)
The assumption is that we will perform a one-shot migration of data in this table that has a value
as part of the VQ migration pack.
Stack created with Sapling. Best reviewed with ReviewStack.