Skip to content

Replace ThreadPoolExecutor context-store with ClassValue cache - #12105

Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit into
masterfrom
mcculls/remove-tpe-context-store
Jul 30, 2026
Merged

Replace ThreadPoolExecutor context-store with ClassValue cache#12105
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit into
masterfrom
mcculls/remove-tpe-context-store

Conversation

@mcculls

@mcculls mcculls commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Motivation

The flag being stored in the TPE context-store is constant, based on whether the class is listed in the 'excludedClasses' set provided via static config. This is better suited to being stored as a ClassValue, especially for AOT scenarios where context-stores are more likely to use the global weak-map.

Additional Notes

Found while looking into #10479

Contributor Checklist

Jira ticket: [PROJ-IDENT]

@mcculls mcculls added tag: performance Performance related changes type: refactoring inst: java Core Java language instrumentation labels Jul 29, 2026
@mcculls
mcculls requested a review from Copilot July 29, 2026 21:46
@mcculls

mcculls commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Java-concurrent ThreadPoolExecutor instrumentation to stop using a ContextStore<ThreadPoolExecutor, Boolean> for a per-executor “propagate?” flag, and instead uses a ClassValue<Boolean> cache keyed by executor class to better fit AOT scenarios where context-stores may fall back to global weak-maps.

Changes:

  • Removed the ThreadPoolExecutor -> Boolean context-store entry and the constructor advice that populated it.
  • Added a ClassValue<Boolean> (PROPAGATE) in TPEHelper and switched advice sites to call TPEHelper.shouldPropagate(tpe) directly.
  • Simplified ThreadPoolExecutorInstrumentation to no longer depend on Platform / constructor instrumentation for propagation decisions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
dd-java-agent/instrumentation/java/java-concurrent/java-concurrent-1.8/src/main/java/datadog/trace/instrumentation/java/concurrent/executor/ThreadPoolExecutorInstrumentation.java Removes constructor advice + switches propagation checks to TPEHelper.shouldPropagate(tpe)
dd-java-agent/instrumentation/java/java-concurrent/java-concurrent-1.8/src/main/java/datadog/trace/instrumentation/java/concurrent/executor/ExecutorModule.java Drops the ThreadPoolExecutor -> Boolean context-store mapping
dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/java/concurrent/TPEHelper.java Replaces per-instance context-store flagging with a ClassValue<Boolean> cache

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6515e4b02b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@datadog-datadog-prod-us1-2

datadog-datadog-prod-us1-2 Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
Patch Coverage: 0.00%
Overall Coverage: 61.96% (+4.10%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 585e962 | Docs | Datadog PR Page | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.98 s 14.03 s [-1.1%; +0.5%] (no difference)
startup:insecure-bank:tracing:Agent 12.92 s 13.04 s [-1.7%; -0.2%] (maybe better)
startup:petclinic:appsec:Agent 16.85 s 16.63 s [+0.3%; +2.3%] (maybe worse)
startup:petclinic:iast:Agent 16.85 s 16.93 s [-1.2%; +0.2%] (no difference)
startup:petclinic:profiling:Agent 16.63 s 16.78 s [-2.0%; +0.2%] (no difference)
startup:petclinic:sca:Agent 16.55 s 16.85 s [-2.7%; -0.8%] (maybe better)
startup:petclinic:tracing:Agent 16.12 s 16.22 s [-1.4%; +0.2%] (no difference)

Commit: 585e962f · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

The flag being stored in the TPE context-store is constant, based on whether
the class is listed in the 'excludedClasses' set provided via static config.
This is better suited to being stored as a ClassValue, especially for AOT
scenarios where context-stores are more likely to use the global weak-map.
@mcculls
mcculls force-pushed the mcculls/remove-tpe-context-store branch from 6515e4b to 585e962 Compare July 29, 2026 23:04
@mcculls

mcculls commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 585e962fb3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@mcculls
mcculls marked this pull request as ready for review July 29, 2026 23:16
@mcculls
mcculls requested review from a team as code owners July 29, 2026 23:16
@mcculls
mcculls requested review from AlexeyKuznetsov-DD and dougqh and removed request for a team July 29, 2026 23:16

@datadog-datadog-prod-us1-2 datadog-datadog-prod-us1-2 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Datadog Autotest: PASS

More details

The ClassValue replacement preserves the previous exact executor-class exclusion decision while removing per-executor context-store state, and native-image build-time propagation remains disabled. Existing tests cover the meaningful propagation, exclusion, custom-subclass, cancellation, and legacy-mode paths; execution was blocked by the unavailable Java 25 toolchain.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Datadog Autotest · Commit 585e962 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

@mcculls
mcculls requested a review from PerfectSlayer July 29, 2026 23:22

@PerfectSlayer PerfectSlayer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, thanks!

@mcculls
mcculls added this pull request to the merge queue Jul 30, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jul 30, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-07-30 08:44:11 UTC ℹ️ Start processing command /merge


2026-07-30 08:44:16 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 1h (p90).


2026-07-30 10:02:07 UTC ℹ️ MergeQueue: This merge request was merged

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 30, 2026
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot merged commit 8fdab02 into master Jul 30, 2026
591 checks passed
@gh-worker-dd-mergequeue-cf854d
gh-worker-dd-mergequeue-cf854d Bot deleted the mcculls/remove-tpe-context-store branch July 30, 2026 10:02
@github-actions github-actions Bot added this to the 1.65.0 milestone Jul 30, 2026

@bric3 bric3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TIL, it's clever !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: java Core Java language instrumentation tag: performance Performance related changes type: refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants