-
Notifications
You must be signed in to change notification settings - Fork 937
support clock=tsc without a JVM #1123
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
Thank you for your PR. |
On AMD and ARM processors, there is no API to get the TSC frequency, so you need a calibration loop to measure it.The code currently does not check that the tsc is invariant (i.e. ticks at a constant real-world frequency). If you run it on an Intel processor with a non-invariant tsc, the ticks-per-second will be incorrect.On the other hand, this code only runs if you explicitly ask for clock source=tsc. I am not sure that silently reverting to wall clock time in that case is actually better. I don’t care either way.On 9 Feb 2025, at 2:37, Andrei Pangin ***@***.***> wrote:
Thank you for your PR.
Could you please provide a bit more context on what calibration logic does and why it is needed?
How do you check that TSC can be used for timestamps? Although TSC is reliable on modern x86 CPUs, this is not always the case for older processors.
Can you also clarify the ownership of the code? If the algorithm is copied from another project with a different license, I'm afraid I can't accept the PR without consent of the original author.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Not really. async-profiler automatically computes frequency at the end of each JFR chunk. The only case when the frequency is needed in advance is Java lock profiling, which requires translating ticks to nanoseconds on the go.
Even worse, recorded timestamps may appear completely wrong.
But this is not how it currently works. With your change, async-profiler will always attempt to use TSC, unless |
It’s easy enough to add a check for non invariant tsc. Should I add one?If the calibration loop is only needed when Java lock profiling is used, and Java lock profiling is only used when there is a JVM, can I just remove the calibration loop code and instead put an assertion to ensure the tsc frequency is only used in JVM mode?On 10 Feb 2025, at 2:47, Andrei Pangin ***@***.***> wrote:
you need a calibration loop to measure it
Not really. async-profiler automatically computes frequency at the end of each JFR chunk. The only case when the frequency is needed in advance is Java lock profiling, which requires translating ticks to nanoseconds on the go.
If you run it on an Intel processor with a non-invariant tsc, the ticks-per-second will be incorrect.
That's my concern exactly.
this code only runs if you explicitly ask for clock source=tsc
But this is not how it currently works. With your change, async-profiler will always attempt to use TSC, unless clock=monotonic is set explicitly.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Yes, I believe calibration loop is unnecessary for non-Java processes, at least for now, while there are no events with thresholds measured in nanoseconds. The simplest way to go forward will be to choose TSC for non-Java processes only when |
316c907
to
9b1deec
Compare
9b1deec
to
0b80acb
Compare
Pushed a branch with no calibration code. Seems to work just fine. ED: still need to add reliable tsc check. |
added invariant tsc detection |
src/tsc.cpp
Outdated
_offset = rdtsc() - jvm_ticks; | ||
_frequency = frequency; | ||
env->ExceptionClear(); | ||
_initialized = true; |
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.
Why is it set to true
only in the VM branch? The initialization code is supposed to run once in non-Java case too, isn't it?
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.
I didn't understand the code logic. fixed that.
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.
ed: when I wrote the code I tried to follow the pre-existing logic, which didn't set _initialized
if the JVM was not loaded. I think that it is better to do it this way.
It's not like the JVM can be loaded while async-profiler is running.
Description
Supports clock=tsc without a JVM. See #1106.
The clock calibration logic has been copied from https://github.com/metrics-rs/quanta
How has this been tested?
unit tests + tested locally
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.