Introduce Maybe<T> an allocation free return value for fallible operations - #12328
Open
dougqh wants to merge 15 commits into
Open
Introduce Maybe<T> an allocation free return value for fallible operations#12328dougqh wants to merge 15 commits into
dougqh wants to merge 15 commits into
Conversation
Fourteen minimal arms, each isolating one thing believed to decide whether C2 can delete a short-lived object, read through gc.alloc.rate.norm. The suite was written against APMLP-1642's granted/refused reservation and carries over unchanged, because a Try<T> is the same two-outcome wrapper and what is measured is the shape, not the caller. It answers part of APMLP-1799's section A ahead of time: the Optional-style merge with a singleton costs 8 B/op on 25 as well as 17, since ReduceAllocationMerges never covers a static input; a merge of two allocations, which is what Success/Failure compiles to, stays 16 on 25; and three receiver types at one call site cost 24. One allocation site carrying a flag is 0 on both, with or without try/finally. Crossing a call boundary is free when the callee inlines and 24 B/op when it does not. The JDK 8, 11 and 21 columns are unfilled, there is no EA-off control arm and no type-profile pollution, so this is not yet the card's deliverable 2 — it is a running start on it. Cold-path escape and callee size, the two arms that could still kill the wrapper, are not probed at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ran the suite on this machine's Zulu 8.72.0.17 with -Pjmh.fork=1, -prof gc. Every arm matches its 17/25 reading, including phiWithNull staying at 8 B/op rather than dropping to 0 -- the ReduceAllocationMerges relaxation only applies from JDK 21, so JDK 8 sees the older, unconditional merge-with-null cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
APMLP-1799 spike: a generic fallible-operation return shape (single construction site, plain nullable field, no Optional-style singleton merge) confirmed allocation-free on JDK 8/11/17/25 by EscapeShapeBenchmark. Not wired into any real caller yet -- Hashtable usage lands in a follow-up PR once it can build on this.
Confirmed 2026-08-27 against a real capacity-refusing lookup method (JDK 8/11/17/25, common and rare refusal ratios): the capturing lambda scalar-replaces as reliably as a plain delegating call. Full benchmark lands with the Hashtable-integration follow-up.
Both flavors are named in the ticket's mutator-flavor set (Consumer / BiConsumer<C> / ObjLongConsumer / ObjIntConsumer) that motivated the N*M overload explosion on Hashtable. Consolidating them onto Try means that shape is paid for once here instead of once per table type. double/boolean context forms are deliberately not added: neither is a named flavor, and boolean has no JDK ObjBooleanConsumer to reuse -- both would be speculative additions without a real caller.
Rounds out the primitive-context set to int/long/double/boolean, anticipating future callers rather than waiting on one, matching how Stream/Optional carry int/long/double specializations. boolean has no JDK ObjBooleanConsumer to reuse (neither does Stream/Optional -- the JDK never shipped a boolean specialization there either), so it's a small hand-rolled functional interface instead.
Completes the mutator-flavor set named in the ticket (Consumer / BiConsumer<C> / ObjLongConsumer / ObjIntConsumer) plus the anticipated double/boolean siblings. Argument order matches Hashtable#forEach's existing (context, entry) convention rather than the primitive forms' (entry, context), since that's the precedent this is meant to line up with.
Standalone JMH demonstration (not a research instrument like
EscapeShapeBenchmark) meant to back a perf-review check like
APMLP-1799's J12 note. Two pairs:
- goodSingleConstructionSite vs badMultiConstructionSite: the
single-construction-point discipline from Try's class javadoc,
made visible as 0 vs 16 B/op rather than left as a claim.
- goodPrimitiveContextUpdate vs badBoxedContext{Inlined,Uninlined}:
update(long, ObjLongConsumer) reads 0 B/op deterministically;
update(Long, BiConsumer) also reads 0 B/op but only because this
call site stays inlined and C2 scalar-replaces the box -- forcing
the same call out of line (CompileCommand=dontinline, the same
technique EscapeShapeBenchmark uses for UninlinedStrategy) shows
the real 24 B/op the primitive overload avoids unconditionally.
Try collides with Scala/Vavr's Try<T>, which carries a captured exception (Success/Failure) -- a model this class never had and, by design, never will: it's a plain present/absent wrapper with no error payload, i.e. Haskell's Maybe (Just/Nothing) rather than Either or Result. The name should say so rather than borrow one that implies exception handling.
The previous commit only captured the file renames -- the actual Try -> Maybe identifier/javadoc replacements were left uncommitted by a failed `git add -A` (stale pathspec aborted the whole add silently).
dougqh
added
type: feature
Contributor
Author
|
@codex review |
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9296289 | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec1419151e
ℹ️ 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".
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
dougqh
changed the title
The multi-construction-site claim ("fails scalar replacement on every
JDK 8-25") was wrong: measured badMultiConstructionSite directly on
JDK 8/11/17/21/25 -- 16 B/op on 8-21, but ~0 B/op on 25, where
ReduceAllocationMerges collapses two branches allocating the same
final type. Documented the JDK-25 exception and why it's not something
to rely on. Also corrected the capturing-lambda javadoc (a capturing
lambda is freshly instantiated per call, not "built once per call
site") and the stale TryUsagePatterns JMH filter in the class doc.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f40ced4c9a
ℹ️ 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-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The numeric update methods make a direct lambda call ambiguous. Callers must cast or declare the consumer, so the intended primitive-context API does not compile in its natural form.
🤖 Datadog Autotest · Commit f40ced4 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
AlexeyKuznetsov-DD
approved these changes
Aug 28, 2026
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
Contributor
There was a problem hiding this comment.
LGTM, left minor comments and one idea.
bric3
reviewed
Aug 28, 2026
bric3
left a comment
bric3
left a comment
Contributor
There was a problem hiding this comment.
My untrained eye tripped over a few things, please bear with me.
- Drop the int/double/boolean update overloads: confirmed by direct compilation that a second primitive overload makes inline-lambda calls ambiguous (JLS 15.12.2.5's most-specific-method rule requires every parameter position to agree, and ObjIntConsumer/ObjLongConsumer/ ObjDoubleConsumer are unrelated interfaces). Keep only update(long, ObjLongConsumer) -- flagged independently by Codex and the Datadog Autotest bot. Removes the now-dead ObjBooleanConsumer. - Qualify MaybeUsagePatternsBenchmark's zero-allocation claim: the boxed-context pair's 0 B/op is contingent on the update call itself staying inlined, not immune to every inlining failure. - Add MaybeTest covering every branch of Maybe (both of() overloads, isPresent/getOrNull, all three update overloads, ifPresentOrElse) so internal-api's per-class JaCoCo coverage gate passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per review nit: that context belongs in the PR description, not a comment that will rot as soon as a caller adopts it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Rename EscapeShapeBenchmark's domain-flavored types (Cell/Granted/ Alternate/REFUSED/Flagged) to neutral names (Outcome/SingleAllocation/ AlternateAllocation/STATIC_SINGLETON/FlaggedAllocation) so the benchmark reads as a compiler-behavior experiment, not a tracer reservation model. - Drop the APMLP-1642/APMLP-1799 JIRA references from the class javadoc in favor of a generic two-outcome-wrapper description. - Add a short glossary paragraph (escape analysis, scalar replacement, ReduceAllocationMerges) and note the results are HotSpot/C2-specific, not validated against OpenJ9/GraalVM. - Rename MaybeUsagePatternsBenchmark's ADD_BOXED to ADD_BOXED_INLINED for symmetry with ADD_BOXED_UNINLINED. - Trim the dated "Confirmed 2026-08-27" research-note phrasing out of Maybe's javadoc, keeping the substance without the research-log tone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Lead the glossary paragraph with plain-language framing before naming EA/scalar-replacement/ReduceAllocationMerges, drop "phi" in favor of "merge" throughout (prose, table, and benchmark method names), and explain the merge case with a concrete if/else example instead of SSA terminology.
Contributor
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in Use ⏳ Building merge commit 07bec6f060 in pipeline 1312990814514985679... |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Adds
Maybe<T>: a present/absent wrapper for fallible operations (e.g. a capacity-refusing table lookup)Motivation
Allowing update/mutation calls without worrying about accidentally raising
NullPointerException.Maybe<T>is carefully designed to be scalar replaced via escape analysis in all supported JVMs.Additional Notes
Confirmed allocation-free (JDK 8/11/17/25), provided the wrapping method constructs the
Maybeat exactly one call site — see the class javadoc andMaybeUsagePatternsBenchmarkfor the single- vs multi-construction-site contrast that makes that discipline concrete rather than a claim. Note: the multi-construction-site "bad" arm measures 16 B/op on JDK 8/11/17/21, but ~0 B/op on JDK 25, whereReduceAllocationMergeshappens to collapse this specific two-same-type-branch shape — documented in the class javadoc as a JDK-25-only behavior not to be relied on.Created in response to
Hashtable's capacity-refusing operations, which forced a choice between an NPE footgun (returning a plain nullable) and a combinatorial explosion of method overloads.Maybe<T>bridges the gap by becoming the logical home for mutation/update methods.MaybeUsagePatternsBenchmarkis also meant as the backing example for a perf-review check like "EA-dependent elision on a hot path where a structural alternative exists at parity → prefer the deterministic form" (APMLP-1799's J12 note): its boxed-context arm shows 0 B/op only while its call site stays inlined, versus the primitive-context overload's 0 B/op unconditionally.update's primitive-context form islong-only. Anint/double/booleansibling was tried and reverted after review (Codex + Datadog Autotest both caught it): a second primitive overload makes inline-lambda calls toupdategenuinely ambiguous, confirmed by direct compilation — JLS 15.12.2.5's most-specific-method rule requires every parameter position to agree, andObjIntConsumer/ObjLongConsumer/ObjDoubleConsumerare unrelated interfaces, so the functional-interface parameter blocks resolution even though the numeric parameter alone would resolve via widening. See the javadoc onupdate(long, ObjLongConsumer)for the full reasoning.Test plan
./gradlew :internal-api:compileJava :internal-api:compileJmhJava— clean./gradlew :internal-api:spotlessCheck— clean./gradlew :internal-api:test --tests datadog.trace.util.MaybeTest— clean, covers bothofoverloads,isPresent/getOrNull, all threeupdateoverloads (present + absent), andifPresentOrElse./gradlew :internal-api:jacocoTestCoverageVerification— clean (per-class branch/instruction coverage gate satisfied)MaybeUsagePatternsBenchmark.badMultiConstructionSiteacross JDK 8/11/17/21/25 — 16 B/op on 8-21, ~0 B/op on 25 (see Additional Notes)MaybeUsagePatternsBenchmark's other arms (JDK 17) — good arms read ≈0 B/op, boxed-context bad arm reads the expected non-zero allocation (24 B/op uninlined)MaybeUsagePatternsBenchmarkarms (already done for the coreMaybe/EscapeShapeBenchmarkshapes; not yet re-run for every arm in this specific demonstration file)🤖 Generated with Claude Code