Skip to content

Detect process-id on Java 8 via ManagementFactory instead of JNR/POSIX#4506

Merged
mcculls merged 1 commit intomasterfrom
mcculls/replace-posix-pid-with-value-from-vmid
Jan 6, 2023
Merged

Detect process-id on Java 8 via ManagementFactory instead of JNR/POSIX#4506
mcculls merged 1 commit intomasterfrom
mcculls/replace-posix-pid-with-value-from-vmid

Conversation

@mcculls
Copy link
Copy Markdown
Contributor

@mcculls mcculls commented Jan 5, 2023

What Does This Do

Relies on an assumption that the initial part of the vmId from ManagementFactory is our process id.

This is true for all JVMs based on OpenJDK, but may not be true for other JVMs that don't follow this convention:
https://github.com/openjdk/jdk/blob/jdk8-b120/jdk/src/share/classes/sun/management/VMManagementImpl.java#L145

Note that JVMs after Java8 will continue to use the standard ProcessHandle.current().pid() API

Motivation

PR #4397 added the process-id to all root spans, which means that the process-id is now requested as part of building the tracer in premain. Unfortunately the JNR/POSIX layer which is used for Java8 only takes some time to initialize, and this leads to a noticeable increase in startup time.

On my M1 mac the JNR/POSIX approach takes 0.5 seconds, whereas using ManagementFactory takes 0.03 seconds.

@mcculls mcculls added the tag: performance Performance related changes label Jan 5, 2023
@mcculls mcculls requested a review from a team as a code owner January 5, 2023 23:51
@mcculls mcculls force-pushed the mcculls/replace-posix-pid-with-value-from-vmid branch from f6463e1 to a4cc1b6 Compare January 6, 2023 00:01
@mcculls mcculls requested a review from a team as a code owner January 6, 2023 00:01
Copy link
Copy Markdown
Contributor

@PerfectSlayer PerfectSlayer left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread internal-api/src/main/java/datadog/trace/util/PidHelper.java
Copy link
Copy Markdown
Contributor

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

🚀

@mcculls mcculls merged commit ab308f4 into master Jan 6, 2023
@mcculls mcculls deleted the mcculls/replace-posix-pid-with-value-from-vmid branch January 6, 2023 12:15
@github-actions github-actions Bot added this to the 1.4.0 milestone Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tag: performance Performance related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants