bulk: Compute MVCCStats of the SST being ingested on-the-fly #39724
Conversation
|
This change is |
pkg/storage/bulk/sst_batcher.go
Outdated
|
|
||
| // If we can't use the fast stats path, or race test is enabled, | ||
| // compute stats over the SST being ingested. | ||
| if !fast || util.RaceEnabled { |
There was a problem hiding this comment.
I was imagining we'd do this in cmd_add_sstable where we choose to use the client-provided stats vs out own instead of here so it'd run no matter who/how we sent an SST, but either way works.
pkg/storage/bulk/sst_batcher.go
Outdated
| // AddSSTable which has been identified as a significant performance | ||
| // regression for IMPORT. | ||
| if b.disallowShadowing { | ||
| b.updateMVCCStats(&key, &value) |
There was a problem hiding this comment.
is escape analysis smart enough to recognize that taking these pointers don't need to move key and value to heap here? I don't know if we need to pass pointers?
pkg/storage/bulk/sst_batcher.go
Outdated
| @@ -134,6 +169,7 @@ func (b *SSTBatcher) Reset() error { | |||
| b.batchEndValue = b.batchEndValue[:0] | |||
| b.flushKey = nil | |||
| b.flushKeyChecked = false | |||
| b.ms = nil | |||
There was a problem hiding this comment.
b.ms.Reset() will save you re-allocating it for next SST.
pkg/storage/bulk/sst_batcher.go
Outdated
| @@ -85,6 +90,26 @@ func MakeSSTBatcher(ctx context.Context, db sender, flushBytes int64) (*SSTBatch | |||
| return b, err | |||
| } | |||
|
|
|||
| // | |||
pkg/storage/bulk/sst_batcher.go
Outdated
| // | ||
| func (b *SSTBatcher) updateMVCCStats(key *engine.MVCCKey, value *[]byte) { | ||
| if b.ms == nil { | ||
| b.ms = &enginepb.MVCCStats{} |
adityamaru27
force-pushed
the
mvccstats-opt
branch
from
589a448
to
fbe5f5d
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt and @nvanbenschoten)
pkg/storage/bulk/sst_batcher.go, line 93 at r1 (raw file):
Previously, dt (David Taylor) wrote…
stray
//
Done.
pkg/storage/bulk/sst_batcher.go, line 96 at r1 (raw file):
Previously, dt (David Taylor) wrote…
2¢: I'd de-pointer
b.ms.
Done.
pkg/storage/bulk/sst_batcher.go, line 157 at r1 (raw file):
Previously, dt (David Taylor) wrote…
is escape analysis smart enough to recognize that taking these pointers don't need to move key and value to heap here? I don't know if we need to pass pointers?
Oh, I didn't know about this, read up more and seems like it gives escape analysis an easier time if passed by value.
pkg/storage/bulk/sst_batcher.go, line 172 at r1 (raw file):
Previously, dt (David Taylor) wrote…
b.ms.Reset()will save you re-allocating it for next SST.
Done.
pkg/storage/bulk/sst_batcher.go, line 290 at r1 (raw file):
Previously, dt (David Taylor) wrote…
I was imagining we'd do this in cmd_add_sstable where we choose to use the client-provided stats vs out own instead of here so it'd run no matter who/how we sent an SST, but either way works.
You're right I was having a hard time choosing which approach to go with. I've reworked it a bit, with the assumption that we only want to verify it when the SST has come with disallowShadowing set to true.
There was a problem hiding this comment.
Reviewed 4 of 7 files at r1, 3 of 4 files at r2.
Reviewable status:
pkg/storage/batcheval/cmd_add_sstable.go, line 156 at r2 (raw file):
// remove the ContainsEstimates flag) after they are done ingesting files by // sending an explicit recompute. stats.ContainsEstimates = !args.DisallowShadowing
Explain why these two conditions are linked.
pkg/storage/bulk/sst_batcher.go, line 157 at r1 (raw file):
Previously, adityamaru27 (Aditya Maru) wrote…
Oh, I didn't know about this, read up more and seems like it gives escape analysis an easier time if passed by value.
Yeah there's no reason to pass these by reference, although it would have been smart enough to avoid letting either arg escape.
adityamaru27
force-pushed
the
mvccstats-opt
branch
from
fbe5f5d
to
94953bc
Compare
This change is an optimization to the MVCCStats collection in the bulk ingestion pipeline. Currently when ingesting an SST via the SSTBatcher, we have one iteration to construct an SST, and an additional one to compute the MVCCStats for the span being ingested. In scenarios such as IMPORT, where we have an enforced guarantee (via the disallowShadowing flag) that the KVs being ingested do not shadow existing data, MVCCStats collection becomes very simple. This change adds logic to collect these stats on-the-fly while the SST is being constructed, thereby saving us an additional iteration which has been profiled as a bottleneck in IMPORT. There is a significant performance win to be achieved by ensuring that the stats computed are not estimates as it prevents recompuation on splits. Running AddSSTable with disallowShadowing=true gets us close to this as we do not allow colliding keys to be ingested. However, in the situation that two SSTs have KV(s) which "perfectly" shadow an existing key (equal ts and value), we do not consider this a collision. While the KV would just overwrite the existing data, the stats would be re-added, causing a double count for such KVs. One solution is to compute the stats for these "skipped" KVs on-the-fly while checking for the collision condition and returning their stats. The final stats would then be base_stats + sst_stats - skipped_stats, and this would be accurate. Release note: None
adityamaru27
force-pushed
the
mvccstats-opt
branch
from
94953bc
to
2a13ce6
Compare
|
bors r+ |
39724: bulk: Compute MVCCStats of the SST being ingested on-the-fly r=adityamaru27 a=adityamaru27 This change is an optimization to the MVCCStats collection in the bulk ingestion pipeline. Currently when ingesting an SST via the SSTBatcher, we have one iteration to construct an SST, and an additional one to compute the MVCCStats for the span being ingested. In scenarios such as IMPORT, where we have an enforced guarantee (via the disallowShadowing flag) that the KVs being ingested do not shadow existing data, MVCCStats collection becomes very simple. This change adds logic to collect these stats on-the-fly while the SST is being constructed, thereby saving us an additional iteration which has been profiled as a bottleneck in IMPORT. TODO: There is a significant performance win to be achieved by ensuring that the stats computed are not estimates as it prevents recompuation on splits. Running AddSSTable with disallowShadowing=true gets us close to this as we do not allow colliding keys to be ingested. However, in the situation that two SSTs have KV(s) which "perfectly" shadow an existing key (equal ts and value), we do not consider this a collision. While the KV would just overwrite the existing data, the stats would be re-added, causing a double count for such KVs. One solution is to compute the stats for these "skipped" KVs on-the-fly while checking for the collision condition and returning their stats. The final stats would then be base_stats + sst_stats - skipped_stats, and this would be accurate. Benchmark update: Over three runs of TPCC 1k on a 4 node, default roachprod cluster, the time dropped from ~32m to ~22m. Release note: None Co-authored-by: Aditya Maru <adityamaru@cockroachlabs.com>
Build succeeded |
|
Tagging #34809 |
This change is an optimization to the MVCCStats collection
in the bulk ingestion pipeline. Currently when ingesting an
SST via the SSTBatcher, we have one iteration to construct an
SST, and an additional one to compute the MVCCStats for the
span being ingested.
In scenarios such as IMPORT, where we have an enforced guarantee
(via the disallowShadowing flag) that the KVs being ingested
do not shadow existing data, MVCCStats collection becomes very
simple. This change adds logic to collect these stats on-the-fly
while the SST is being constructed, thereby saving us an additional
iteration which has been profiled as a bottleneck in IMPORT.
TODO:
There is a significant performance win to be achieved by
ensuring that the stats computed are not estimates as it prevents
recompuation on splits. Running AddSSTable with disallowShadowing=true gets
us close to this as we do not allow colliding keys to be ingested. However,
in the situation that two SSTs have KV(s) which "perfectly" shadow an
existing key (equal ts and value), we do not consider this a collision.
While the KV would just overwrite the existing data, the stats would be
re-added, causing a double count for such KVs. One solution is to
compute the stats for these "skipped" KVs on-the-fly while checking for the
collision condition and returning their stats. The final stats would then
be base_stats + sst_stats - skipped_stats, and this would be accurate.
Benchmark update: Over three runs of TPCC 1k on a 4 node, default roachprod cluster, the time dropped from ~32m to ~22m.
Release note: None