perf(table): deduplicate conflict-validation manifest reads

[fallintoplace]

What changed

• Cache parsed manifest-list descriptors for one conflict-validation context.
• Reuse completed manifest reads by path across validators and snapshots.
• Keep the first scan streaming and do not retain early exits.
• Add shared-read and early-exit correctness coverage.
• Add a benchmark that reports backend opens and bytes fetched.

Why

A commit can run more than one conflict validator against the same conflictContext. Each pass used to reopen the same manifest list and manifest file, which added extra object-store reads.

The cache only lives for one validation attempt. Manifest entry decoding still runs with each descriptor, so inheritance behavior stays unchanged.

Benchmark

Run on an Apple M1 Pro with:

go test ./table -run '^$' -bench '^BenchmarkConflictValidationSharedManifestReads/(entries=(1000|10000))/(validators=(2|8))$' -benchmem -benchtime=1s -count=5

These are medians over five runs. Before is upstream/main at c9813324. The backend metrics are per commit attempt. Local wall time is mostly Avro decoding and is more noisy.

Workload	Before	After
1K entries / 2 validators	4 opens, 16,160 B, 21,302 allocs	2 opens, 8,080 B, 19,954 allocs
1K entries / 8 validators	16 opens, 64,640 B, 85,214 allocs	2 opens, 8,080 B, 75,718 allocs
10K entries / 2 validators	4 opens, 63,976 B, 148,189 allocs	2 opens, 31,988 B, 146,843 allocs
10K entries / 8 validators	16 opens, 255,904 B, 592,759 allocs	2 opens, 31,988 B, 583,261 allocs

Testing

• go test ./table -count=1
• go test ./table -race -count=1
• go vet ./table
• go test ./... -run '^$'

[laskoviymishka]

The caching design is solid where it counts: the per-descriptor inheritance still runs on every logical read, so the cached-bytes layer is behavior-neutral and I don't see any way it changes what the validators decide. The commit-only-on-EOF idea for not caching early exits is a nice touch too.

I'd hold this before merging though. My main concern is the memory trade-off, which the PR doesn't call out: the old snap.entries path streamed and GC'd one manifest at a time, so peak manifest bytes on the heap was O(one file). This version keeps every fully-read manifest in conflictManifestIO.files for the life of the context, with no bound or eviction, so peak becomes O(all manifests read during the attempt). validateDataFilesExist walks every data manifest of the head snapshot, and under the concurrent-commit load this is meant to help, that's the exact case where the footprint grows most. I'd either document that bound and argue it's acceptable, or add a max-bytes fallback to uncached reads.

The other thing I'd want settled is that the "each manifest opened once" benefit only actually holds for the full-read path. validateDataFilesExist early-exits as soon as it finds the referenced files, which abandons the iterator so that manifest is never cached, and the next validator re-opens it. The benchmark loops a single validator, so it only measures the clean case; the realistic RowDelta composition (validateDataFilesExist then validateAddedDataFilesMatchingFilter) is neither benchmarked nor tested.

A few things I'd want before merge:

• Bound or explicitly document the cache's memory growth.
• Pass c.fs (not the recording IO) to snap.Manifests in manifestsFor; the manifest-list bytes are cached but never reused.
• Firm up the "record only on EOF" invariant so it doesn't depend on Read returning (0, io.EOF) separately, and drop the errors.Is for a plain == while there.
• Add a test and sub-benchmark for the mixed-validator / early-exit path so the opened-once claim is covered.

Once those are addressed, happy to take another pass.

[laskoviymishka]

This flips the peak memory profile and the PR doesn't call it out. The old snap.entries path streamed and GC'd one manifest at a time, so peak manifest bytes on the heap was O(one file). Here every fully-read manifest stays in files for the life of the context, with no bound or eviction, so peak becomes O(all manifests read during the attempt).

validateDataFilesExist walks every data manifest of the head snapshot, and under the concurrent-commit load this optimization is meant to help, that's exactly the case where the footprint grows most: 1000 manifests at 100KB is 100MB held per attempt, times N in-flight attempts.

I'd either document that bound and argue it's acceptable (the manifests were going to be read anyway; only the simultaneity is new), or add a max-bytes guard that falls back to uncached reads past a threshold. wdyt?

[laskoviymishka]

There are no compile-time guards that these satisfy the IO/File interfaces, so if iceio.File ever grows a method the break surfaces at the distant Open call site rather than here.

var (
	_ iceio.IO   = (*conflictManifestIO)(nil)
	_ iceio.File = (*conflictManifestFile)(nil)
	_ iceio.File = (*conflictManifestRecordingFile)(nil)
)

[laskoviymishka]

A read-through cache that forwards Remove to the backing store can delete a committed manifest, which is supposed to be immutable. No caller hits it today (only Open is used), but it's a sharp edge the moment this IO gets wired into anything with a cleanup path.

I'd drop the c.base.Remove forwarding and return an unsupported error instead. Thoughts?

[laskoviymishka]

The "record only on EOF so early exits don't cache" contract is derived from EOF-signal timing, not enforced. The io.Reader contract explicitly permits returning (n, io.EOF) in one call, and if the backing reader ever does that (or drains the file before the decoder yields entries), complete flips true and an abandoned iteration still caches the bytes.

TestConflictValidationDoesNotCacheEarlyManifestExit only passes because local-FS Read happens to return (n, nil) then (0, io.EOF) separately, so the test proves less than it claims. A different FS, buffer size, or OS could silently break the stated invariant. I'd tie completeness to a known size from Stat (or make the decision explicit) rather than to when EOF arrives.

Separately, since the reader contract forbids wrapping io.EOF, errors.Is here is just a slower err == io.EOF on a per-Read hot path. wdyt?

[laskoviymishka]

The manifest-list bytes recorded here are never reused. manifestsFor caches the parsed []iceberg.ManifestFile and short-circuits on it, so every later call returns from the parse cache without re-reading the path, so the raw list bytes just sit in files for the life of the context.

I'd pass c.fs here instead of c.manifestReadIO(). Only the data/delete manifest Avro files get read by multiple validators and actually benefit from the byte cache; the manifest list is read exactly once per snapshot.

[laskoviymishka]

This early return abandons the manifest iteration, so mf.Entries closes with complete=false and the manifest never lands in the cache. validateDataFilesExist usually runs first in a RowDelta and hits this exit in the first manifest with a small needed set, then validateAddedDataFilesMatchingFilter re-opens the same manifest from the backing store.

So the "each manifest opened once" benefit only holds on full-read paths, not this common one. I'd either read the rest of the current manifest to EOF before returning, or note the limitation in the comment. wdyt?

[laskoviymishka]

The benchmark loops the same validator, so it only measures the full-read path where the cache pays off cleanly; that's where the 16->2 opens number comes from. The realistic RowDelta shape (validateDataFilesExist with an early exit, then validateAddedDataFilesMatchingFilter) is exactly the case where a manifest gets re-opened, and it's neither benchmarked nor tested.

A sub-benchmark for that composition, plus a test that runs validateDataFilesExist then validateAddedDataFilesMatchingFilter on one context and asserts data manifests open at most once, would keep the opened-once claim honest. wdyt?

[laskoviymishka]

Good to land!

[laskoviymishka]

@fallintoplace conflicts need to be resolved