-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Consolidate QueryPerformanceCounter/GetTickCount usages to minipal/time.h #115408
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.
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
Conversation
I wonder whether the different timer implementations can be replaced by |
Build error looks unrelated:
Should be introduced by #115569 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Build breaks on wasm:
|
The large number of non-wasm timeouts looks a bit suspect as well. We will see whether it repros once the build breaks are fixed. |
I'm somehow confused with wasm build config. Should |
These build breaks occur in the Windows-hosted Browser-targeting cross-compiler. Mono compiles with higher warning levels - it is why the build break shows up for browser only. |
I left a suggestion that should make the compiler happy |
Co-authored-by: Jan Kotas <[email protected]>
This breaks the PAL tests build. @huoyaoyuan How was this tested locally?
|
|
||
return ts.QuadPart / (performanceFrequency.QuadPart / 1000); | ||
return (UINT64)minipal_hires_ticks(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very sloppy change :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be minipal_lowres_ticks
indeed because it only uses ms precision.
I tested PAL test build on local WSL before publishing the PR. Maybe some later commit gets it broken? |
Doubtful. The APIs were never removed. How did you build and run the tests? I used |
I use CMake target view in Visual Studio and builds the paltests target. It does break for me for latest commit. I don't remember the exact commit I tested this. |
I thought that we have trigger to run PAL tests on PAL changes, but it is apparently not the case. I am sorry... |
…me.h (#115408) Remove the duplicated implementations and use minipal/time.h as single source of truth. Not touching a bit ifdef'd-out instances, which are supposed for local debug usages only. Co-authored-by: Jan Kotas <[email protected]>
Is this supposed to be no-diff change? I see some perf regressions (from #115904) that correlate strongly with this change. Example perf chart ![]() The jump on May 22 (lab time, GMT) spans 3 commits: f9ad7c9...e163124 ADX shows the regression is only on Zen4/Ubuntu 22.04 ![]() |
Yes, this is expected to be non-impactful change. I do not see how this change can affect the microbenchmark in question. |
Ok, thanks. Another possibility is that we took some kind of os/firmware update. Let me check. |
Remove the duplicated implementations and use
minipal/time.h
as single source of truth.Not touching a bit ifdef'd-out instances, which are supposed for local debug usages only.