Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework winapi's QueryPerformance* functions to match XDK #663

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Rework winapi's QueryPerformance* functions to match XDK #663

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ab3a9f5
winapi: Use rdtsc for QueryPerformanceCounter
GXTX Oct 16, 2023
64f6505
winapi: Follow XDK with QueryPerformanceFrequency by hardcoding 733MHz
GXTX Oct 16, 2023
3faa575
winapi: Allow dynamic calculation of CPU frequency for QueryPerforman…
GXTX Oct 16, 2023
235dde3
winapi: Prime QueryPerformanceFrequency instead of calculating freq e…
GXTX Oct 17, 2023
9a603f7
winapi: Squash me
GXTX Oct 17, 2023
8ea50ce
winapi: More verbose private variable naming
GXTX Oct 18, 2023
eb4385b
winapi: Move to assembly
GXTX Oct 24, 2023
ef0e000
winapi: Less assembly, good enough(tm)
GXTX Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions lib/winapi/profiling.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,60 @@
// SPDX-FileCopyrightText: 2019 Stefan Schmidt

#include <profileapi.h>
#ifdef USE_RDTSC_FOR_FREQ
#include <synchapi.h>
#endif
#include <assert.h>
#include <stddef.h>
#include <xboxkrnl/xboxkrnl.h>

#ifdef USE_RDTSC_FOR_FREQ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't hide correct behavior behind a compile-time flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XDK only replies with 733MHz, so that would be "correct". As stated, I don't like hard numbers, but don't want to force wasted time on developers if they don't require it.

Copy link
Member

@JayFoxRox JayFoxRox Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't hide correct behavior behind a compile-time flag.

Is it correct though? I'd even consider rejecting this with "move it to a fork":

  • Added complexity
  • We can safely assume every single official Xbox will be running at 733MHz (and even if they don't, then it's up to the person who over-/underclocked to fix applications, because none of the MS code will work anymore either)

XDK only replies with 733MHz, so that would be "correct".

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can safely assume every single official Xbox will be running at 733MHz

It's assumptions like this that stop nxdk with pbkit running from functioning on my Xbox. There are applications now due to MIT licensing that I can't readily modify the source of, which means I need to hack the xbe.

Copy link
Member

@JayFoxRox JayFoxRox Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's assumptions like this that stop nxdk with pbkit running from functioning on my Xbox

Note that this is code in the winapi - so it's probably not mission critical.

If your Xbox drifts too far from 733.333MHz to crash applications then it's probably broken - it's not a fault of nxdk.
If you changed the clocks or the CPU (which I suspect), then it's no longer an Xbox and breaks many assumptions that both, MS XDK and the nxdk have (also the kernel probably.. which you probably had to patch then to even get your Xbox to boot).

It's assumptions like this ...

... which make the Xbox a gaming console instead of a generic PC.
nxdk winapi isn't a generic winapi and it's not a PC OS / environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a fault of nxdk

It's entirely nxdk's fault for killing pbkit if the frequency is not default. https://github.com/XboxDev/nxdk/blob/master/lib/pbkit/pbkit.c#L2812

Again bad assumptions about the box we're running on. I've tried multiple times to introduce similar things (NV base address being one of them) and at every turn I seem to hit this review bomb or ignore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's entirely nxdk's fault for killing pbkit if the frequency is not default

Yes: That particular check is bad and inprecise - it can easily cause issues and is totally avoidable.
And yes.. and that should be addressed.. but that's not what this PR is doing.

I've tried multiple times to introduce similar things (NV base address being one of them)

But modifying the NV base address is also not really reasonable. I also needed that for some of my projects.
However, I can see how it's not practical to do such invasive changes in nxdk.

I think one of the things we could do is to expose constants in some header (probably as macro, so they can be changed dynamically, potentially with a source marker so you know where the constant gets used from).
People could just reconfigure their nxdk if necessary then - however, that doesn't help with closed source apps.

However, I don't think it's practical to keep some of these hacks for niche system configurations or even niche software applications (like my attempt to emulate a virtual NIC from a DXT / xbe-loader) in the upstream nxdk codebase.
A lot of these changes quickly become incompatible with much more important (and required) changes.
Testing such niche applications is also a lot more difficult and therefore we'd be constantly introducing new bugs for those niche cases.

We'd also lose the major benefit of a gaming console (at least in the sense up to the 7th console generation or so): fixed hardware which allows massive simplifications and assumptions (which brings performance gains).
I think this is also a huge draw for some of the developers - Xbox being a fixed target makes it an interesting coding challenge and having a fixed spec brings some comfort.

xbox-linux and some of OpenXDK tried to turn Xbox into a PC.. that's also a noble goal, but I think that's a non-goal for nxdk. And even then, the xbox-linux Xbox platform likely has to make a bunch of assumptions.

and at every turn I seem to hit this review bomb or ignore.

I feel like this PR has gotten a lot more attention than it deserves.

There aren't many active maintainers (or outside reviewers) in XboxDev, but good changes still gets merged eventually.. occasionally I see merges in my GH notifications.

I'd prefer if some things got merged sooner than later, but this is how it is.
How many PRs did you review today? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes.. and that should be addressed..

It should, and I tried. Both an frequency accurate timer version, and a dumb "remove the kill code" version. Both were left stale and nitpicked.

I feel like this PR has gotten a lot more attention than it deserves.

We're all just giving our free time to see the change in the world that we envision. If you feel you're giving it more attention than you should, then I'm confused why you are.

How many PRs did you review today?

None. I don't do software development for work. Hopefully you had a good day at work today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're all just giving our free time to see the change in the world that we envision. If you feel you're giving it more attention than you should, then I'm confused why you are.

There's an assumption of good faith: One would expect to gain more than to lose on doing a review.

Even if there's a bad PR we still have to take the time to review and close it.
That's the cost of doing FOSS.

In turn, we hope that the author improves their code so that it might be eventually mergeable (hopefully giving us a return of invest). Or we hope that some author shows up with perfectly mergeable code (which is an instant win).

This PR in particular looked to become mergeable eventually:

  • it started out with good arguments
  • it had fatal design flaws, but it looked like you are willing to fix it (and learn)

However, then it stagnated quickly, leading to many discussions, rather than progress on the code.
The initial arguments faded into the background as some weird arguments came up like "bad caps" or "unstable clocks at XBE launch" which made little sense.. unless you aren't running a stock Xbox.

Both, @thrimbor and I tried to assist or nudge you in the right direction, but you don't seem to care for our guidance (regarding our comments about numerical stability for example, starting at surface level and then even taking time to explain it with examples - still in good faith.. either that this PR is merged, or that others learn from it).

By now, I've lost faith in this PR. I'll do a last pass responding to the remaining open discussions.
It might still be merged eventually, but I don't think it's going to happen because of comments like "Wont fix".

static LARGE_INTEGER frequency = {0, 0};
static void __attribute__((constructor)) PrimeQueryPerformanceFrequency ()
{
#define AVG_SET 2
ULARGE_INTEGER f_rdtsc, avg = {0, 0}, s_rdtsc;
JayFoxRox marked this conversation as resolved.
Show resolved Hide resolved
ULONG f_ticks = 0, s_ticks = 0;
JayFoxRox marked this conversation as resolved.
Show resolved Hide resolved

Sleep(500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

@GXTX GXTX Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values generated soon after the system is booted seem to give less precise results, as to why; I'm not sure yet. Runs with a 700ms initial wait and 200ms loop wait give us 733332000, and calling it again even moments later bring us to 733333000.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that's good enough? What kind of precision do you expect? With your multiplication by 1000 at the end, this means that there's a single tick difference (1.36425648 nanoseconds).

That's a 0.00013636369% error.. how is that not negligible?

What are you using this function for that you require such precision?

Copy link
Contributor Author

@GXTX GXTX Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you using this function for that you require such precision?

I don't need such precision. It's an attempt to placate you and other maintainers. I don't want a hard value, you do. So I've made it so it's as close as I think is reasonable (possible?). What percentage ± would you accept, how long of a wait?

Copy link
Member

@JayFoxRox JayFoxRox Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say a sleep of less than a millisecond would be good in constructors. Maybe 10ms if necessary.
There's valid cases of sleeping in the network, USB and GPU driver - even up to 100ms wouldn't be too problematic (especially async).

Not sure about the percentage. But I'd expect the integer to be exact (that's the "correct" solution after all) on a stock Xbox. If the integer is off by 1 I wouldn't care too much, although I could already see problems with different functions making assumptions about the fixed frequency (and there you'd likely use the hardcoded value). Maybe using the int -> float32 error would be a good measure.

But in this instance, there's a perfect solution which has no wait.
It outperforms your solution in every single way on a stock Xbox.

The only time it doesn't work is with CPU mods / under- or overclocking.
But that's not a case nxdk should support unless it's supportable with reasonable effort.
Hint: it likely isn't, because it becomes a problem of defining the limit of what is considered an Xbox. If I overclock the audio chip but that's used in some nxdk code to calibrate the overclocked CPU clock, then how could this ever work without telling nxdk what you did to your system?
I can see perfect reasons for overclocking either of them, but absolutely no way how to reasonably support it in nxdk.

We could support the more common case of overclocking the CPU, but not if it has major drawbacks for most users which run a stock Xbox (which is the majority).


for (int i = 0; i < AVG_SET; i++) {
// If we call rdtsc too fast we'll end up with div by 0
Sleep(200);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sleep at the start of the loop? To me, it looks like this should only do measure(); wait(); measure() (= a single wait).
Although I'm not sure about waiting altogether - I'd expect the CPU to know its frequency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm not sure about waiting altogether - I'd expect the CPU to know its frequency.

I guess I'm confused about this. Is there a cpuid flag for current frequency? There's a hardware register that sets the FSB, but this can't be guaranteed accurate for various reasons, including bad caps.

Copy link
Member

@JayFoxRox JayFoxRox Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this can't be guaranteed accurate for various reasons, including bad caps.

If the caps are so much out of tolerance that you are that far away from 733MHz that it even drifts a second per minute then I'd consider your Xbox broken.

You don't use QueryPerformanceCounter to implement a date/time system (wall-clock) but for relative measurements. If it's off by 0.1%, heck make it 1%, that shouldn't matter for applications.

Hence, we can safely assume the reference clock speed. There shouldn't be a need to read the FSB or cpuid - we know what CPU the Xbox platform has. If it doesn't, then it's not an Xbox and you should probably keep these things in local patches to nxdk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider your Xbox broken

As the Xbox continues to age this will become more and more relevant. I recently had a box which would report a 729MHz CPU frequency as the filter capacitors had blown, now, games still worked, and most wouldn't notice such an issue. Why should be not account for this? Why should we continue to make bad assumptions about the platform.

No patch I've ever introduced has attempted to move away from Xbox as a platform, to support different AGP cards, add-in PCI cards, FooBar SMBus device, I've only attempted to remove these hard coded values and addresses in an attempt to support more Xboxes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should be not account for this? Why should we continue to make bad assumptions about the platform.

Because this will likely affect the entire system.
The entire system will run faster or slower, but it doesn't matter because all components are expected to run equally different.

So even if you calibrate against another clock, the result will still be very close to 733.333MHz measured.

This is only a problem if the clock on a single component varies. However, most clocks are affected by a huge number of components, so drift on any of them can't cause the result to deviate much (likely there's an opposing force too.. and clock generators are also build to combat this).

But even if there is a problem with a single clock, then you don't know which one, you can't calibrate against another clock source because that might also have deteriorated. At best, you can average out the error of all clock sources.
But at that point, your measurement error or rounding errors due to your calculations will be more significant than the initial error.

You are also introducing a very high cost of synchronizing / measuring clocks and then continue to have a high cost due to a bunch of optimizations you can't do anymore due to lack of known clock frequencies at compile time (which also means you run more code to get the same result = the errors in clock affect you even more).

And even if you managed to get accurate measurements, you couldn't account for drift at runtime which would likely exist (in the case of electronic components due to external factors like heat).
Even by design the clock will not be a constant 733.333MHz - instead, it cycles a bit around (no idea how much or at what speed) the reference clock because otherwise it would cause some annoying humming as it would hit resonance frequencies (fail FCC tests etc.).

You can't compare a clock in a computer system to a wall-clock (unless specifically designed to be that.. which are RTCs.. which usually run at lower precision). It's not important that the frequency is perfect. But you still want it to be a very very good average if you don't know how people will use it.

Especially on a console like the Xbox, there'll be plenty of application code (out of our control) that will assume rdtsc to run with 733.333 MHz, and you can also assume that someone will compare results from QueryPerformanceCounter to rdtsc even if that's bad practice.

If you hardcode the expected frequency you get around all of these issues (and a couple of other benefits), because if one counter runs slower, they likely all will. What is important is that everyone agrees on what these clocks should be running at.

All that said, even a bad and drifting counter that's not an RTC or misconfigured is still perfectly valid for relative measurements or measurements which a human couldn't possible notice:

  • Would you notice a 0.1% speed difference when playing Halo? Probably not.
  • Would you care about a 0.1% speed difference when doing micro-optimizations on a function that's called a couple of million times per second: probably (but the error will cancel out during measurement).

I've only attempted to remove these hard coded values and addresses in an attempt to support more Xboxes.

And that's a noble goal.

The issue is that it's hard to get away from constant clock frequencies in the code for the reason mentioned above: they can't be measured (without depending on another clock.. which can also have errors).

I'm all for removing stupid checks or getting instead of setting clocks (= cases where we depend on hw setup by some XDK code instead of being able to do it ourselves).

What we should attempt is to mimic how the XDK does it.
If the XDK hardcodes a clock, then we can probably get away with it too.
If the XDK code stops working, it would imply the kernel wouldn't run and the intro animation would also fail.
The dashboard would crash and so would every game.
That wouldn't be a sign of nxdk needing code fixes, but a sign of an Xbox needing repair.

The goal should be: if XDK code works, then equivalent nxdk code should work. If the XDK hardcodes a value, then we should do that too.


s_rdtsc.QuadPart = __rdtsc();
s_ticks = KeTickCount;

s_rdtsc.QuadPart -= f_rdtsc.QuadPart;
JayFoxRox marked this conversation as resolved.
Show resolved Hide resolved
s_rdtsc.QuadPart /= s_ticks - f_ticks;
JayFoxRox marked this conversation as resolved.
Show resolved Hide resolved

f_rdtsc.QuadPart = __rdtsc();
f_ticks = KeTickCount;

// Skip the first result as invalid
if (i)
avg.QuadPart += s_rdtsc.QuadPart;
}
frequency.QuadPart = avg.QuadPart / (AVG_SET - 1) * 1000LL;
}
#endif

BOOL QueryPerformanceCounter (LARGE_INTEGER *lpPerformanceCount)
{
assert(lpPerformanceCount != NULL);

lpPerformanceCount->QuadPart = KeQueryPerformanceCounter();
lpPerformanceCount->QuadPart = __rdtsc();
return TRUE;
}

BOOL QueryPerformanceFrequency (LARGE_INTEGER *lpFrequency)
{
assert(lpFrequency != NULL);

lpFrequency->QuadPart = KeQueryPerformanceFrequency();
#ifdef USE_RDTSC_FOR_FREQ
lpFrequency->QuadPart = frequency.QuadPart;
#else
lpFrequency->QuadPart = 733333333;
#endif
return TRUE;
}
}