[dbsp] Windows compatibility#6671
Conversation
Replaced the Unix-only pread implementation with platform-specific positioned reads. Unix uses FileExt::read_at, while Windows duplicates an overlapped file handle once per operation and reads with ReadFile/GetOverlappedResult. Unlike previous version of this code (abandoned PR), So it opens one duplicated, overlapped handle for the full read_exact_at request and reuses it through every loop iteration. Added regression tests for preserving the file cursor and for Windows overlapped reads. Added the Windows-only windows-sys dependency.
Limit nix to Unix targets and use std::process::id for the profiling marker process ID. Generate non-Unix timestamps from a shared process-relative Instant origin. Convert externally supplied Instant span starts using that same origin, keeping with_start markers aligned with captured timestamps. Add a regression test for the conversion.
Use GetThreadTimes to measure current-thread CPU time on Windows and retain the existing CLOCK_THREAD_CPUTIME_ID implementation on Unix. Make DBSP storage work on Windows with a reusable overlapped read handle per reader, portable file-length APIs, flushed directory metadata, and `LockFileEx` directory locks keyed by volume serial number and file index. Keep Unix positioned reads, fcntl locking, resource use, and runtime behavior unchanged. Use more portable functions (e.g. len() instead of size() in a few places) Add Windows-only tests for lock identities across hard links, and run the column-layer benchmark without Unix-only flamegraph profiling.
mythical-fred
left a comment
There was a problem hiding this comment.
This is the reworked version of #6627, with the requirements I'd flagged before addressed:
- Squashed and cfg-gated cleanly (unix vs windows dependencies, no leftover
nixin[dependencies]). - Every new
unsafeblock carries aSAFETY:comment naming the invariants. posixio_implcaches a per-readerReOpenFileoverlapped handle (reader_file→Arc<File>) and reuses it across all reads instead of re-opening per call.fbuf.rs::read_exact_atrefactored into a sharedread_exact_at_withloop with a Windows overlapped variant.dirlockuses a stable identity from volume serial + file index; thelock_id_is_shared_by_hard_linkstest proves the equivalence class.TIMESTAMP_ORIGINOnceLockgives a shared process-relative clock for the non-UnixInstantconversion, and the new regression test pins the clock choice.ThreadCpuTimefactored intocurrent_thread_cpu_time()with an explicit Windows impl and a monotonic regression test.
One observation for a follow-up (not blocking): the non-Unix Timestamp::From<Instant> panics on macOS builds if TIMESTAMP_ORIGIN is initialized by that path before the macOS Timestamp::now picks its own origin. Currently guarded by #[cfg(not(unix))], so macOS is not affected in this PR. Worth a doc line explaining the TIMESTAMP_ORIGIN invariant so a future refactor doesn't cross-wire the clocks.
LGTM.
|
|
||
| #[cfg(windows)] | ||
| { | ||
| let read_file = reopen_for_overlapped_read(file)?; |
There was a problem hiding this comment.
Windows callers in hot paths should reach for read_exact_at_overlapped with a cached FILE_FLAG_OVERLAPPED handle rather than this convenience method — this path calls reopen_for_overlapped_read on every invocation, which is a full ReOpenFile syscall per read. Consider a doc line here pointing hot-path callers at the overlapped variant (as posixio_impl already does).
|
I spent a little time looking at the state of reading from a given file position on Windows. I think that it is easy: you just use seek_read. The only apparent difference from read_at is that it changes the file position. I think that's OK, we don't care about the file position. Did you try that? |
|
Is the clock that this uses for profiling annotations the correct one? That is, when you run a profile, do the annotations have the correct times? |
|
on first comment - i didn' try that because my goal is to make it behave like linux for safety reasons. I don't fully understand all the upstream/downstream components, so I'm no taking any liberties with compatibility. That also saves me from having to read code up and down the tree. But if you are confident changing file position is okay, I can change it. Or we can do a later optimization. on second comment - my goal is to keep my changes only for DBSP crate, so I am not familiar how to run a profile and I am not even compiling the entire platform since then I probably need more changes. I stopped at openssl blockers earlier, and if I were to implement it on windows, I'd want either support for the native libraries instead of openssl, or just use rustls across the board if it works. Anyway, my understanding is that these are "elapsed" times. I think where it matters is if you are aligning annotations to another clock source. |
read_exact_at has only two callers, and those are ones where it's OK to change the file position. Please change it. And update the doc comment on the function to note that it might or might not change the file position.
These timestamps are exactly for alignment with another clock source, which is whatever clock source samply uses on Windows. If you just use some randomly chosen clock source then the profile will look wrong. |
|
On #1, it's one of those where perfect is enemy of good, but you're the boss. It's not as simple as just the pointer. seek_read is not thread safe since it's implemented as two calls, (seek+read), and therefore race-prone. read_at is thread safe -- it converts more or less directly into a ReadFile(). So there are other side-effects at play. If you insist I change it, fine. I don't quite grasp the threading model yet, especially when crate is used in arbitrary apps. So I guess you are saying you will be comparing performance numbers in different clock domains. Ok, i'll have to mull this over sometime, probably need to use QueryUnbiasedInterruptTimePrecise(). Don't know yet. |
I don't think it's true that it is implemented as two calls. It looks to me like it ends up here: https://github.com/rust-lang/rust/blob/main/library/std/src/sys/pal/windows/handle.rs#L247. That code uses the single NtReadFile call to do the read. The documentation for that function at https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntreadfile says that specifying ByteOffset gives an atomic seek-and-read effect.
The right thing to do is to find out how samply does it on Windows, then do it the same way. |
|
You are right, i think the thread issue for seek_read was fixed in rust-lang/rust#81357. As long as you don't use Read::read or Seek::seek calls on the same handle somewhere else in the code, which would cause the file pointer to jump around unpredictably. |
We don't do that; we only do this kind of fixed-offset read in the files where this is used. |
This PR only updates the DBSP crate and its dependencies. It does not guarantee the entire monorepo compiles under windows. Every attempt was made to not affect linux path.
Summary of changes (please see individual commit for more):
Add platform-specific positioned reads to feldera-storage: retain Unix read_at, add Windows overlapped ReadFile/GetOverlappedResult, preserve the caller’s file cursor, and reuse one duplicated handle per read_exact_at operation.
Add regression coverage for positioned reads, including cursor preservation and Windows overlapped reads, plus local safety documentation for Windows FFI and handle ownership.
Make Samply profiling annotations portable to Windows: gate nix to Unix, use std::process::id, and derive non-Unix timestamps from a shared process-relative Instantorigin.
Keep externally supplied profiling span start times aligned with the timestamp clock, with a regression test for the conversion.
Add Windows thread CPU accounting in DBSP through GetThreadTimes while retaining CLOCK_THREAD_CPUTIME_ID on linux.
Make DBSP file storage work on Windows with reusable overlapped reader handles, portable metadata-length APIs, directory metadata flushing, and LockFileEx directory locks keyed by stable file identity.
Preserve the existing linux DBSP storage behavior and resource use: direct positioned reads, fcntl locking, linux cache flags, and linux benchmark profiling remain unchanged.
Add Windows coverage for lock identities across hard links and retain the column-layer benchmark on non-Unix platforms without Unix-only flamegraph profiling.
Unit tests that apply to linux path also apply to windows path, and where missing, unit tests were added.
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes