Add portable random-access file reads to Env - #32503
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Windows high-offset behavior remains untested, requiring additional coverage and human validation.
Pull request overview
Adds a portable, owned random-access file abstraction with stable file identity and thread-safe positional reads.
Changes:
- Introduces
RandomAccessFileandEnv::OpenRandomAccessFile. - Implements POSIX
preadand Windows overlapped reads. - Adds cross-platform behavior, concurrency, and edge-case tests.
File summaries
| File | Description |
|---|---|
onnxruntime/test/platform/env_test.cc |
Adds platform tests, but lacks Windows coverage for reads above 4 GiB. |
onnxruntime/core/platform/windows/env.h |
Declares the Windows factory override. |
onnxruntime/core/platform/windows/env.cc |
Implements overlapped positional reads. |
onnxruntime/core/platform/posix/env.cc |
Implements descriptor-backed reads using pread. |
onnxruntime/core/platform/env.h |
Defines the new interface and default unsupported implementation. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Resolve open inside a lambda before passing it to TempFailureRetry so Android's fortified overloads do not prevent callable type deduction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use std::thread with scoped joins on libc++ versions without jthread. Exercise atomic replacement of an open Windows file with FileRenameInfoEx POSIX semantics instead of legacy rename, without loosening production sharing restrictions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fixed the next two CI issues in b4a5d03:
The minimal Android job still failed before any binary-size measurement, so no threshold adjustment was made. All 12 targeted Linux platform tests passed; native Windows/Android/macOS validation will come from the new CI runs. |
CI now completes compilation and measures 1,587,406 bytes of ELF sections, exceeding the old 1,585,152-byte budget by 2,254 bytes. Raise the active workflow threshold by 4 KiB to 1,589,248 bytes, retaining 1,842 bytes of headroom without disabling the size check or dropping the new platform API from minimal builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest Android minimal-baseline job now finishes compilation and reaches the actual size check. It reports 1,587,406 bytes of ELF sections versus the 1,585,152-byte limit: a real 2,254-byte overage (the on-disk file size is not the checked metric). Commit 6788a30 raises the active Android workflow budget by 4 KiB to 1,589,248 bytes, leaving 1,842 bytes of headroom for the added random-access file support. The size check remains enabled and minimal builds retain the new API. Earlier failures were compilation failures; this is the first measured size failure on this PR. |
Qualify the helper call from PosixEnv to avoid member-name hiding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Request Windows 10 RS1 declarations before including Windows headers in the platform test, exposing FileRenameInfoEx without changing production build targets. Enable the sparse-file test on 64-bit Windows, mark the file sparse before extending it, and reopen the reader after writing to honor its no-write-sharing contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The current Windows/QNN failures are compilation errors: FileRenameInfoEx is hidden by the SDK unless NTDDI_VERSION is at least NTDDI_WIN10_RS1. Commit fb98d57 requests those declarations before any Windows headers, scoped only to env_test.cc; production targets and sharing rules are unchanged. This fixes the missing declaration rather than replacing the enum with a magic integer. The same commit addresses the outstanding review comment by enabling the >4 GiB sparse-file read test on 64-bit Windows. All 12 Linux platform tests passed; the Windows branches will be exercised by the new CI runs. |
Replace the ineffective NTDDI override with the documented, typed FileRenameInfoEx ABI selector. On Android, skip only FIFO creation denied with EACCES or EPERM, matching the SELinux denials in both emulator jobs; retain failures for all other creation errors and keep native FIFO coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The NTDDI-only change in the previous commit was insufficient: Windows CI still did not expose the FileRenameInfoEx enumerator. Commit 13da6e1 removes that override and uses a local typed constant for the documented FILE_INFO_BY_HANDLE_CLASS ABI value (22), retaining the same POSIX replacement operation. Reference: https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ne-minwinbase-file_info_by_handle_class Both Android runtime failures are now diagnosed as SELinux setup restrictions, not a failed reader: logcat reports avc: denied { create } ... tclass=fifo_file for the test filename. The FIFO test skips on Android only if mkfifo fails with EACCES or EPERM, with an explicit reason. Other errors remain failures, and the FIFO behavior still executes on Linux. The >4 GiB test remains enabled on Windows. All 12 targeted Linux platform tests passed. New CI is required to confirm the Windows and Android branches. |
There was a problem hiding this comment.
🟢 Approval recommended
The implementations satisfy the documented ownership, concurrency, range-validation, and file-identity contracts with strong platform coverage.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Ti-Tai Wang (titaiwangms)
left a comment
There was a problem hiding this comment.
Review: approved
I reviewed ae562b5cf0..13da6e1ca, including the public contract, POSIX and Windows implementations, concurrent reads, integer and EOF handling, file-identity semantics, platform compatibility, binary-size impact, and test coverage.
The core implementation is sound. POSIX uses one descriptor with pread and handles interrupted and short reads correctly. Windows uses independent per-call OVERLAPPED state and events, correctly handles immediate and pending completion, drains failed asynchronous operations before stack-owned state is released, and preserves 64-bit offsets. Both implementations preserve output parameters on failure. The tests cover concurrency, invalid ranges, partial EOF, pathname replacement, platform-specific truncation behavior, special files, and reads above 4 GiB.
Non-blocking follow-ups:
- Clarify in the API documentation that Windows denies ordinary concurrent writes and may reject ordinary replacement operations while the handle is open, whereas POSIX does not prevent in-place mutation.
- Rename the POSIX free helper to
GetFileLengthFromDescriptorto avoid shadowing the member overload. - The Windows directory-attribute query can be removed because
CreateFile2is called withoutFILE_FLAG_BACKUP_SEMANTICS; alternatively, use theExAPI if an explicit check is retained. - Consider normalizing
ERROR_HANDLE_EOFto the same explicit unexpected-EOF diagnostic used on POSIX.
This provides the correct abstraction for #32437. That CUDA loader must still be migrated to open each external-data file once and retain this object across its length query and all parallel reads before its stable-file-identity issue is resolved.
Description
Fixes #32500.
Add
Env::OpenRandomAccessFile()and an ownedRandomAccessFileinterface withGetLength()and synchronous, thread-safe positionalRead()operations. All operations use the same open file identity rather than reopening its pathname for each range.pread, retries interrupted operations, handles short reads, and rejects non-regular files without blocking on FIFOs.ReadFileIntoBufferremains available and unchanged. The new Env virtual is appended, and its default implementation explicitly returns NOT_IMPLEMENTED so custom environments are not silently bypassed.This is not protection against in-place file modification under POSIX, or a snapshot across multiple files. Callers must retain the object for the entire sequence of reads that needs a stable identity and keep it alive until all readers finish.
Scope
This PR is based directly on main. It provides the platform abstraction requested during #32437; migration of that CUDA loader is not included. Loader lifecycle cleanup is tracked separately in #32502.
Platform compatibility
Android exposes fortified overloads of
open, which preventedTempFailureRetryfrom deducing its callable type and caused all four failing Android CI jobs to stop at compilation. The call toopenis now wrapped in a lambda, preserving the fortified call and EINTR retry behavior. After the compilation fixes, Android minimal-baseline CI measured 1,587,406 bytes of ELF sections. The active workflow budget is now 1,589,248 bytes (a 4 KiB increase), leaving 1,842 bytes of headroom while keeping the size check enabled.The concurrent-read tests use
std::threadwith scope-bound joins for Android and older libc++ implementations that lackstd::jthread. On Windows, the atomic replacement test usesFileRenameInfoExwith POSIX replacement semantics because legacy filesystem rename rejects an open destination; the production write-sharing restriction is unchanged.Coverage
Adds platform tests for concurrent reads, file size, invalid ranges, EOF after a partial read, empty files, failed-open output preservation, the default unsupported implementation, atomic pathname replacement, platform-specific truncation behavior, FIFO rejection, and sparse files beyond 4 GiB on 64-bit POSIX and Windows. Existing path-based reads and length queries are also exercised.
Validation
All 12 selected Linux CPU tests passed:
Windows code and Windows-specific test branches require Windows CI; no Windows compiler/runtime was available locally.
The local CPU build has contrib ops disabled and encountered an existing unrelated unused-function warning in nhwc_transformer_test.cc. Only that translation unit was compiled with -Wno-error=unused-function; no unrelated source or global warning settings were changed.