Feature/io uring - #224
Conversation
|
Thanks for taking the time to put this together — io_uring is genuinely interesting territory for a disk-IO-heavy app like aMule, and the contribution is appreciated. That said, the PR as it stands is not in shape to merge: it would silently corrupt downloads, breaks the build for anyone not setting an undeclared option, and bundles three behavior changes wholly unrelated to io_uring. I'll walk through each in priority order so it's clear what would need to change. Silent corruption in the io_uring pathThe hot-path implementation in FileAutoClose.cpp:340-341 returns Related, the same two functions call Build-breaking change for everyoneCMakeLists.txt wraps Undocumented scope creepBeyond io_uring the PR also smuggles in:
Single-platform featureaMule is cross-platform (Linux, macOS, Windows, *BSD). io_uring is Linux-only — not a hard barrier, optional features behind Perf claims — methodology pleaseThe PR description says "drastically improve disk I/O performance during uploads and downloads", but the implementation does Could you share:
Without that it's hard to reconcile the claimed gain with what the code actually does. Style / packagingA few smaller items to clean up regardless:
Suggested next stepThis is an I/O optimisation PR — please remove everything that isn't io_uring from it. Specifically: drop the |
|
Thank you so much for the incredibly thorough and constructive review! After carefully reading your feedback and re-evaluating the implementation, I realize you are completely right. Since this code uses Furthermore, retrofitting a true asynchronous, batched I/O model into aMule's current disk path would require a massive architectural rewrite of the core. As you wisely pointed out, if an effort of that magnitude is going to be undertaken, it makes much more sense to build it around a I'm closing this PR to avoid polluting the codebase with a single-platform feature that doesn't provide tangible async benefits yet. I really appreciate the time you took to point out the silent corruption flaws and the scope creep—it's been a great learning experience. Thanks again for steering me in the right direction! |
This PR introduces native
io_uringsupport for Linux systems to drastically improve disk I/O performance during uploads and downloads.The implementation uses a
thread_localring approach (ThreadLocalRing), allowing the disk I/O threads to reuse a persistentio_uringinstance. This eliminates the massive CPU and syscall overhead of initializing and destroying a ring on every single read/write operation.The feature is integrated natively using Modern CMake (
PkgConfig::LIBURINGwithIMPORTED_TARGET) and safely wrapped in#ifdef USE_IO_URING. This keeps cross-platform compilation fully intact and avoids polluting the global CMake namespace.