nxdk
nxdk copied to clipboard
XboxDev
Rework winapi's QueryPerformance* functions to match XDK
We aren't following XDK with the current implementation.
QueryPerformanceFrequency is a hardcoded value of 733MHz
In later XDK revisions they used rdtsc for QueryPerformanceCounter
I don't agree with hard coded values, but didn't want to force a ~100mS sleep on developers wishing to use the function so I locked dynamic frequency behind a define.
235dde3 produces a number that is only 0.000454501% off the static value on hardware. With a slightly longer sleep we can get another digit of accuracy but I'm happy where it is.
The 1kHz tick rate with a 200ms sleep means that even the most optimistic error calculation results in a ±0.5% approximation error. Combined with the numerically disadvantageous division that is several orders of magnitude worse precision than what we had before.
It's 0.0004% off on hardware with an extra digit of accuracy by waiting 250mS longer.
That's 0.0004% coming from only the last step in your calculation, and arguing that you can get lucky with the measurements doesn't change that this code has ±0.5% approximation error, is numerically badly conditioned and orders of magnitude less precise than what we already have.
Latest commit brings us down from 0.0004% to 0.0001%, with an initial run value of 733332000. Subsequent calls on prime bring this to 733333000.
I do want to leave this as well. My motivations for this are Microsoft specifically moved away from using KeQueryPerformance* for these functions directly after the initial launch of the Xbox. 3824 uses it, 4361 does not.
I have to imagine there's some reason for this.
I feel like this PR wasted far too much reviewer time already and it's time for a "final word". To summarize, the issues are:
- excessive start-up delay
- imprecise timing source (tick counter) which introduces a ±0.5% error when measuring with a 200ms interval
- numerical issues due to how the calculation is done
- compile-time switch
Doubling down on the decisions behind these flaws, or, as you call it, "justifying", will not change any of this. If the issues above are not an issue to you than that is fine and you can keep the code around in your own fork, but with them present this PR will not get merged. If you are unwilling to fix them - or want to continue to ignore the parts of the review that made you aware of them - then I suggest you close this PR and let it serve as a source of information for someone who is willing to put his effort into a solution that doesn't have these problems.
Here are some suggestions of things that could get merged:
- A PR with just the
rdtscchange. Not necessarily because it's the way the XDK does it, but because it's an objectively better timing source than what we have now (should be around ~209 times more precise) - A PR that calculates the timer frequency from FSB frequency and multipliers and stores it in a global variable - if it matches the hardcoded value we have on a stock Xbox (or is maybe even more precise). This will support configurations that are outside the Xbox's spec, so this is not a necessity nor a priority for nxdk, but rather something that is nice to have. I don't expect such a change to have the negative performance impact that a runtime-modifiable NV base address would have.
Whether something with a ~100ms or less start-up delay should get merged is still debatable, especially since its only benefit would be supporting systems that - intentionally or not - run outside of the specs of an Xbox.
The main focus of nxdk is supporting retail Xbox systems, like the XDK did. If we can extend that support to things the XDK did not support, without any major drawbacks (that includes maintainability!), then why not. But adding a hack that benefits only people that run their Xbox outside of what the XDK supported and has very obvious problems is not an option for nxdk.