Polar Signals Profiling Results

Latest Run

Powered by Polar Signals Cloud

Benchmarks: Vortex queries 📖

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +0.7%
Engines: DataFusion No clear signal (+8.4%, medium confidence) · DuckDB No clear signal (-6.5%, low confidence)
Vortex (geomean): 0.946x ➖
Parquet (geomean): 0.943x ➖
Shifts: Parquet (control) -5.7% · Median polish -5.1%

No file size changes detected.

@claude review

Claude finished @0ax1's task in 3m 25s —— View job

Claude PR Review — Direct I/O for local CUDA file scans

• Gather context (read changed files)
• Review FFI direct I/O changes (ffi/src/lib.rs, vortex_cuda.h)
• Review pooled_read_at.rs direct I/O implementation
• Review file.rs / pinned.rs changes
• Post review feedback

Overall this is a clean, careful implementation. The alignment math is correct, the FFI surface is additive and ABI-safe, error handling avoids panics on the I/O path, and the Linux/non-Linux split is handled consistently. Below are findings ordered by importance — none are blocking, mostly hardening and questions.

Correctness — looks solid

I traced the alignment path end-to-end and it holds up:

• direct_io_range widens [offset, offset+length) down to the aligned read_offset, up to a block-aligned read_length, and returns the prefix..requested_end slice. All arithmetic is overflow-checked. The unit tests cover the representative cases.
• read_direct_at always passes an offset-aligned buffer length (buffer[filled..] with filled proven aligned) at an aligned file offset, so every pread satisfies O_DIRECT constraints. The short-read-past-EOF case terminates correctly because required_bytes ≤ file_size − read_offset for any valid request. The mid-stream unaligned-short-read guard is a nice defensive touch.
• Memory alignment is guaranteed in practice: alloc_pinned (cudaHostAlloc) returns page-aligned memory (4096), which satisfies stx_dio_mem_align (typically 512/4096). The runtime address check backstops this.

1. Device-buffer element alignment after .slice(prefix..) — worth a comment

read_byte_range returns BufferHandle::new_device(...).slice(requested_range). In the buffered path the data starts at the (page-aligned) device allocation base; in the direct path it starts at base + prefix, where prefix = offset % offset_align.

This preserves the segment's natural alignment only if the on-disk segment alignment A is a power-of-two divisor of the block alignment (then prefix is a multiple of A, and base + prefix stays A-aligned). That invariant holds for Vortex today, but it's load-bearing and non-obvious. Consider a short comment at the .slice(...) call noting that the widened read preserves segment alignment because segment offsets divide the DIO block size. If any consumer ever assumes a stronger alignment than the segment's, this would silently break.

2. Hard failure when dio_mem_align > 4096

If a device reports a memory alignment larger than the pinned allocation's page alignment, every data-plane read fails with InvalidInput and the scan is dead in the water — a confusing failure mode for the caller who just set direct_io = true. This is rare, but since the goal of the statx/DIOALIGN probe is graceful degradation, it'd be more robust to either fall back to buffered reads (or return a clear "device requires N-byte alignment, unsupported" error at open_direct time rather than per-read). At minimum, worth documenting the assumption that pinned buffers are page-aligned.

3. Fallback alignment may be insufficient on old kernels

When statx doesn't report DIOALIGN (kernels < 6.1), the code falls back to FALLBACK_DIRECT_IO_ALIGNMENT = 4096. On a device whose true logical block is larger than 4096 this fallback would produce EINVAL on read. 4096 is a reasonable default and the common case, but a one-line comment explaining why 4096 (page size, covers virtually all real devices) would help future readers weigh the tradeoff.

4. Small inefficiency: full-read_length transfer for tiny reads

Each direct read transfers up to ~2 × alignment − 1 extra bytes to the GPU (the widened prefix + tail) and the device buffer retains the full read_length allocation even though only the sub-slice is used. Immaterial for large data segments (the intended target), but if read_byte_range is ever called for many small ranges under direct I/O, this amplifies both H2D traffic and resident device memory. Not worth changing now — just flagging the characteristic.

5. Test coverage

The pure direct_io_range logic is unit-tested, but read_direct_at (the short-read/alignment-guard loop) and the widen→read→truncate→slice round-trip are not exercised. A full integration test needs Linux + CUDA + an O_DIRECT-capable FS, which is hard in CI — understandable. If feasible, a host-only test of the loop logic (factoring the read loop to take a read_at closure over a plain File opened without O_DIRECT) would cover the trickiest untested code cheaply.

Minor / nits

• direct_io_range: the first offset.checked_add(length_u64)? result is discarded; the subsequent prefix + length and + alignment - 1 checks already catch overflow, so this line is redundant (harmless).
• read_byte_range: for length == 0 with an unaligned offset the direct path reads a full aligned block just to return an empty slice. Edge case, likely never hit, but a length == 0 early return would avoid it.
• The non-Linux vortex_ensure!(!direct_io, ...) in read_byte_range is unreachable because open_direct already bails on non-Linux — fine as defense-in-depth.

Style / repo conventions

• New public APIs (with_direct_io, open_direct, vx_cuda_scan_options, the new FFI fn) all carry doc comments ✅ and FFI functions have # Safety sections ✅.
• Tests return VortexResult<()> and use ? ✅, and live in a tests module ✅.

I could not run cargo clippy/cargo +nightly fmt --check -p vortex-cuda in this review environment (the crate requires the CUDA toolchain to build). Please make sure those pass locally before merge, per CLAUDE.md.
· branch os/o-direct