KEMBAR78
support clock=tsc without a JVM by arielb1 · Pull Request #1123 · async-profiler/async-profiler · GitHub
Skip to content

Conversation

arielb1
Copy link
Collaborator

@arielb1 arielb1 commented Feb 3, 2025

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.

@apangin
Copy link
Member

apangin commented Feb 9, 2025

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.

@arielb1
Copy link
Collaborator Author

arielb1 commented Feb 9, 2025 via email

@apangin
Copy link
Member

apangin commented Feb 10, 2025

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.

Even worse, recorded timestamps may appear completely wrong.

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.

@arielb1
Copy link
Collaborator Author

arielb1 commented Feb 10, 2025 via email

@apangin
Copy link
Member

apangin commented Feb 10, 2025

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 clock=tsc is set explicitly: clock == CLK_TSC. Alternatively, if a check for reliablie tsc is implemented, tsc can be selected by default.

@arielb1
Copy link
Collaborator Author

arielb1 commented Feb 17, 2025

Pushed a branch with no calibration code. Seems to work just fine.

ED: still need to add reliable tsc check.

@arielb1
Copy link
Collaborator Author

arielb1 commented Feb 17, 2025

added invariant tsc detection

src/tsc.cpp Outdated
_offset = rdtsc() - jvm_ticks;
_frequency = frequency;
env->ExceptionClear();
_initialized = true;
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@arielb1 arielb1 Feb 18, 2025

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.

@apangin apangin merged commit 2d764cc into async-profiler:master Feb 18, 2025
9 checks passed
@apangin apangin mentioned this pull request Feb 25, 2025
2 tasks
krk pushed a commit to krk/async-profiler that referenced this pull request Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants