persist: count compaction fast-path eligible reqs #18380
Conversation
| } | ||
| if let Some(single_nonempty_batch) = single_nonempty_batch { | ||
| if single_nonempty_batch.runs.len() == 0 | ||
| && single_nonempty_batch.desc.since() != &Antichain::from_elem(T::minimum()) |
There was a problem hiding this comment.
technically we can only stop compacting it when the since is past the upper (otherwise we might get some consolidation from forwarding the timestamps), but this is probably close enough to give us a signal
There was a problem hiding this comment.
I'm not sure we'd see that in practice, because the upper of the output will also get bumped each time through, right?
There was a problem hiding this comment.
not necessarily. like the upper of the shard could be at 7 and we could end up compacting [0,2) and [2,4)
There was a problem hiding this comment.
IIRC the most common case of this was a mostly-empty shard that looks like [0, 1) (with data), followed by empty [1, 2), [2, 3), ... progress batches that didn't get the empty-batch shortcut. so each time compaction fires, it gets an input of all the batches, and so in practice the upper would advance each time
There was a problem hiding this comment.
yeah, good point. I don't think it would be correct to use the logic you have here to trigger the optimization, but it's certainly safe to use as an upper bound on the potential benefit of the technique
There was a problem hiding this comment.
that's fair -- I mostly want a ballpark on whether fast-path compactions are like, 1%, 10%, 50% of our writes. I'm related-ly curious about TimelyDataflow/differential-dataflow#277 which seems like it'd help address your point here without having to compact a batch that will never benefit from logical compaction
There was a problem hiding this comment.
ha, I was about to point you at that PR! last time I took Frank's temperature on it, he was pretty hesitant to make any scary changes to the DD Spine, but I think it's a pretty straightforward cherry-pick to apply it to our fork
Seeing how much compaction work / S3 PUTs we could shave off if we reintroduce the compaction fast path for single non-empty batches (with 1 run and were written by compaction)
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.