[PROF-12714] Add CriticalNative support #282

zhengyu123 · 2025-10-24T13:50:36Z

What does this PR do?:

Motivation:

Additional Notes:

How to test the change?:

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.
JIRA: [JIRA-XXXX]

Unsure? Have a question? Request a review!

ivoanjo

Hey 👋 . As requested, I gave it a stab at reviewing the PR!

I'm missing some context on this code, in particular it was hard to determine which methods were performance-critical and which were used only for setup/testing/etc off the critical path.

I wonder if it's worth documenting and/or reflecting that in the method names.

I saw the benchmarks in JIRA -- I guess the important part is that ThreadFilterBenchmark is faster? May be worth posting them here too, to give a bit more context.

ddprof-lib/src/main/cpp/javaApi.cpp

ivoanjo · 2025-10-28T12:01:37Z

ddprof-lib/src/main/cpp/javaApi.cpp

 extern "C" DLLEXPORT jlong JNICALL
 Java_com_datadoghq_profiler_JavaProfiler_tscFrequency0(JNIEnv *env,
-                                                       jobject unused) {
+                                                       jclass unused) {
+  return TSC::frequency();
+}
+
+extern "C" DLLEXPORT jlong JNICALL
+JavaCritical_com_datadoghq_profiler_JavaProfiler_tscFrequency0() {


Similar note to init0 -- this value seems to be saved in a static final on the Java side and never called again.

ddprof-lib/src/main/cpp/javaApi.cpp

ddprof-lib/src/main/cpp/vmStructs_dd.cpp

zhengyu123 · 2025-10-29T14:30:25Z

Hey 👋 . As requested, I gave it a stab at reviewing the PR!

I'm missing some context on this code, in particular it was hard to determine which methods were performance-critical and which were used only for setup/testing/etc off the critical path.

I wonder if it's worth documenting and/or reflecting that in the method names.

I saw the benchmarks in JIRA -- I guess the important part is that ThreadFilterBenchmark is faster? May be worth posting them here too, to give a bit more context.

ThreadFilter methods are performance critical. However, given the simplicity of JavaCritical variants, it does not hurt to add them for none performance critical ones.

ivoanjo · 2025-10-29T15:36:22Z

However, given the simplicity of JavaCritical variants, it does not hurt to add them for none performance critical ones.

Hmm... I'll be honest and say that I don't think this is the way to go. Specifically:

Adding JavaCritical variants means more code + more maintenance for it generically. We need to remember to always keep the copy-pasted stuff up-to-date.
It makes it look like those methods are performance-sensitive, which they aren't, so it communicates a weird intent.
If we ever want to refactor some of those methods, we're now constrained by existence of the JavaCritical variant. What happens if it can't be a JavaCritical anymore because we'd like to pass in an object? It may not be clear for many of them we could trivially remove the JavaCritical variant and then change the method signature.

Can I maybe convince you to revisit your approach here? 😄 😉 👀

zhengyu123 · 2025-10-29T16:38:42Z

However, given the simplicity of JavaCritical variants, it does not hurt to add them for none performance critical ones.

Hmm... I'll be honest and say that I don't think this is the way to go. Specifically:

Adding JavaCritical variants means more code + more maintenance for it generically. We need to remember to always keep the copy-pasted stuff up-to-date.

It makes it look like those methods are performance-sensitive, which they aren't, so it communicates a weird intent.

If we ever want to refactor some of those methods, we're now constrained by existence of the JavaCritical variant. What happens if it can't be a JavaCritical anymore because we'd like to pass in an object? It may not be clear for many of them we could trivially remove the JavaCritical variant and then change the method signature.

Can I maybe convince you to revisit your approach here? 😄 😉 👀

I am bit confused by your comments - Are you suggesting not to introduce JavaCritical variants? or to remove none performance critical ones.

ivoanjo · 2025-10-29T17:05:50Z

Are you suggesting not to introduce JavaCritical variants? or to remove none performance critical ones.

The latter, indeed: I am suggesting to remove the non-performance critical.

TL;DR version of my point is: I think the trade-off is worth it 👍 for the ones that give us performance, but not worth it for the ones that don't 👎 .

pr-commenter · 2025-10-29T19:48:09Z

Benchmarks [x86_64 wall]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	wall	wall
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 15 metrics, 23 unstable metrics.

pr-commenter · 2025-10-29T19:48:38Z

Benchmarks [x86_64 alloc]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	alloc	alloc
wall	off	off

Summary

Found 0 performance improvements and 1 performance regressions! Performance is the same for 14 metrics, 23 unstable metrics.

scenario	Δ mean execution_time	Δ mean rss
scenario:renaissance:philosophers	worse [+212.536ms; +1115.464ms] or [+1.653%; +8.675%]	unstable [-504.838MB; +507.589MB] or [-51.563%; +51.844%]

pr-commenter · 2025-10-29T19:48:56Z

Benchmarks [x86_64 cpu]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	cpu	cpu
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 15 metrics, 23 unstable metrics.

pr-commenter · 2025-10-29T19:48:59Z

Benchmarks [x86_64 cpu,wall,alloc,memleak]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	cpu,wall,alloc,memleak	cpu,wall,alloc,memleak
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 15 metrics, 23 unstable metrics.

pr-commenter · 2025-10-29T19:49:01Z

Benchmarks [x86_64 memleak]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	memleak	memleak
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 16 metrics, 22 unstable metrics.

pr-commenter · 2025-10-29T19:49:10Z

Benchmarks [aarch64 cpu]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	cpu	cpu
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics.

pr-commenter · 2025-10-29T19:49:14Z

Benchmarks [x86_64 memleak,alloc]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	memleak,alloc	memleak,alloc
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics.

pr-commenter · 2025-10-29T19:49:34Z

Benchmarks [x86_64 cpu,wall]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	cpu,wall	cpu,wall
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics.

pr-commenter · 2025-10-29T19:49:56Z

Benchmarks [aarch64 memleak]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	memleak	memleak
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics.

pr-commenter · 2025-10-29T19:50:49Z

Benchmarks [aarch64 wall]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	wall	wall
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 16 metrics, 22 unstable metrics.

pr-commenter · 2025-10-29T19:51:06Z

Benchmarks [aarch64 alloc]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	alloc	alloc
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics.

pr-commenter · 2025-10-29T19:51:29Z

Benchmarks [aarch64 cpu,wall,alloc,memleak]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	cpu,wall,alloc,memleak	cpu,wall,alloc,memleak
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics.

pr-commenter · 2025-10-29T19:52:08Z

Benchmarks [aarch64 cpu,wall]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	off	off
cpu	on	on
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	off	off
modes	cpu,wall	cpu,wall
wall	on	on

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 16 metrics, 22 unstable metrics.

pr-commenter · 2025-10-29T19:52:23Z

Benchmarks [aarch64 memleak,alloc]

Parameters

	Baseline	Candidate
config	baseline	candidate
ddprof	1.34.0	1.34.0-critical_natives-SNAPSHOT

See matching parameters

	Baseline	Candidate
alloc	on	on
cpu	off	off
iterations	5	5
java	"11.0.28"	"11.0.28"
memleak	on	on
modes	memleak,alloc	memleak,alloc
wall	off	off

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 16 metrics, 22 unstable metrics.

ivoanjo

👍 Looks reasonable! I'd still strongly recommend being a lot more explicit about the ARM64 workaround code, so it's extremely clear for future maintainers why it exists (and when we can get rid of it)

ivoanjo · 2025-10-30T09:06:09Z

ddprof-lib/src/main/cpp/javaApi.cpp

 extern "C" DLLEXPORT void JNICALL
-Java_com_datadoghq_profiler_JavaProfiler_filterThreadAdd0(JNIEnv *env,
-                                                          jobject unused) {
+JavaCritical_com_datadoghq_profiler_JavaProfiler_filterThreadAdd0() {


Minor: Should we maybe declare these as inline to softly suggest to the compiler that Java_com_datadoghq_profiler_JavaProfiler_filterThreadAdd0 should inline this, not call into it?

(Or does that not work very well anyway?)

ivoanjo · 2025-10-30T09:09:36Z

ddprof-lib/src/main/cpp/vmStructs_dd.cpp

+  void VMStructs_::initCriticalJNINatives() {
+#ifdef __aarch64__
+    // aarch64 does not support CriticalJNINatives
+    JVMFlag* flag = JVMFlag::find("CriticalJNINatives", {JVMFlag::Type::Bool});
+    if (flag != nullptr && flag->get()) {
+        flag->set(0);
+    }
+#endif // __aarch64__
+  }
+


I still think we should have better documentation here of:

Why is this needed (work around JDK 8 bug on ... )

What happens if this is not here

"// aarch64 does not support CriticalJNINatives" I'm not sure will be enough for someone 1/2/n years from now to understand why this was added.

Bonus points for reflecting this into the method name e.g. installWorkaroundForJdk8ArmCrash (or something similarly descriptive) would be a good way of conveying some of this information :)

zhengyu123 added 2 commits October 10, 2025 14:24

[PROF-12714] Add CriticalNative support

4feff30

Disable CriticalJNINatives on aarch64

6cfd40f

zhengyu123 requested review from ivoanjo, jbachorik and r1viollet and removed request for ivoanjo October 24, 2025 21:39

zhengyu123 marked this pull request as ready for review October 24, 2025 21:41

ivoanjo reviewed Oct 28, 2025

View reviewed changes

Remove none performance critical variants and add comments

19b54db

zhengyu123 mentioned this pull request Oct 29, 2025

Remove unused ThreadFilter.filterThread0() method #286

Open

3 tasks

Remove more none performance critical CriticalJNINatives methods

18c788e

ivoanjo approved these changes Oct 30, 2025

View reviewed changes

[PROF-12714] Add CriticalNative support #282

Are you sure you want to change the base?

[PROF-12714] Add CriticalNative support #282

Conversation

zhengyu123 commented Oct 24, 2025

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivoanjo Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zhengyu123 commented Oct 29, 2025

Uh oh!

ivoanjo commented Oct 29, 2025

Uh oh!

zhengyu123 commented Oct 29, 2025

Uh oh!

ivoanjo commented Oct 29, 2025

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 wall]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 alloc]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 cpu]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 cpu,wall,alloc,memleak]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 memleak]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [aarch64 cpu]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 memleak,alloc]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [x86_64 cpu,wall]

Parameters

Summary

Uh oh!

pr-commenter bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [aarch64 memleak]

Parameters

Summary

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading

pr-commenter bot commented Oct 29, 2025 •

edited

Loading