Repository navigation
fix(libkernel): use precise waits for select timeouts - #4754
Conversation
|
Local validation and measured timeout waits for this PR. Publication base: Windows used the full project CMake configuration, the original supported compiler/binutils and the pinned bundled dependency sources. Only libc/libkernel and the relevant test targets were built. Linux used the actual upstream libc/libkernel CMake subdirectories in a local validation wrapper. A complete build of all targets, graphics tests and game FPS were not run. The Windows UDP timeout also occurred in the earlier unchanged-main comparison. Local build and loaded-library paths are redacted from the public logs below; all timing samples, test results and library hashes are preserved. Windows publication-base test outputLinux publication-base test outputThe official convention checker passes on the committed branch with the PR title/body; Before/after wait measurementsMeasurement base:
The figures summarize elapsed waits through the guest ABI in the compiled PRX library; they are not FPS measurements or hard scheduling guarantees. Compared library SHA-256 hashes{
"baseline": {
"libkernel.prx": "5d435c4f6c7da210a2762a540c4dead3e27c3b14a3ecaf396b8eca0b42f4ea41",
"libc.prx": "fac53d34db8259861fe28b7264f226c21d2436ad075edcf70193b672f6a2c81d"
},
"patched": {
"libkernel.prx": "721220ce05a6c261590923e48e2061a8c6cc2c30e25a3bf6112113523e177bb4",
"libc.prx": "fac53d34db8259861fe28b7264f226c21d2436ad075edcf70193b672f6a2c81d"
}
}Probe source used for the measurements#include "prx/libc/include/general/VabiMacros.hpp"
#include <algorithm>
#include <chrono>
#include <cstdint>
#include <cstdio>
#include <vector>
#ifdef _WIN32
#include <windows.h>
#endif
extern "C" int APS5_VABI select_nid_postfix(int, void*, void*, void*, const void*);
struct Timeval { std::int64_t seconds; std::int64_t microseconds; };
int main() {
#ifdef _WIN32
wchar_t kernelPath[32768]{};
if (!GetModuleFileNameW(GetModuleHandleW(L"libkernel.prx"), kernelPath, 32768)) return 2;
char utf8Path[131072]{};
if (!WideCharToMultiByte(CP_UTF8, 0, kernelPath, -1, utf8Path, sizeof(utf8Path), nullptr, nullptr)) return 3;
std::printf("loaded_kernel=%s\n", utf8Path);
#endif
using Clock = std::chrono::steady_clock;
for (const std::int64_t micros : {0, 50, 500, 1000, 2000, 16667}) {
std::vector<double> samples;
const Timeval limit{0, micros};
for (int trial = 0; trial < 41; ++trial) {
const auto begin = Clock::now();
const int result = select_nid_postfix(0, nullptr, nullptr, nullptr, &limit);
const auto elapsed = Clock::now() - begin;
if (result != 0 || elapsed < std::chrono::microseconds(micros)) return 1;
if (trial != 0) samples.push_back(std::chrono::duration<double, std::micro>(elapsed).count());
}
std::sort(samples.begin(), samples.end());
std::printf("timeout_us=%lld median_us=%.3f p90_us=%.3f min_us=%.3f max_us=%.3f\n",
static_cast<long long>(micros), samples[20], samples[36], samples.front(), samples.back());
std::printf("samples_us=%lld", static_cast<long long>(micros));
for (const auto value : samples) std::printf(" %.3f", value);
std::puts("");
}
}Pair 1: baseline, raw samples and loaded libraryPair 1: patched, raw samples and loaded libraryPair 2: patched, raw samples and loaded libraryPair 2: baseline, raw samples and loaded libraryPair 3: baseline, raw samples and loaded libraryPair 3: patched, raw samples and loaded libraryPair 4: patched, raw samples and loaded libraryPair 4: baseline, raw samples and loaded libraryPair 5: baseline, raw samples and loaded libraryPair 5: patched, raw samples and loaded libraryPair 6: patched, raw samples and loaded libraryPair 6: baseline, raw samples and loaded libraryPair 7: baseline, raw samples and loaded libraryPair 7: patched, raw samples and loaded libraryPair 8: patched, raw samples and loaded libraryPair 8: baseline, raw samples and loaded libraryPair 9: baseline, raw samples and loaded libraryPair 9: patched, raw samples and loaded library |
aee3971 to
8b296a8
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the PR. The change itself looks right (wait capped at the guest deadline, only the groups.empty() path).
One blocking issue:
core/libs/tests/GuestSelectWait.cpppasses without the fix. I revertedSocket.cpptomainandguest_select_waitstill passes. Every wait check only asserts the call does not return before the deadline (Clock::now() - start >= micros); nothing bounds how late it returns, so the oldsleep_for(which only overshoots) satisfies it too. Please add a repeated short wait (e.g. N empty-set selects of 1 ms) and assert the average/total stays well below the coarse ~15.6 ms Windows tick, as theguest_pollcheck in #4980 does, so the test fails onmainon Windows.
The conflict with #5013 is only adjacent lines in libkernel/CMakeLists.txt and Socket.cpp; whoever merges second rebases.
8b296a8 to
c6fe305
Compare
|
@adriadam10 addressed in Windows 11, Ryzen 7 5800H, WinLibs MinGW-w64 GCC 15.2.0 r7, Release, on
On Linux (Ubuntu 26.04 under WSL, GCC 15.2.0) the new check passes with and without the fix, because the host wait is already precise there. To reproduce the control on this head: Wait per call on Windows, median of 40 calls of
The type is now Windows control: Socket.cpp from main, new test (3 of 3 fail)Windows with the fix: guest_select_wait, 20 repeatsWindows with the fix: socket and wait tests (8 of 9; guest_udp reaches its existing 20 s limit)Linux with the fix: socket and wait tests (10 of 10)Linux control: Socket.cpp from main, new test (5 of 5 pass)Windows probe output with the fixWindows probe output with Socket.cpp from mainProbe source#include "prx/libc/include/general/VabiMacros.hpp"
#include <algorithm>
#include <chrono>
#include <cstdint>
#include <cstdio>
#include <vector>
#ifdef _WIN32
#include <windows.h>
#endif
extern "C" int APS5_VABI select_nid_postfix(int, void*, void*, void*, const void*);
struct Timeval { std::int64_t seconds; std::int64_t microseconds; };
int main() {
#ifdef _WIN32
wchar_t kernelPath[32768]{};
if (!GetModuleFileNameW(GetModuleHandleW(L"libkernel.prx"), kernelPath, 32768)) return 2;
char utf8Path[131072]{};
if (!WideCharToMultiByte(CP_UTF8, 0, kernelPath, -1, utf8Path, sizeof(utf8Path), nullptr, nullptr)) return 3;
std::printf("loaded_kernel=%s\n", utf8Path);
#endif
using Clock = std::chrono::steady_clock;
for (const std::int64_t micros : {0, 50, 500, 1000, 2000, 16667}) {
std::vector<double> samples;
const Timeval limit{0, micros};
for (int trial = 0; trial < 41; ++trial) {
const auto begin = Clock::now();
const int result = select_nid_postfix(0, nullptr, nullptr, nullptr, &limit);
const auto elapsed = Clock::now() - begin;
if (result != 0 || elapsed < std::chrono::microseconds(micros)) return 1;
if (trial != 0) samples.push_back(std::chrono::duration<double, std::micro>(elapsed).count());
}
std::sort(samples.begin(), samples.end());
std::printf("timeout_us=%lld median_us=%.3f p90_us=%.3f min_us=%.3f max_us=%.3f\n",
static_cast<long long>(micros), samples[20], samples[36], samples.front(), samples.back());
std::printf("samples_us=%lld", static_cast<long long>(micros));
for (const auto value : samples) std::printf(" %.3f", value);
std::puts("");
}
}AI-assisted (Claude Code). |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c6fe305 to
6e9fbc0
Compare
|
@p3n1cillin the PR overlap report lists #5013 against this PR. A trial
Neither change builds on the other. Proposed order: this PR first, since it is older and already reviewed, and #5013 rebases with that resolution and lists this PR as its dependency, so the report stops listing the pair. If you prefer the opposite order, say so and I will list #5013 here and rebase after it. AI-assisted (Claude Code). |
Addressed in 6e9fbc0: the new 20 x 1 ms check fails on main on Windows (author 3/3) and passes with the fix; full build green.
What
Use the existing
PreciseSleepUntilhelper for the timeout-only path of guestselect(), capping each wait at the guest deadline. After the descriptor-group refactor inmain, this path isgroups.empty(). Descriptor dispatch, socket/host pollers, readiness reporting and error handling are preserved. No user configuration, dependencies or additional runtime tools are required.Rebased onto
mainat61b8742b6b4508db14e8d253e60cfaa7d105379b. The patch is three files, 85 additions and 2 deletions.guest_select_waitnow also runs twenty empty-set selects of 1 ms and requires their total to stay between 20 ms and 200 ms, as theguest_pollcheck in #4980 does forpoll(). The earlier checks only required that a call did not return before its deadline, so they passed without the fix.Related:
poll()path inUdp.cpp; this PR changesselect()inSocket.cpp.libkernel/CMakeLists.txtandSocket.cpp. Neither builds on the other; as noted in the review, whichever merges second rebases.Tested
Windows 11, Ryzen 7 5800H, WinLibs MinGW-w64 GCC 15.2.0 r7, Release; Ubuntu 26.04 under WSL, GCC 15.2.0, Release. Head
6e9fbc0519a41dc58779dab878d38064b8d825e2onmain61b8742b6. Raw test output, per-call wait measurements and the probe source are in this comment.Socket.cppfrommainand the new test,guest_select_waitfails 3 of 3 runs withtwenty 1 ms selects did not stay close to their deadlines. With the fix it passes 21 of 21 runs.guest_udpreached its existing 20 s limit. The earlier published baseline/patched comparison also recorded this Windows timeout. The limit was not raised.Socket.cppfrommain, because the host wait is already precise on Linux; it is a Windows regression check.guest_select_waitcovers 20 scenarios: zero/short deadlines, the repeated 1 ms wait, invalid arguments, unready/ready devices, unchanged timeout and descriptor bits abovenfds.git diff --checkpassed.Earlier measurements from the first publication base remain in the earlier evidence comment.
Not tested: macOS, retail titles, game FPS, a complete build of all project targets, or the full graphics suite. This corrects the wait duration of the call; no game-scene speed change is claimed and no performance traces were captured. Only the three implementation/test files belong to this PR; local validation scripts, snapshots and logs are not committed.
Checklist
main; no equivalent open PR implements thisselect()timeout-wait change