andygrove commented on code in PR #6098:
URL: https://github.com/apache/datafusion-comet/pull/6098#discussion_r4088299423
##########
native/shuffle/src/ipc.rs:
##########
@@ -46,11 +46,25 @@ const CONTINUATION_MARKER: [u8; 4] = [0xff; 4];
/// from several shuffles, and a single entry would thrash.
const SCHEMA_CACHE_CAPACITY: usize = 4;
+/// Maximum estimated serialized-plus-parsed size of all cached schemas on a
thread, excluding
+/// allocator overhead. A shared 4 MiB budget keeps retention modest while
allowing wide schemas
+/// to use space left by other entries: 8,000 short-named Int32 fields need
about 1.4 MiB with
+/// Arrow 59 on a 64-bit target. Both field count and names/metadata
contribute to this estimate.
+const SCHEMA_CACHE_RETAIN_LIMIT: usize = 4 << 20;
Review Comment:
The shared budget fixes the single wide schema case. One schema is now
cached up to about 23,600 short-named `Int32` columns. But I think it moves the
cliff for interleaved shuffles rather than removing it. Because the 4 MiB is
shared, wide schemas get fewer effective slots than `SCHEMA_CACHE_CAPACITY`.
When a thread cycles through more of them than fit, LRU evicts each one just
before it comes around again and nothing hits. `pull_input_batches` in
`jni_api.rs` pulls the next block from every shuffle scan on the task thread in
turn, so a native join over two or three wide shuffles is exactly that cycle.
I decoded blocks round robin for 10 rounds. Three 8,000-column schemas,
about 1.36 MiB each, got 0 hits and 30 misses, where `main` gets 27 hits. Two
12,000-column schemas got 0 hits and 20 misses, where `main` gets 18. Your
benchmark puts a miss at 5.27 ms against 2.30 ms warm for 8,000 columns
uncompressed, so those stages decode as though #5809 never landed. With four
shuffles the cutoff is about 1 MiB per schema, which is roughly the 5,800
columns we discussed on the first revision.
Would you consider raising the budget so that four schemas at the width the
doc comment uses as its example still fit, 16 MiB for instance? That keeps the
worst case bounded at 16 MiB per decoding thread, where `main` has no bound at
all. Whichever number you pick, could you add a test next to
`wide_schemas_hit_the_cache_and_reset_releases_them` that interleaves three or
four 8,000-column schemas and checks they hit after the first round?
##########
native/shuffle/benches/shuffle_reader.rs:
##########
@@ -132,6 +132,12 @@ fn criterion_benchmark(c: &mut Criterion) {
}
}
+ // Wide schemas exceed 1 MiB when both serialized and parsed metadata
are counted.
Review Comment:
This still refers to the first revision's 1 MiB per-entry limit. The only 1
MiB constant left is `SCRATCH_RETAIN_LIMIT`, and this schema's serialized
message is about 416 KB, so it doesn't reach that either. Could the comment say
what the arm guards now, that 8,000 columns is about 1.4 MiB of the cache
budget?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
