Repository navigation
Windows unbuffered model load - #26014
JTischbein wants to merge 3 commits into
Conversation
|
Hi @JTischbein, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
| LocalFree(lpMsgBuf); | ||
| } | ||
|
|
||
| while (!ret.empty() && (ret.back() == '\r' || ret.back() == '\n')) { |
There was a problem hiding this comment.
Could you elaborate why this is needed?
There was a problem hiding this comment.
The returned message from FormatMessageA often contains a line break at the end. With this change we avoid double line breaks when logging the error message
|
|
||
| impl(const char * fname, const char * mode, [[maybe_unused]] const bool use_direct_io = false) { | ||
| fp = ggml_fopen(fname, mode); | ||
| impl(const char * fname, const char * mode, const bool use_direct_io = false) : fname(fname) { |
There was a problem hiding this comment.
My 2 cents on this mr, is that it actually needs to be decomposed into separate classes. With an interface and multiple implementation classes. This allow you to keep things simple and seperate (mmap, direct_io, win_unbuffered, etc).
There was a problem hiding this comment.
I agree with you on that, for now I wanted to keep the changes minimal to validate whether unbuffered IO is welcome on WIndows and works on all systems.
Let me try to get a new version together
There was a problem hiding this comment.
I would prefer to make a separate PR for the abstraction of the paths. The required changes are quite large
ORippler
left a comment
There was a problem hiding this comment.
Thanks for bringing this feature to Windows as well!
- Regarding the refactor: nice to have, but beyond the scope of this PR imo - this PR aims to bring Windows to performance parity with Linux for the direct-io path
- What would be nice is some tests for file loading (both for Linux and Windows). Not sure how difficult they are to spin up and if they should be part of this PR.
- Do you have some perf numbers for dGPU systems you could share?
| void * raw_buffer = _aligned_malloc(bytes_to_read, alignment); | ||
| if (raw_buffer == nullptr) { | ||
| LLAMA_LOG_WARN("%s: Falling back to buffered I/O due to %s\n", | ||
| __func__, GetErrorMessageWin32(ERROR_NOT_ENOUGH_MEMORY).c_str()); | ||
| CloseHandle(fp_win32_direct); | ||
| fp_win32_direct = INVALID_HANDLE_VALUE; | ||
| alignment = 1; | ||
| seek(offset, SEEK_SET); | ||
| read_raw_unsafe(dest, len); | ||
| return; | ||
| } | ||
|
|
||
| struct aligned_buffer_deleter { | ||
| void operator()(void * p) const { _aligned_free(p); } | ||
| }; | ||
| std::unique_ptr<void, aligned_buffer_deleter> buffer(raw_buffer); |
There was a problem hiding this comment.
There is no need to materialize raw_buffer, just pass it into the unique_ptr directly
There was a problem hiding this comment.
Changed with the upcoming commit, thanks!
| } | ||
| const size_t bytes_to_read = (offset_from_alignment + len + alignment - 1) & ~(alignment - 1); | ||
|
|
||
| void * raw_buffer = _aligned_malloc(bytes_to_read, alignment); |
There was a problem hiding this comment.
I guess adding a chunked read is not advisable from a perf perspective? LM Head may be a couple of 100 MB in size (500 MB for Qwen3.6-35BA3B if we have 8 BPW)
There was a problem hiding this comment.
Actually we sometimes have regressing performance with larger read size, in my tests above 512MB (1GB compared to 512MB loads 16% slower). I will add chunked reads in the upcoming PR
There was a problem hiding this comment.
I can remember a 'trick' to deal with this more efficiently.
- first check if the buffer given is already aligned.
- If not, we read unaligned until we are at the aligned boundary.
- Handle the left over bytes as an aligned read.
That would avoid the memory allocation
|
Just FYI, no critic at all or anything like that: PR#26542 covers the "OVERLAPPED" and "Handle/Thread" part of this problem - both PRs combined, should solve this problem. Shared handle at depth 8: 1.01x Yours |
Yes, it shouldn't be difficult to unit-test the
Then we can run these tests on multiple devices on |
2cd2f5f to
a2923f6
Compare
|
We should add buffered fall-back path similar to #29749 for Windows also |
Testing
As the DirectIO change on Linux lead to many new issues with several backends and devices, I'd appreciate it if people could test this PR on different devices and let me know if it works.
Overview
Linux supported unbuffered reading from disk via DirectIO for fast model loading. On my machine with PCIe5.0 NVMe drive this path increases the model loading performance from 1-3GB/s to >9GB/s.
For Windows this change is needed, as using mmap of large models on setups with small CPU RAM lead to Sysmem pressure and eviction of runtime pages.
Additional information
This PR introduces the unbuffered read file API of Windows. #18012 is the merged Linux PR.
Requirements