From 0c69ed9a5360fa6a022add7ce2d4ee2493128fde Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 28 Sep 2026 19:45:20 -0400 Subject: [PATCH 01/12] Add JMH benchmark exploring a generic guarded-decode/breaker pattern Follow-up to the Kafka header Base64-decode guard fix. Compares the hand-specialized GuardedBase64Decode/Breaker shape against a generic ParseHandler + Breaker abstraction (construction-time vs. call-time strategy composition) to see what devirtualization costs, if any, a reusable version of this pattern would carry. Motivated by APMLP-1513's children (APMLP-1760, APMLP-1769, APMLP-1772, APMLP-1783), which show enough independent exception/parsing-cost cases to justify considering a shared primitive rather than one-off copies. Co-Authored-By: Claude Sonnet 5 --- .../trace/api/FunctionsBase64Benchmark.java | 364 ++++++++++++++++++ 1 file changed, 364 insertions(+) create mode 100644 internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java new file mode 100644 index 00000000000..6ff6fa86e6f --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -0,0 +1,364 @@ +package datadog.trace.api; + +import static datadog.trace.api.Functions.BASE64_DECODE; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.function.Function; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * Compares {@link Functions#BASE64_DECODE} on valid input (no exception) against malformed input, + * where every call throws and catches an {@link IllegalArgumentException}. Motivated by a customer + * seeing ~180K/day of this exact throw from Kafka header extraction (non-Base64 header values from + * a mixed producer). Point is to see whether HotSpot's fast-throw stack-trace omission actually + * kicks in for this call site under sustained repeated throws, or whether the caught path pays for + * a full stack trace fill-in every time. + */ +@Fork(2) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@State(Scope.Benchmark) +public class FunctionsBase64Benchmark { + + static final byte[] VALID = + Base64.getEncoder().encode("x-datadog-trace-id=1234567890".getBytes(StandardCharsets.UTF_8)); + + static final byte[] INVALID = "not-valid-base64!@#".getBytes(StandardCharsets.UTF_8); + + private static final boolean[] BASE64_ALPHABET = buildAlphabetTable(); + + private static boolean[] buildAlphabetTable() { + boolean[] table = new boolean[256]; + String alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/="; + for (int i = 0; i < alphabet.length(); i++) { + table[alphabet.charAt(i)] = true; + } + return table; + } + + // Branch-free per byte on purpose: a data-dependent early exit helps the rare "bad byte early" + // case but hurts the common valid case, which always scans the whole buffer anyway. + private static boolean looksLikeBase64(byte[] bytes) { + if (bytes.length == 0 || (bytes.length & 3) != 0) { + return false; + } + boolean valid = true; + for (byte b : bytes) { + valid &= BASE64_ALPHABET[b & 0xFF]; + } + return valid; + } + + static final Function PRECHECK_BASE64_DECODE = + bytes -> looksLikeBase64(bytes) ? BASE64_DECODE.apply(bytes) : null; + + private static final int CLOSE_THRESHOLD = 20; + + /** + * Thrown only when the cheap alphabet-scan precheck already knows the input can't be valid + * Base64, so we never call into {@link Base64}'s decoder (or pay for its exception's stack-trace + * capture) at all. Deliberately extends {@link IllegalArgumentException} so it's a drop-in for + * existing catch sites around the real decoder; callers should not rely on a stack trace being + * present for this specific instance — a real decode failure still throws the JDK's own + * exception, unmodified, with its own message and stack trace. + */ + static final class FastFailBase64Exception extends IllegalArgumentException { + private static final String MESSAGE = "Header value is not valid Base64"; + + FastFailBase64Exception() { + super(MESSAGE); + } + + // IllegalArgumentException has no writableStackTrace-suppressing constructor of its own, so + // skip the stack walk here instead. + @Override + public synchronized Throwable fillInStackTrace() { + return this; + } + } + + // Single-word countdown: 0 == closed (no precheck), >0 == guarded, counting down to close. + // Plain int on purpose: this is advisory hysteresis, not correctness-critical state, so a lost + // update or a stale read across threads just means one extra precheck or one extra exception. + static final class Breaker { + int state; + + String decode(byte[] bytes) { + if (state > 0 && !looksLikeBase64(bytes)) { + return null; + } + String result = BASE64_DECODE.apply(bytes); + if (result == null) { + state = CLOSE_THRESHOLD; + } else if (state > 0) { + state--; + } + return result; + } + + // Same hysteresis, but preserves throw-based failure semantics: a precheck-known failure + // throws our stack-trace-free stand-in, a real decode failure lets the JDK's own + // IllegalArgumentException (with its own message and stack trace) propagate untouched. + String decodeOrThrow(byte[] bytes) { + if (state > 0 && !looksLikeBase64(bytes)) { + throw new FastFailBase64Exception(); + } + try { + String result = new String(Base64.getDecoder().decode(bytes), StandardCharsets.UTF_8); + if (state > 0) { + state--; + } + return result; + } catch (IllegalArgumentException e) { + state = CLOSE_THRESHOLD; + throw e; + } + } + } + + final Breaker breakerForValid = new Breaker(); + final Breaker breakerForInvalid = new Breaker(); + final Breaker breakerThrowingForValid = new Breaker(); + final Breaker breakerThrowingForInvalid = new Breaker(); + + /** + * Hand-written specialization above vs. a generic {@code ParseHandler}/{@code GenericBreaker} + * pair below, to see what the CHA-based devirtualization actually costs relative to a single + * concrete, non-generic class with everything inlined by hand. + */ + abstract static class ParseHandler { + abstract boolean isDefinitelyInvalid(TInput input); + + abstract TOutput parse(TInput input); + + abstract TException createFastFailException(); + } + + static final ParseHandler BASE64_PARSE_HANDLER = + new ParseHandler() { + @Override + boolean isDefinitelyInvalid(byte[] input) { + return !looksLikeBase64(input); + } + + @Override + String parse(byte[] input) { + return new String(Base64.getDecoder().decode(input), StandardCharsets.UTF_8); + } + + @Override + IllegalArgumentException createFastFailException() { + return new FastFailBase64Exception(); + } + }; + + // Strategy composed at construction time (stored as a field). Contrasted below with + // GenericBreakerParam, which takes the same strategy at call time instead — comparing them is + // the actual point, since a field forces C2 to also prove the receiver is constant before it + // can fold through to a concrete ParseHandler, while a call-time argument doesn't. + static final class GenericBreakerCtor { + private final ParseHandler handler; + int state; + + GenericBreakerCtor(ParseHandler handler) { + this.handler = handler; + } + + TOutput decode(TInput input) { + if (state > 0 && handler.isDefinitelyInvalid(input)) { + throw handler.createFastFailException(); + } + try { + TOutput result = handler.parse(input); + if (state > 0) { + state--; + } + return result; + } catch (RuntimeException e) { + state = CLOSE_THRESHOLD; + throw e; + } + } + } + + // Same hysteresis, but the strategy is passed at call time rather than stored in a field. + static final class GenericBreakerParam { + int state; + + TOutput decode(TInput input, ParseHandler handler) { + if (state > 0 && handler.isDefinitelyInvalid(input)) { + throw handler.createFastFailException(); + } + try { + TOutput result = handler.parse(input); + if (state > 0) { + state--; + } + return result; + } catch (RuntimeException e) { + state = CLOSE_THRESHOLD; + throw e; + } + } + } + + final GenericBreakerCtor genericBreakerForValid = + new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); + final GenericBreakerCtor genericBreakerForInvalid = + new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); + + final GenericBreakerParam genericBreakerParamForValid = + new GenericBreakerParam<>(); + final GenericBreakerParam + genericBreakerParamForInvalid = new GenericBreakerParam<>(); + + @Benchmark + public void breakerValid(Blackhole bh) { + bh.consume(breakerForValid.decode(VALID)); + } + + @Benchmark + public void breakerInvalid(Blackhole bh) { + bh.consume(breakerForInvalid.decode(INVALID)); + } + + @Benchmark + public void breakerThrowingValid(Blackhole bh) { + bh.consume(breakerThrowingForValid.decodeOrThrow(VALID)); + } + + @Benchmark + public void breakerThrowingInvalid(Blackhole bh) { + try { + bh.consume(breakerThrowingForInvalid.decodeOrThrow(INVALID)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } + + @Benchmark + public void genericBreakerValid(Blackhole bh) { + bh.consume(genericBreakerForValid.decode(VALID)); + } + + @Benchmark + public void genericBreakerInvalid(Blackhole bh) { + try { + bh.consume(genericBreakerForInvalid.decode(INVALID)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } + + @Benchmark + public void genericBreakerParamValid(Blackhole bh) { + bh.consume(genericBreakerParamForValid.decode(VALID, BASE64_PARSE_HANDLER)); + } + + @Benchmark + public void genericBreakerParamInvalid(Blackhole bh) { + try { + bh.consume(genericBreakerParamForInvalid.decode(INVALID, BASE64_PARSE_HANDLER)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } + + @Benchmark + public void valid(Blackhole bh) { + bh.consume(BASE64_DECODE.apply(VALID)); + } + + @Benchmark + public void invalid(Blackhole bh) { + bh.consume(BASE64_DECODE.apply(INVALID)); + } + + @Benchmark + public void precheckValid(Blackhole bh) { + bh.consume(PRECHECK_BASE64_DECODE.apply(VALID)); + } + + @Benchmark + public void precheckInvalid(Blackhole bh) { + bh.consume(PRECHECK_BASE64_DECODE.apply(INVALID)); + } + + // Deterministic stream: one INVALID every invalidEveryN calls, VALID otherwise. Each mix + // benchmark gets its own counter/breaker so the three strategies see the identical pattern. + @State(Scope.Thread) + public static class Mix { + + @Param({"2", "10", "100", "1000", "10000"}) + int invalidEveryN; + + int counter; + final Breaker breaker = new Breaker(); + final Breaker throwingBreaker = new Breaker(); + final GenericBreakerCtor genericBreaker = + new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); + final GenericBreakerParam genericBreakerParam = + new GenericBreakerParam<>(); + + byte[] next() { + if (++counter >= invalidEveryN) { + counter = 0; + return INVALID; + } + return VALID; + } + } + + @Benchmark + public void mixUnguarded(Mix mix, Blackhole bh) { + bh.consume(BASE64_DECODE.apply(mix.next())); + } + + @Benchmark + public void mixGuarded(Mix mix, Blackhole bh) { + bh.consume(PRECHECK_BASE64_DECODE.apply(mix.next())); + } + + @Benchmark + public void mixBreaker(Mix mix, Blackhole bh) { + bh.consume(mix.breaker.decode(mix.next())); + } + + @Benchmark + public void mixBreakerThrowing(Mix mix, Blackhole bh) { + byte[] bytes = mix.next(); + try { + bh.consume(mix.throwingBreaker.decodeOrThrow(bytes)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } + + @Benchmark + public void mixGenericBreaker(Mix mix, Blackhole bh) { + byte[] bytes = mix.next(); + try { + bh.consume(mix.genericBreaker.decode(bytes)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } + + @Benchmark + public void mixGenericBreakerParam(Mix mix, Blackhole bh) { + byte[] bytes = mix.next(); + try { + bh.consume(mix.genericBreakerParam.decode(bytes, BASE64_PARSE_HANDLER)); + } catch (IllegalArgumentException e) { + bh.consume(e); + } + } +} From 94ca267e2c931e7e0552a254f352a2494b177590 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 30 Sep 2026 16:56:54 -0400 Subject: [PATCH 02/12] Rework the Base64 guard benchmark around an abstract DynamicLatch Replace the ParseHandler plus GenericBreakerCtor/GenericBreakerParam pair with one abstract DynamicLatch sketch, local to the benchmark, whose subclass is the strategy: an optimistic parse, a cheap correct pre-check and a stackless failure. That removes the constructor-versus-call-time question, since there is no separate strategy object. The latch has two flavors: get lets the failure flow to the caller (throwing a stackless stand-in while engaged), and tryGetOrNull converts it to null and builds no exception at all while engaged. Latches are static final fields of a named final subclass. The hand-written Breaker stays as the specialized baseline, and a setup self-check fails fast if the sketch misbehaves. Co-Authored-By: Claude Sonnet 5.5 --- .../trace/api/FunctionsBase64Benchmark.java | 223 +++++++++++------- 1 file changed, 134 insertions(+), 89 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index 6ff6fa86e6f..4d8a315a96f 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -10,6 +10,7 @@ import org.openjdk.jmh.annotations.Measurement; import org.openjdk.jmh.annotations.Param; import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; @@ -21,6 +22,17 @@ * a mixed producer). Point is to see whether HotSpot's fast-throw stack-trace omission actually * kicks in for this call site under sustained repeated throws, or whether the caught path pays for * a full stack trace fill-in every time. + * + *

It also measures a reusable form of the guard. {@link DynamicLatch} is a local sketch of the + * shape discussed in APMLP-1884: an abstract class whose subclass is the strategy (an + * optimistic parse, a cheap correct pre-check, and a stackless failure), held in a {@code static + * final}. It replaces an earlier pair of generic variants that stored the strategy in a field or + * took it at call time, which are not needed once the subclass carries the hooks. It has two public + * flavors: {@link DynamicLatch#get} lets the failure flow to the caller (throwing a stackless + * stand-in while engaged), and {@link DynamicLatch#tryGetOrNull} converts it to {@code null} and + * never builds an exception at all while engaged. + * + *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. */ @Fork(2) @Warmup(iterations = 3, time = 1) @@ -69,6 +81,9 @@ private static boolean looksLikeBase64(byte[] bytes) { * existing catch sites around the real decoder; callers should not rely on a stack trace being * present for this specific instance — a real decode failure still throws the JDK's own * exception, unmodified, with its own message and stack trace. + * + *

A new instance is built per failure, never shared, so suppression cannot accumulate on it. A + * shared instance would also have to disable suppression and forbid {@code initCause}. */ static final class FastFailBase64Exception extends IllegalArgumentException { private static final String MESSAGE = "Header value is not valid Base64"; @@ -124,101 +139,144 @@ String decodeOrThrow(byte[] bytes) { } } - final Breaker breakerForValid = new Breaker(); - final Breaker breakerForInvalid = new Breaker(); - final Breaker breakerThrowingForValid = new Breaker(); - final Breaker breakerThrowingForInvalid = new Breaker(); - /** - * Hand-written specialization above vs. a generic {@code ParseHandler}/{@code GenericBreaker} - * pair below, to see what the CHA-based devirtualization actually costs relative to a single - * concrete, non-generic class with everything inlined by hand. + * Local sketch of a {@code DynamicLatch}: the subclass supplies the strategy, so there is no + * separate handler object and no question of storing it in a field versus passing it per call. + * + *

Two states with hysteresis: closed (optimistic path) and engaged (guarded path). A failure + * of the declared type engages it, and {@link #closeAfter()} consecutive successes disengage it. + * It counts calls, not time, never rejects a call, and keeps plain racy state: a stale read costs + * one more pre-check or one more exception, never a wrong result. + * + *

Three hooks, all cheap to state: {@link #parse}, {@link #isDefinitelyInvalid} and {@link + * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryGetOrNull} converts the + * failure to {@code null} and never builds an exception while engaged. + * + * @param input type + * @param result type + * @param the failure this latch reacts to (unchecked here, to keep the sketch small) */ - abstract static class ParseHandler { - abstract boolean isDefinitelyInvalid(TInput input); - - abstract TOutput parse(TInput input); + abstract static class DynamicLatch { + private final Class failureType; + private int state; - abstract TException createFastFailException(); - } + DynamicLatch(Class failureType) { + this.failureType = failureType; + } - static final ParseHandler BASE64_PARSE_HANDLER = - new ParseHandler() { - @Override - boolean isDefinitelyInvalid(byte[] input) { - return !looksLikeBase64(input); - } + /** The optimistic parse. May throw {@code X} for bad input. */ + abstract O parse(I input); - @Override - String parse(byte[] input) { - return new String(Base64.getDecoder().decode(input), StandardCharsets.UTF_8); - } + /** A cheap, correct pre-check: true only if the input is definitely invalid. Never throws. */ + abstract boolean isDefinitelyInvalid(I input); - @Override - IllegalArgumentException createFastFailException() { - return new FastFailBase64Exception(); - } - }; - - // Strategy composed at construction time (stored as a field). Contrasted below with - // GenericBreakerParam, which takes the same strategy at call time instead — comparing them is - // the actual point, since a field forces C2 to also prove the receiver is constant before it - // can fold through to a concrete ParseHandler, while a call-time argument doesn't. - static final class GenericBreakerCtor { - private final ParseHandler handler; - int state; + /** A failure to throw while engaged. Must carry no stack trace, and must not be shared. */ + abstract X stacklessFailure(I input); - GenericBreakerCtor(ParseHandler handler) { - this.handler = handler; + /** How many consecutive successes disengage the latch. */ + int closeAfter() { + return CLOSE_THRESHOLD; } - TOutput decode(TInput input) { - if (state > 0 && handler.isDefinitelyInvalid(input)) { - throw handler.createFastFailException(); + /** Flow-through: the caller sees the failure, but while engaged it costs no stack trace. */ + final O get(I input) { + if (state > 0 && isDefinitelyInvalid(input)) { + throw stacklessFailure(input); } try { - TOutput result = handler.parse(input); + O result = parse(input); if (state > 0) { state--; } return result; } catch (RuntimeException e) { - state = CLOSE_THRESHOLD; + if (failureType.isInstance(e)) { + state = closeAfter(); + } throw e; } } - } - - // Same hysteresis, but the strategy is passed at call time rather than stored in a field. - static final class GenericBreakerParam { - int state; - TOutput decode(TInput input, ParseHandler handler) { - if (state > 0 && handler.isDefinitelyInvalid(input)) { - throw handler.createFastFailException(); + /** Converting: {@code null} for bad input. While engaged, no exception is built at all. */ + final O tryGetOrNull(I input) { + if (state > 0 && isDefinitelyInvalid(input)) { + return null; } try { - TOutput result = handler.parse(input); + O result = parse(input); if (state > 0) { state--; } return result; } catch (RuntimeException e) { - state = CLOSE_THRESHOLD; - throw e; + if (!failureType.isInstance(e)) { + throw e; + } + state = closeAfter(); + return null; } } } - final GenericBreakerCtor genericBreakerForValid = - new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); - final GenericBreakerCtor genericBreakerForInvalid = - new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); + /** The Base64 strategy. A named final class, so a field of this type has an exact type. */ + static final class Base64Latch extends DynamicLatch { + Base64Latch() { + super(IllegalArgumentException.class); + } + + @Override + String parse(byte[] input) { + return new String(Base64.getDecoder().decode(input), StandardCharsets.UTF_8); + } + + @Override + boolean isDefinitelyInvalid(byte[] input) { + return !looksLikeBase64(input); + } + + @Override + IllegalArgumentException stacklessFailure(byte[] input) { + return new FastFailBase64Exception(); + } + } + + final Breaker breakerForValid = new Breaker(); + final Breaker breakerForInvalid = new Breaker(); + final Breaker breakerThrowingForValid = new Breaker(); + final Breaker breakerThrowingForInvalid = new Breaker(); - final GenericBreakerParam genericBreakerParamForValid = - new GenericBreakerParam<>(); - final GenericBreakerParam - genericBreakerParamForInvalid = new GenericBreakerParam<>(); + // One latch per arm, each a static final of the exact type, as a call site would hold it. + static final Base64Latch LATCH_FLOW_VALID = new Base64Latch(); + static final Base64Latch LATCH_FLOW_INVALID = new Base64Latch(); + static final Base64Latch LATCH_CONVERT_VALID = new Base64Latch(); + static final Base64Latch LATCH_CONVERT_INVALID = new Base64Latch(); + static final Base64Latch LATCH_FLOW_MIX = new Base64Latch(); + static final Base64Latch LATCH_CONVERT_MIX = new Base64Latch(); + + /** Fails fast if the sketch does not behave as the benchmark assumes. */ + @Setup + public void checkTheSketchBehaves() { + Base64Latch latch = new Base64Latch(); + String expected = new String(Base64.getDecoder().decode(VALID), StandardCharsets.UTF_8); + if (!expected.equals(latch.get(VALID)) || !expected.equals(latch.tryGetOrNull(VALID))) { + throw new IllegalStateException("a valid value must decode"); + } + if (latch.tryGetOrNull(INVALID) != null) { + throw new IllegalStateException("bad input must convert to null"); + } + // the failure above engaged it: the flow-through flavor must now fail without a stack trace + try { + latch.get(INVALID); + throw new IllegalStateException("bad input must throw"); + } catch (FastFailBase64Exception e) { + if (e.getStackTrace().length != 0) { + throw new IllegalStateException("the stand-in must carry no stack trace"); + } + } + if (!expected.equals(latch.get(VALID))) { + throw new IllegalStateException("good input must still decode while engaged"); + } + } @Benchmark public void breakerValid(Blackhole bh) { @@ -245,31 +303,27 @@ public void breakerThrowingInvalid(Blackhole bh) { } @Benchmark - public void genericBreakerValid(Blackhole bh) { - bh.consume(genericBreakerForValid.decode(VALID)); + public void latchFlowValid(Blackhole bh) { + bh.consume(LATCH_FLOW_VALID.get(VALID)); } @Benchmark - public void genericBreakerInvalid(Blackhole bh) { + public void latchFlowInvalid(Blackhole bh) { try { - bh.consume(genericBreakerForInvalid.decode(INVALID)); + bh.consume(LATCH_FLOW_INVALID.get(INVALID)); } catch (IllegalArgumentException e) { bh.consume(e); } } @Benchmark - public void genericBreakerParamValid(Blackhole bh) { - bh.consume(genericBreakerParamForValid.decode(VALID, BASE64_PARSE_HANDLER)); + public void latchConvertValid(Blackhole bh) { + bh.consume(LATCH_CONVERT_VALID.tryGetOrNull(VALID)); } @Benchmark - public void genericBreakerParamInvalid(Blackhole bh) { - try { - bh.consume(genericBreakerParamForInvalid.decode(INVALID, BASE64_PARSE_HANDLER)); - } catch (IllegalArgumentException e) { - bh.consume(e); - } + public void latchConvertInvalid(Blackhole bh) { + bh.consume(LATCH_CONVERT_INVALID.tryGetOrNull(INVALID)); } @Benchmark @@ -293,7 +347,7 @@ public void precheckInvalid(Blackhole bh) { } // Deterministic stream: one INVALID every invalidEveryN calls, VALID otherwise. Each mix - // benchmark gets its own counter/breaker so the three strategies see the identical pattern. + // benchmark gets its own counter and state so the strategies see the identical pattern. @State(Scope.Thread) public static class Mix { @@ -303,10 +357,6 @@ public static class Mix { int counter; final Breaker breaker = new Breaker(); final Breaker throwingBreaker = new Breaker(); - final GenericBreakerCtor genericBreaker = - new GenericBreakerCtor<>(BASE64_PARSE_HANDLER); - final GenericBreakerParam genericBreakerParam = - new GenericBreakerParam<>(); byte[] next() { if (++counter >= invalidEveryN) { @@ -343,22 +393,17 @@ public void mixBreakerThrowing(Mix mix, Blackhole bh) { } @Benchmark - public void mixGenericBreaker(Mix mix, Blackhole bh) { + public void mixLatchFlow(Mix mix, Blackhole bh) { byte[] bytes = mix.next(); try { - bh.consume(mix.genericBreaker.decode(bytes)); + bh.consume(LATCH_FLOW_MIX.get(bytes)); } catch (IllegalArgumentException e) { bh.consume(e); } } @Benchmark - public void mixGenericBreakerParam(Mix mix, Blackhole bh) { - byte[] bytes = mix.next(); - try { - bh.consume(mix.genericBreakerParam.decode(bytes, BASE64_PARSE_HANDLER)); - } catch (IllegalArgumentException e) { - bh.consume(e); - } + public void mixLatchConvert(Mix mix, Blackhole bh) { + bh.consume(LATCH_CONVERT_MIX.tryGetOrNull(mix.next())); } } From 89ad7501c70132b0e52b480a76584fec474eb405 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 30 Sep 2026 17:07:34 -0400 Subject: [PATCH 03/12] Name the DynamicLatch hooks handle and isKnownToFail Rename the optimistic hook parse to handle, so it reads the same across the Latch family, and the pre-check isDefinitelyInvalid to isKnownToFail, which is not specific to parsers. Naming only. Co-Authored-By: Claude Sonnet 5.5 --- .../trace/api/FunctionsBase64Benchmark.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index 4d8a315a96f..45e9d90c56d 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -148,7 +148,7 @@ String decodeOrThrow(byte[] bytes) { * It counts calls, not time, never rejects a call, and keeps plain racy state: a stale read costs * one more pre-check or one more exception, never a wrong result. * - *

Three hooks, all cheap to state: {@link #parse}, {@link #isDefinitelyInvalid} and {@link + *

Three hooks, all cheap to state: {@link #handle}, {@link #isKnownToFail} and {@link * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryGetOrNull} converts the * failure to {@code null} and never builds an exception while engaged. * @@ -164,11 +164,15 @@ abstract static class DynamicLatch { this.failureType = failureType; } - /** The optimistic parse. May throw {@code X} for bad input. */ - abstract O parse(I input); + /** + * The operation, optimistically (for a parser, the parse). May throw {@code X} for bad input. + */ + abstract O handle(I input); - /** A cheap, correct pre-check: true only if the input is definitely invalid. Never throws. */ - abstract boolean isDefinitelyInvalid(I input); + /** + * A cheap, correct pre-check: true only if {@link #handle} would definitely fail. Never throws. + */ + abstract boolean isKnownToFail(I input); /** A failure to throw while engaged. Must carry no stack trace, and must not be shared. */ abstract X stacklessFailure(I input); @@ -180,11 +184,11 @@ int closeAfter() { /** Flow-through: the caller sees the failure, but while engaged it costs no stack trace. */ final O get(I input) { - if (state > 0 && isDefinitelyInvalid(input)) { + if (state > 0 && isKnownToFail(input)) { throw stacklessFailure(input); } try { - O result = parse(input); + O result = handle(input); if (state > 0) { state--; } @@ -199,11 +203,11 @@ final O get(I input) { /** Converting: {@code null} for bad input. While engaged, no exception is built at all. */ final O tryGetOrNull(I input) { - if (state > 0 && isDefinitelyInvalid(input)) { + if (state > 0 && isKnownToFail(input)) { return null; } try { - O result = parse(input); + O result = handle(input); if (state > 0) { state--; } @@ -225,12 +229,12 @@ static final class Base64Latch extends DynamicLatch Date: Wed, 30 Sep 2026 21:23:49 -0400 Subject: [PATCH 04/12] Add benchmark results for the Base64 guard and the DynamicLatch sketch Record a five-fork run on Zulu 17 (M1) in the benchmark's Javadoc: the single-input arms and the mixed-input arms at five invalid rates. The sketch matches the hand-written breaker; the adaptive form is a tradeoff against always pre-checking at high invalid rates. Co-Authored-By: Claude Sonnet 5.5 --- .../trace/api/FunctionsBase64Benchmark.java | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index 45e9d90c56d..773f627a39c 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -33,6 +33,41 @@ * never builds an exception at all while engaged. * *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. + * + *

Results, one run: Zulu 17.0.7 (HotSpot), MacBook M1, single thread, 5 forks, on a laptop with + * normal background activity (load about 4 to 7). JDK 8 and x86 are not measured. Error margins are + * below 4%. + * + * 

+ * ns/op                                valid input  invalid input
+ * unguarded (status quo)                      30.5          905.7
+ * always pre-check                            71.3           2.75
+ * hand-written Breaker, converting            31.2           2.73
+ * hand-written Breaker, throwing              31.4           11.6
+ * DynamicLatch.tryGetOrNull                   31.1           2.78
+ * DynamicLatch.get                            31.2           11.5
+ *
+ * Mix, ns/op, one invalid input in every N
+ *      N  unguarded  pre-check  Breaker  throwing  latch.get  tryGetOrNull
+ *      2      467.1       36.6     47.7      49.9       51.9          47.3
+ *     10      118.4       64.9     71.0      71.3       72.1          70.0
+ *    100       41.9       70.0     46.1      46.4       47.9          45.9
+ *   1000       41.6       70.3     44.7      45.7       45.1          45.6
+ *  10000       34.3       70.2     36.2      35.3       39.6          35.5
+ *
+ * + * The sketch matches the hand-written breaker: within 0.2 ns in the single-input arms, and within + * about 4 ns in the mixed ones (the largest gap is flow-through at one in 10,000, 39.6 ns against + * 35.3 ns). While engaged, the converting flavor costs about 2.8 ns where the status quo costs + * about 906 ns; the flow-through flavor costs about 11.5 ns, the price of building a stackless + * exception. Always pre-checking more than doubles the cost of valid input (71 ns against 30 ns), + * which is what the adaptive form avoids. + * + *

It is a tradeoff, not a free win. With one invalid input in 100 or rarer, the guard costs 1 to + * 4 ns over doing nothing and is about 24 to 34 ns cheaper than always pre-checking. With a high + * rate (one in 2 or one in 10), always pre-checking is faster than the adaptive guard (36.6 ns + * against about 47 to 52 ns at one in 2, and 65 ns against about 70 to 72 ns at one in 10). The + * cause was not investigated. */ @Fork(2) @Warmup(iterations = 3, time = 1) From 453da2d2640d442f0470ce1321dedc413158f14a Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 2 Oct 2026 08:45:08 -0400 Subject: [PATCH 05/12] Rename the DynamicLatch sketch to AdaptiveLatch and add a fallback Brings the sketch in line with Latch and ClassLatch: tryGetOrNull now returns fallback(input), null unless overridden, for a failed or known-to-fail input. Co-Authored-By: Claude Opus 5.5 --- .../trace/api/FunctionsBase64Benchmark.java | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index 773f627a39c..8a6ff6dcf89 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -23,13 +23,13 @@ * kicks in for this call site under sustained repeated throws, or whether the caught path pays for * a full stack trace fill-in every time. * - *

It also measures a reusable form of the guard. {@link DynamicLatch} is a local sketch of the + *

It also measures a reusable form of the guard. {@link AdaptiveLatch} is a local sketch of the * shape discussed in APMLP-1884: an abstract class whose subclass is the strategy (an * optimistic parse, a cheap correct pre-check, and a stackless failure), held in a {@code static * final}. It replaces an earlier pair of generic variants that stored the strategy in a field or * took it at call time, which are not needed once the subclass carries the hooks. It has two public - * flavors: {@link DynamicLatch#get} lets the failure flow to the caller (throwing a stackless - * stand-in while engaged), and {@link DynamicLatch#tryGetOrNull} converts it to {@code null} and + * flavors: {@link AdaptiveLatch#get} lets the failure flow to the caller (throwing a stackless + * stand-in while engaged), and {@link AdaptiveLatch#tryGetOrNull} converts it to {@code null} and * never builds an exception at all while engaged. * *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. @@ -44,8 +44,8 @@ * always pre-check 71.3 2.75 * hand-written Breaker, converting 31.2 2.73 * hand-written Breaker, throwing 31.4 11.6 - * DynamicLatch.tryGetOrNull 31.1 2.78 - * DynamicLatch.get 31.2 11.5 + * AdaptiveLatch.tryGetOrNull 31.1 2.78 + * AdaptiveLatch.get 31.2 11.5 * * Mix, ns/op, one invalid input in every N * N unguarded pre-check Breaker throwing latch.get tryGetOrNull @@ -61,7 +61,9 @@ * 35.3 ns). While engaged, the converting flavor costs about 2.8 ns where the status quo costs * about 906 ns; the flow-through flavor costs about 11.5 ns, the price of building a stackless * exception. Always pre-checking more than doubles the cost of valid input (71 ns against 30 ns), - * which is what the adaptive form avoids. + * which is what the adaptive form avoids. (Measured before {@code fallback} was added and the + * sketch renamed from {@code DynamicLatch}; the default {@code null} fallback is expected to inline + * away, but has not been re-measured.) * *

It is a tradeoff, not a free win. With one invalid input in 100 or rarer, the guard costs 1 to * 4 ns over doing nothing and is about 24 to 34 ns cheaper than always pre-checking. With a high @@ -175,7 +177,7 @@ String decodeOrThrow(byte[] bytes) { } /** - * Local sketch of a {@code DynamicLatch}: the subclass supplies the strategy, so there is no + * Local sketch of a {@code AdaptiveLatch}: the subclass supplies the strategy, so there is no * separate handler object and no question of storing it in a field versus passing it per call. * *

Two states with hysteresis: closed (optimistic path) and engaged (guarded path). A failure @@ -185,17 +187,18 @@ String decodeOrThrow(byte[] bytes) { * *

Three hooks, all cheap to state: {@link #handle}, {@link #isKnownToFail} and {@link * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryGetOrNull} converts the - * failure to {@code null} and never builds an exception while engaged. + * failure to {@link #fallback} and never builds an exception while engaged. {@link #fallback} is + * {@code null} unless overridden, as on {@code Latch} and {@code ClassLatch}. * * @param input type * @param result type * @param the failure this latch reacts to (unchecked here, to keep the sketch small) */ - abstract static class DynamicLatch { + abstract static class AdaptiveLatch { private final Class failureType; private int state; - DynamicLatch(Class failureType) { + AdaptiveLatch(Class failureType) { this.failureType = failureType; } @@ -212,6 +215,11 @@ abstract static class DynamicLatch { /** A failure to throw while engaged. Must carry no stack trace, and must not be shared. */ abstract X stacklessFailure(I input); + /** What a failed or known-to-fail input yields from {@link #tryGetOrNull}. */ + O fallback(I input) { + return null; + } + /** How many consecutive successes disengage the latch. */ int closeAfter() { return CLOSE_THRESHOLD; @@ -236,10 +244,10 @@ final O get(I input) { } } - /** Converting: {@code null} for bad input. While engaged, no exception is built at all. */ + /** Converting: {@link #fallback} for bad input. While engaged, no exception is built at all. */ final O tryGetOrNull(I input) { if (state > 0 && isKnownToFail(input)) { - return null; + return fallback(input); } try { O result = handle(input); @@ -252,13 +260,13 @@ final O tryGetOrNull(I input) { throw e; } state = closeAfter(); - return null; + return fallback(input); } } } /** The Base64 strategy. A named final class, so a field of this type has an exact type. */ - static final class Base64Latch extends DynamicLatch { + static final class Base64Latch extends AdaptiveLatch { Base64Latch() { super(IllegalArgumentException.class); } From de594c3b1cc7218ba034e2c939bf522c24c074ca Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 2 Oct 2026 09:52:53 -0400 Subject: [PATCH 06/12] Align AdaptiveLatch names with Latch and ClassLatch handle becomes apply and tryGetOrNull becomes tryApply. Also re-aligns the results table after the AdaptiveLatch rename. Co-Authored-By: Claude Opus 5.5 --- .../trace/api/FunctionsBase64Benchmark.java | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index 8a6ff6dcf89..a909e74d500 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -29,8 +29,8 @@ * final}. It replaces an earlier pair of generic variants that stored the strategy in a field or * took it at call time, which are not needed once the subclass carries the hooks. It has two public * flavors: {@link AdaptiveLatch#get} lets the failure flow to the caller (throwing a stackless - * stand-in while engaged), and {@link AdaptiveLatch#tryGetOrNull} converts it to {@code null} and - * never builds an exception at all while engaged. + * stand-in while engaged), and {@link AdaptiveLatch#tryApply} converts it to {@code null} and never + * builds an exception at all while engaged. * *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. * @@ -44,11 +44,11 @@ * always pre-check 71.3 2.75 * hand-written Breaker, converting 31.2 2.73 * hand-written Breaker, throwing 31.4 11.6 - * AdaptiveLatch.tryGetOrNull 31.1 2.78 - * AdaptiveLatch.get 31.2 11.5 + * AdaptiveLatch.tryApply 31.1 2.78 + * AdaptiveLatch.get 31.2 11.5 * * Mix, ns/op, one invalid input in every N - * N unguarded pre-check Breaker throwing latch.get tryGetOrNull + * N unguarded pre-check Breaker throwing latch.get tryApply * 2 467.1 36.6 47.7 49.9 51.9 47.3 * 10 118.4 64.9 71.0 71.3 72.1 70.0 * 100 41.9 70.0 46.1 46.4 47.9 45.9 @@ -61,9 +61,9 @@ * 35.3 ns). While engaged, the converting flavor costs about 2.8 ns where the status quo costs * about 906 ns; the flow-through flavor costs about 11.5 ns, the price of building a stackless * exception. Always pre-checking more than doubles the cost of valid input (71 ns against 30 ns), - * which is what the adaptive form avoids. (Measured before {@code fallback} was added and the - * sketch renamed from {@code DynamicLatch}; the default {@code null} fallback is expected to inline - * away, but has not been re-measured.) + * which is what the adaptive form avoids. (Measured before {@code fallback} was added, while the + * sketch was {@code DynamicLatch} and {@code tryApply} was {@code tryGetOrNull}; the default {@code + * null} fallback is expected to inline away, but has not been re-measured.) * *

It is a tradeoff, not a free win. With one invalid input in 100 or rarer, the guard costs 1 to * 4 ns over doing nothing and is about 24 to 34 ns cheaper than always pre-checking. With a high @@ -185,8 +185,8 @@ String decodeOrThrow(byte[] bytes) { * It counts calls, not time, never rejects a call, and keeps plain racy state: a stale read costs * one more pre-check or one more exception, never a wrong result. * - *

Three hooks, all cheap to state: {@link #handle}, {@link #isKnownToFail} and {@link - * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryGetOrNull} converts the + *

Three hooks, all cheap to state: {@link #apply}, {@link #isKnownToFail} and {@link + * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryApply} converts the * failure to {@link #fallback} and never builds an exception while engaged. {@link #fallback} is * {@code null} unless overridden, as on {@code Latch} and {@code ClassLatch}. * @@ -205,17 +205,17 @@ abstract static class AdaptiveLatch { /** * The operation, optimistically (for a parser, the parse). May throw {@code X} for bad input. */ - abstract O handle(I input); + abstract O apply(I input); /** - * A cheap, correct pre-check: true only if {@link #handle} would definitely fail. Never throws. + * A cheap, correct pre-check: true only if {@link #apply} would definitely fail. Never throws. */ abstract boolean isKnownToFail(I input); /** A failure to throw while engaged. Must carry no stack trace, and must not be shared. */ abstract X stacklessFailure(I input); - /** What a failed or known-to-fail input yields from {@link #tryGetOrNull}. */ + /** What a failed or known-to-fail input yields from {@link #tryApply}. */ O fallback(I input) { return null; } @@ -231,7 +231,7 @@ final O get(I input) { throw stacklessFailure(input); } try { - O result = handle(input); + O result = apply(input); if (state > 0) { state--; } @@ -245,12 +245,12 @@ final O get(I input) { } /** Converting: {@link #fallback} for bad input. While engaged, no exception is built at all. */ - final O tryGetOrNull(I input) { + final O tryApply(I input) { if (state > 0 && isKnownToFail(input)) { return fallback(input); } try { - O result = handle(input); + O result = apply(input); if (state > 0) { state--; } @@ -272,7 +272,7 @@ static final class Base64Latch extends AdaptiveLatch Date: Mon, 28 Sep 2026 19:27:43 -0400 Subject: [PATCH 07/12] Guard Kafka header Base64 decoding against repeated exception cost Customer (~180K/day) hits repeated IllegalArgumentException from Functions.BASE64_DECODE when a Kafka producer sends non-Base64 header values, paying full stack-trace fill-in on every throw. Adds a guarded decode path (GuardedBase64Decode) that fast-fails with a stack-trace-free exception once a real failure is seen, with hysteresis to re-check after a run of valid input, wired behind a new kafka.client.base64.decoding.guard.enabled config flag (default true). Co-Authored-By: Claude Sonnet 5 --- .../kafka_clients/TextMapExtractAdapter.java | 12 ++- .../TextMapExtractAdapter.java | 12 ++- .../config/TraceInstrumentationConfig.java | 2 + .../main/java/datadog/trace/api/Config.java | 10 ++ .../java/datadog/trace/api/Functions.java | 91 +++++++++++++++++++ .../trace/api/GuardedBase64DecodeTest.java | 86 ++++++++++++++++++ metadata/supported-configurations.json | 8 ++ 7 files changed, 217 insertions(+), 4 deletions(-) create mode 100644 internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java diff --git a/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java b/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java index 429038773ba..08d332a848a 100644 --- a/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java +++ b/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java @@ -6,6 +6,7 @@ import static datadog.trace.instrumentation.kafka_clients.KafkaDecorator.KAFKA_PRODUCED_KEY; import datadog.trace.api.Config; +import datadog.trace.api.Functions; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation.ContextVisitor; import java.nio.ByteBuffer; @@ -20,14 +21,21 @@ public class TextMapExtractAdapter implements ContextVisitor { private static final Logger log = LoggerFactory.getLogger(TextMapExtractAdapter.class); public static final TextMapExtractAdapter GETTER = - new TextMapExtractAdapter(Config.get().isKafkaClientBase64DecodingEnabled()); + new TextMapExtractAdapter( + Config.get().isKafkaClientBase64DecodingEnabled(), + Config.get().isKafkaClientBase64DecodingGuardEnabled()); private final Function headerValueTransformer; private final Base64.Decoder decoder; public TextMapExtractAdapter(boolean decodeBase64Headers) { + this(decodeBase64Headers, true); + } + + public TextMapExtractAdapter(boolean decodeBase64Headers, boolean guardEnabled) { if (decodeBase64Headers) { - this.headerValueTransformer = BASE64_DECODE; + this.headerValueTransformer = + guardEnabled ? new Functions.GuardedBase64Decode()::decodeOrNull : BASE64_DECODE; this.decoder = Base64.getDecoder(); } else { this.headerValueTransformer = UTF8_BYTES_TO_STRING; diff --git a/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java b/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java index 30b35216a4b..6e2f891830c 100644 --- a/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java +++ b/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java @@ -5,6 +5,7 @@ import static datadog.trace.api.telemetry.LogCollector.EXCLUDE_TELEMETRY; import datadog.trace.api.Config; +import datadog.trace.api.Functions; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation.ContextVisitor; import java.nio.ByteBuffer; @@ -19,14 +20,21 @@ public class TextMapExtractAdapter implements ContextVisitor { private static final Logger log = LoggerFactory.getLogger(TextMapExtractAdapter.class); public static final TextMapExtractAdapter GETTER = - new TextMapExtractAdapter(Config.get().isKafkaClientBase64DecodingEnabled()); + new TextMapExtractAdapter( + Config.get().isKafkaClientBase64DecodingEnabled(), + Config.get().isKafkaClientBase64DecodingGuardEnabled()); private final Function headerValueTransformer; private final Base64.Decoder decoder; public TextMapExtractAdapter(boolean decodeBase64Headers) { + this(decodeBase64Headers, true); + } + + public TextMapExtractAdapter(boolean decodeBase64Headers, boolean guardEnabled) { if (decodeBase64Headers) { - this.headerValueTransformer = BASE64_DECODE; + this.headerValueTransformer = + guardEnabled ? new Functions.GuardedBase64Decode()::decodeOrNull : BASE64_DECODE; this.decoder = Base64.getDecoder(); } else { this.headerValueTransformer = UTF8_BYTES_TO_STRING; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 137e9805519..86ccd4b9fdd 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -113,6 +113,8 @@ public final class TraceInstrumentationConfig { "kafka.client.propagation.disabled.topics"; public static final String KAFKA_CLIENT_BASE64_DECODING_ENABLED = "kafka.client.base64.decoding.enabled"; + public static final String KAFKA_CLIENT_BASE64_DECODING_GUARD_ENABLED = + "kafka.client.base64.decoding.guard.enabled"; public static final String JMS_PROPAGATION_DISABLED_TOPICS = "jms.propagation.disabled.topics"; public static final String JMS_PROPAGATION_DISABLED_QUEUES = "jms.propagation.disabled.queues"; diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index b1707b77a71..5194934ba10 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -623,6 +623,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.JMS_PROPAGATION_DISABLED_TOPICS; import static datadog.trace.api.config.TraceInstrumentationConfig.JMS_UNACKNOWLEDGED_MAX_AGE; import static datadog.trace.api.config.TraceInstrumentationConfig.KAFKA_CLIENT_BASE64_DECODING_ENABLED; +import static datadog.trace.api.config.TraceInstrumentationConfig.KAFKA_CLIENT_BASE64_DECODING_GUARD_ENABLED; import static datadog.trace.api.config.TraceInstrumentationConfig.KAFKA_CLIENT_PROPAGATION_DISABLED_TOPICS; import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_INJECTION; import static datadog.trace.api.config.TraceInstrumentationConfig.LOGS_INJECTION_ENABLED; @@ -1321,6 +1322,7 @@ public static String getHostName() { private final boolean kafkaClientPropagationEnabled; private final Set kafkaClientPropagationDisabledTopics; private final boolean kafkaClientBase64DecodingEnabled; + private final boolean kafkaClientBase64DecodingGuardEnabled; private final boolean jmsPropagationEnabled; private final Set jmsPropagationDisabledTopics; @@ -3156,6 +3158,8 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) tryMakeImmutableSet(configProvider.getList(KAFKA_CLIENT_PROPAGATION_DISABLED_TOPICS)); kafkaClientBase64DecodingEnabled = configProvider.getBoolean(KAFKA_CLIENT_BASE64_DECODING_ENABLED, false); + kafkaClientBase64DecodingGuardEnabled = + configProvider.getBoolean(KAFKA_CLIENT_BASE64_DECODING_GUARD_ENABLED, true); jmsPropagationEnabled = isPropagationEnabled(true, "jms"); jmsPropagationDisabledTopics = tryMakeImmutableSet(configProvider.getList(JMS_PROPAGATION_DISABLED_TOPICS)); @@ -5180,6 +5184,10 @@ public boolean isKafkaClientBase64DecodingEnabled() { return kafkaClientBase64DecodingEnabled; } + public boolean isKafkaClientBase64DecodingGuardEnabled() { + return kafkaClientBase64DecodingGuardEnabled; + } + public boolean isRabbitPropagationEnabled() { return rabbitPropagationEnabled; } @@ -6983,6 +6991,8 @@ public String toString() { + kafkaClientPropagationDisabledTopics + ", kafkaClientBase64DecodingEnabled=" + kafkaClientBase64DecodingEnabled + + ", kafkaClientBase64DecodingGuardEnabled=" + + kafkaClientBase64DecodingGuardEnabled + ", jmsPropagationEnabled=" + jmsPropagationEnabled + ", jmsPropagationDisabledTopics=" diff --git a/internal-api/src/main/java/datadog/trace/api/Functions.java b/internal-api/src/main/java/datadog/trace/api/Functions.java index 731cd711a9a..56f535e5175 100644 --- a/internal-api/src/main/java/datadog/trace/api/Functions.java +++ b/internal-api/src/main/java/datadog/trace/api/Functions.java @@ -188,4 +188,95 @@ public T apply(Object input) { return null; } }; + + /** + * Base64-decodes bytes that occasionally aren't valid Base64 at all (e.g. a misconfigured + * upstream producer sending mixed encodings), without paying for {@link Base64}'s decoder + * throwing and filling in a stack trace on every one of those failures. + * + *

Tracks a simple hysteresis "breaker": after a real decode failure it switches into a guarded + * state and cheaply pre-checks the Base64 alphabet before calling into {@link Base64}'s decoder, + * until enough consecutive inputs look valid again to close the guard. A precheck-detected + * failure throws {@link DefinitelyNotBase64Exception}, a stack-trace-free stand-in, so the {@link + * #decode} contract still matches {@link Base64}'s own (throws {@link IllegalArgumentException} + * on failure); a failure the precheck misses still throws the JDK's own exception, unmodified, + * with its real message and stack trace. + * + *

Not safe for concurrent use by multiple threads expecting independent guard state — the + * guard state is a plain (non-volatile, non-atomic) field on purpose: it's advisory hysteresis, + * not correctness-critical, so a stale read across threads just costs one extra precheck or one + * extra real exception. + */ + public static final class GuardedBase64Decode { + + private static final int CLOSE_THRESHOLD = 20; + private static final boolean[] BASE64_ALPHABET = buildAlphabetTable(); + + private static boolean[] buildAlphabetTable() { + boolean[] table = new boolean[256]; + String alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/="; + for (int i = 0; i < alphabet.length(); i++) { + table[alphabet.charAt(i)] = true; + } + return table; + } + + // Branch-free per byte on purpose: a data-dependent early exit helps the rare "bad byte + // early" case but hurts the common valid case, which always scans the whole buffer anyway. + private static boolean looksLikeBase64(byte[] bytes) { + if (bytes.length == 0 || (bytes.length & 3) != 0) { + return false; + } + boolean valid = true; + for (byte b : bytes) { + valid &= BASE64_ALPHABET[b & 0xFF]; + } + return valid; + } + + /** Thrown only when the alphabet precheck already knows decoding would fail. */ + public static final class DefinitelyNotBase64Exception extends IllegalArgumentException { + private static final String MESSAGE = "Input is not valid Base64"; + + DefinitelyNotBase64Exception() { + super(MESSAGE); + } + + // No caller needs a stack trace for a failure we already knew about before calling into + // Base64's decoder, so skip the walk that gives java.util.Base64's own exception its cost. + @Override + public synchronized Throwable fillInStackTrace() { + return this; + } + } + + // Single-word countdown: 0 == closed (no precheck), >0 == guarded, counting down to close. + private int state; + + /** Decodes {@code bytes}, throwing {@link IllegalArgumentException} on failure. */ + public String decode(byte[] bytes) { + if (state > 0 && !looksLikeBase64(bytes)) { + throw new DefinitelyNotBase64Exception(); + } + try { + String result = new String(Base64.getDecoder().decode(bytes), UTF_8); + if (state > 0) { + state--; + } + return result; + } catch (IllegalArgumentException e) { + state = CLOSE_THRESHOLD; + throw e; + } + } + + /** Decodes {@code bytes}, returning {@code null} on failure instead of throwing. */ + public String decodeOrNull(byte[] bytes) { + try { + return decode(bytes); + } catch (IllegalArgumentException e) { + return null; + } + } + } } diff --git a/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java new file mode 100644 index 00000000000..a8ddbde2442 --- /dev/null +++ b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java @@ -0,0 +1,86 @@ +package datadog.trace.api; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import datadog.trace.api.Functions.GuardedBase64Decode; +import datadog.trace.api.Functions.GuardedBase64Decode.DefinitelyNotBase64Exception; +import java.util.Base64; +import org.junit.jupiter.api.Test; + +class GuardedBase64DecodeTest { + + private static final byte[] VALID = + Base64.getEncoder().encode("x-datadog-trace-id".getBytes(UTF_8)); + private static final byte[] INVALID = "not-valid-base64!@#".getBytes(UTF_8); + + @Test + void decodeReturnsDecodedStringForValidInput() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertEquals("x-datadog-trace-id", decode.decode(VALID)); + } + + @Test + void decodeThrowsRealExceptionForInvalidInputWhenNotGuarded() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + // the guard hasn't seen a failure yet, so this must be the JDK's own exception, not our + // stack-trace-free stand-in + assertEquals(IllegalArgumentException.class, e.getClass()); + } + + @Test + void decodeThrowsFastFailStandInWhileGuarded() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + + DefinitelyNotBase64Exception e = + assertThrows(DefinitelyNotBase64Exception.class, () -> decode.decode(INVALID)); + assertEquals(0, e.getStackTrace().length); + } + + @Test + void decodeStillDecodesValidInputWhileGuarded() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + + assertEquals("x-datadog-trace-id", decode.decode(VALID)); + } + + @Test + void decodeClosesGuardAfterEnoughValidInput() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + + for (int i = 0; i < 50; i++) { + decode.decode(VALID); + } + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + // the guard has closed again, so this should be a real decode failure, not the stand-in + assertEquals(IllegalArgumentException.class, e.getClass()); + } + + @Test + void decodeOrNullReturnsDecodedStringForValidInput() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertEquals("x-datadog-trace-id", decode.decodeOrNull(VALID)); + } + + @Test + void decodeOrNullReturnsNullForInvalidInput() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertNull(decode.decodeOrNull(INVALID)); + } + + @Test + void decodeOrNullReturnsNullWhileGuarded() { + GuardedBase64Decode decode = new GuardedBase64Decode(); + assertNull(decode.decodeOrNull(INVALID)); + assertNull(decode.decodeOrNull(INVALID)); + } +} diff --git a/metadata/supported-configurations.json b/metadata/supported-configurations.json index e981287521e..11e06c79f45 100644 --- a/metadata/supported-configurations.json +++ b/metadata/supported-configurations.json @@ -2340,6 +2340,14 @@ "aliases": [] } ], + "DD_KAFKA_CLIENT_BASE64_DECODING_GUARD_ENABLED": [ + { + "version": "A", + "type": "boolean", + "default": "true", + "aliases": [] + } + ], "DD_KAFKA_CLIENT_PROPAGATION_DISABLED_TOPICS": [ { "version": "A", From 140a424d224e145541ac9f432414a8516509c141 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 5 Oct 2026 13:49:25 -0400 Subject: [PATCH 08/12] Move AdaptiveLatch into production and build the Kafka Base64 guard on it AdaptiveLatch moves from the benchmark into datadog.trace.util with only the converting form, tryApply; the flow-through get has no caller. Functions.GuardedBase64Decode becomes a subclass of it, so a malformed header no longer builds even a stackless exception while engaged. The pre-check no longer turns away unpadded or empty input: the basic decoder accepts both, so only a byte outside the alphabet, or a final unit of one character, is known to fail. Co-Authored-By: Claude Opus 5.5 --- .../kafka_clients/TextMapExtractAdapter.java | 2 +- .../TextMapExtractAdapter.java | 2 +- .../trace/api/FunctionsBase64Benchmark.java | 199 +++--------------- .../java/datadog/trace/api/Functions.java | 106 ++++------ .../datadog/trace/util/AdaptiveLatch.java | 95 +++++++++ .../trace/api/GuardedBase64DecodeTest.java | 107 ++++++---- .../datadog/trace/util/AdaptiveLatchTest.java | 185 ++++++++++++++++ 7 files changed, 415 insertions(+), 281 deletions(-) create mode 100644 internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java create mode 100644 internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java diff --git a/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java b/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java index 08d332a848a..b7aa2936643 100644 --- a/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java +++ b/dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/main/java/datadog/trace/instrumentation/kafka_clients/TextMapExtractAdapter.java @@ -35,7 +35,7 @@ public TextMapExtractAdapter(boolean decodeBase64Headers) { public TextMapExtractAdapter(boolean decodeBase64Headers, boolean guardEnabled) { if (decodeBase64Headers) { this.headerValueTransformer = - guardEnabled ? new Functions.GuardedBase64Decode()::decodeOrNull : BASE64_DECODE; + guardEnabled ? new Functions.GuardedBase64Decode()::tryApply : BASE64_DECODE; this.decoder = Base64.getDecoder(); } else { this.headerValueTransformer = UTF8_BYTES_TO_STRING; diff --git a/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java b/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java index 6e2f891830c..dd9e5683367 100644 --- a/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java +++ b/dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/main/java17/datadog/trace/instrumentation/kafka_clients38/TextMapExtractAdapter.java @@ -34,7 +34,7 @@ public TextMapExtractAdapter(boolean decodeBase64Headers) { public TextMapExtractAdapter(boolean decodeBase64Headers, boolean guardEnabled) { if (decodeBase64Headers) { this.headerValueTransformer = - guardEnabled ? new Functions.GuardedBase64Decode()::decodeOrNull : BASE64_DECODE; + guardEnabled ? new Functions.GuardedBase64Decode()::tryApply : BASE64_DECODE; this.decoder = Base64.getDecoder(); } else { this.headerValueTransformer = UTF8_BYTES_TO_STRING; diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index a909e74d500..e10bab8f792 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -2,6 +2,8 @@ import static datadog.trace.api.Functions.BASE64_DECODE; +import datadog.trace.api.Functions.GuardedBase64Decode; +import datadog.trace.util.AdaptiveLatch; import java.nio.charset.StandardCharsets; import java.util.Base64; import java.util.function.Function; @@ -23,14 +25,18 @@ * kicks in for this call site under sustained repeated throws, or whether the caught path pays for * a full stack trace fill-in every time. * - *

It also measures a reusable form of the guard. {@link AdaptiveLatch} is a local sketch of the - * shape discussed in APMLP-1884: an abstract class whose subclass is the strategy (an - * optimistic parse, a cheap correct pre-check, and a stackless failure), held in a {@code static - * final}. It replaces an earlier pair of generic variants that stored the strategy in a field or - * took it at call time, which are not needed once the subclass carries the hooks. It has two public - * flavors: {@link AdaptiveLatch#get} lets the failure flow to the caller (throwing a stackless - * stand-in while engaged), and {@link AdaptiveLatch#tryApply} converts it to {@code null} and never - * builds an exception at all while engaged. + *

It also measures the reusable form of the guard, {@link AdaptiveLatch} (APMLP-1884): an + * abstract class whose subclass is the strategy (an optimistic parse and a cheap, correct + * pre-check), held in a {@code static final}. The arms use the production subclass, {@link + * Functions.GuardedBase64Decode}. {@link AdaptiveLatch#tryApply} converts a failure to {@code null} + * and never builds an exception at all while engaged. + * + *

The numbers below were measured on an earlier, benchmark-local sketch of the latch, then named + * {@code DynamicLatch}, with {@code tryGetOrNull} for {@code tryApply}, before {@code fallback} was + * added and before the pre-check stopped rejecting unpadded input. The sketch also had a + * flow-through flavor, {@code get}, which threw a stackless stand-in while engaged; it was dropped + * when the latch moved into production because no caller needs it, and its arms were removed. The + * hand-written throwing {@link Breaker} still measures that shape. Not re-measured since. * *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. * @@ -44,8 +50,8 @@ * always pre-check 71.3 2.75 * hand-written Breaker, converting 31.2 2.73 * hand-written Breaker, throwing 31.4 11.6 - * AdaptiveLatch.tryApply 31.1 2.78 - * AdaptiveLatch.get 31.2 11.5 + * sketch, converting (tryApply) 31.1 2.78 + * sketch, flow-through (get) 31.2 11.5 * * Mix, ns/op, one invalid input in every N * N unguarded pre-check Breaker throwing latch.get tryApply @@ -61,9 +67,7 @@ * 35.3 ns). While engaged, the converting flavor costs about 2.8 ns where the status quo costs * about 906 ns; the flow-through flavor costs about 11.5 ns, the price of building a stackless * exception. Always pre-checking more than doubles the cost of valid input (71 ns against 30 ns), - * which is what the adaptive form avoids. (Measured before {@code fallback} was added, while the - * sketch was {@code DynamicLatch} and {@code tryApply} was {@code tryGetOrNull}; the default {@code - * null} fallback is expected to inline away, but has not been re-measured.) + * which is what the adaptive form avoids. * *

It is a tradeoff, not a free win. With one invalid input in 100 or rarer, the guard costs 1 to * 4 ns over doing nothing and is about 24 to 34 ns cheaper than always pre-checking. With a high @@ -176,151 +180,28 @@ String decodeOrThrow(byte[] bytes) { } } - /** - * Local sketch of a {@code AdaptiveLatch}: the subclass supplies the strategy, so there is no - * separate handler object and no question of storing it in a field versus passing it per call. - * - *

Two states with hysteresis: closed (optimistic path) and engaged (guarded path). A failure - * of the declared type engages it, and {@link #closeAfter()} consecutive successes disengage it. - * It counts calls, not time, never rejects a call, and keeps plain racy state: a stale read costs - * one more pre-check or one more exception, never a wrong result. - * - *

Three hooks, all cheap to state: {@link #apply}, {@link #isKnownToFail} and {@link - * #stacklessFailure}. The last is needed only by {@link #get}; {@link #tryApply} converts the - * failure to {@link #fallback} and never builds an exception while engaged. {@link #fallback} is - * {@code null} unless overridden, as on {@code Latch} and {@code ClassLatch}. - * - * @param input type - * @param result type - * @param the failure this latch reacts to (unchecked here, to keep the sketch small) - */ - abstract static class AdaptiveLatch { - private final Class failureType; - private int state; - - AdaptiveLatch(Class failureType) { - this.failureType = failureType; - } - - /** - * The operation, optimistically (for a parser, the parse). May throw {@code X} for bad input. - */ - abstract O apply(I input); - - /** - * A cheap, correct pre-check: true only if {@link #apply} would definitely fail. Never throws. - */ - abstract boolean isKnownToFail(I input); - - /** A failure to throw while engaged. Must carry no stack trace, and must not be shared. */ - abstract X stacklessFailure(I input); - - /** What a failed or known-to-fail input yields from {@link #tryApply}. */ - O fallback(I input) { - return null; - } - - /** How many consecutive successes disengage the latch. */ - int closeAfter() { - return CLOSE_THRESHOLD; - } - - /** Flow-through: the caller sees the failure, but while engaged it costs no stack trace. */ - final O get(I input) { - if (state > 0 && isKnownToFail(input)) { - throw stacklessFailure(input); - } - try { - O result = apply(input); - if (state > 0) { - state--; - } - return result; - } catch (RuntimeException e) { - if (failureType.isInstance(e)) { - state = closeAfter(); - } - throw e; - } - } - - /** Converting: {@link #fallback} for bad input. While engaged, no exception is built at all. */ - final O tryApply(I input) { - if (state > 0 && isKnownToFail(input)) { - return fallback(input); - } - try { - O result = apply(input); - if (state > 0) { - state--; - } - return result; - } catch (RuntimeException e) { - if (!failureType.isInstance(e)) { - throw e; - } - state = closeAfter(); - return fallback(input); - } - } - } - - /** The Base64 strategy. A named final class, so a field of this type has an exact type. */ - static final class Base64Latch extends AdaptiveLatch { - Base64Latch() { - super(IllegalArgumentException.class); - } - - @Override - String apply(byte[] input) { - return new String(Base64.getDecoder().decode(input), StandardCharsets.UTF_8); - } - - @Override - boolean isKnownToFail(byte[] input) { - return !looksLikeBase64(input); - } - - @Override - IllegalArgumentException stacklessFailure(byte[] input) { - return new FastFailBase64Exception(); - } - } - final Breaker breakerForValid = new Breaker(); final Breaker breakerForInvalid = new Breaker(); final Breaker breakerThrowingForValid = new Breaker(); final Breaker breakerThrowingForInvalid = new Breaker(); // One latch per arm, each a static final of the exact type, as a call site would hold it. - static final Base64Latch LATCH_FLOW_VALID = new Base64Latch(); - static final Base64Latch LATCH_FLOW_INVALID = new Base64Latch(); - static final Base64Latch LATCH_CONVERT_VALID = new Base64Latch(); - static final Base64Latch LATCH_CONVERT_INVALID = new Base64Latch(); - static final Base64Latch LATCH_FLOW_MIX = new Base64Latch(); - static final Base64Latch LATCH_CONVERT_MIX = new Base64Latch(); - - /** Fails fast if the sketch does not behave as the benchmark assumes. */ + static final GuardedBase64Decode LATCH_CONVERT_VALID = new GuardedBase64Decode(); + static final GuardedBase64Decode LATCH_CONVERT_INVALID = new GuardedBase64Decode(); + static final GuardedBase64Decode LATCH_CONVERT_MIX = new GuardedBase64Decode(); + + /** Fails fast if the latch does not behave as the benchmark assumes. */ @Setup - public void checkTheSketchBehaves() { - Base64Latch latch = new Base64Latch(); + public void checkTheLatchBehaves() { + GuardedBase64Decode latch = new GuardedBase64Decode(); String expected = new String(Base64.getDecoder().decode(VALID), StandardCharsets.UTF_8); - if (!expected.equals(latch.get(VALID)) || !expected.equals(latch.tryApply(VALID))) { + if (!expected.equals(latch.tryApply(VALID))) { throw new IllegalStateException("a valid value must decode"); } - if (latch.tryApply(INVALID) != null) { - throw new IllegalStateException("bad input must convert to null"); - } - // the failure above engaged it: the flow-through flavor must now fail without a stack trace - try { - latch.get(INVALID); - throw new IllegalStateException("bad input must throw"); - } catch (FastFailBase64Exception e) { - if (e.getStackTrace().length != 0) { - throw new IllegalStateException("the stand-in must carry no stack trace"); - } + if (latch.tryApply(INVALID) != null || !latch.isEngaged()) { + throw new IllegalStateException("bad input must convert to null and engage the latch"); } - if (!expected.equals(latch.get(VALID))) { + if (!expected.equals(latch.tryApply(VALID))) { throw new IllegalStateException("good input must still decode while engaged"); } } @@ -349,20 +230,6 @@ public void breakerThrowingInvalid(Blackhole bh) { } } - @Benchmark - public void latchFlowValid(Blackhole bh) { - bh.consume(LATCH_FLOW_VALID.get(VALID)); - } - - @Benchmark - public void latchFlowInvalid(Blackhole bh) { - try { - bh.consume(LATCH_FLOW_INVALID.get(INVALID)); - } catch (IllegalArgumentException e) { - bh.consume(e); - } - } - @Benchmark public void latchConvertValid(Blackhole bh) { bh.consume(LATCH_CONVERT_VALID.tryApply(VALID)); @@ -439,16 +306,6 @@ public void mixBreakerThrowing(Mix mix, Blackhole bh) { } } - @Benchmark - public void mixLatchFlow(Mix mix, Blackhole bh) { - byte[] bytes = mix.next(); - try { - bh.consume(LATCH_FLOW_MIX.get(bytes)); - } catch (IllegalArgumentException e) { - bh.consume(e); - } - } - @Benchmark public void mixLatchConvert(Mix mix, Blackhole bh) { bh.consume(LATCH_CONVERT_MIX.tryApply(mix.next())); diff --git a/internal-api/src/main/java/datadog/trace/api/Functions.java b/internal-api/src/main/java/datadog/trace/api/Functions.java index 56f535e5175..7f2b87af8e2 100644 --- a/internal-api/src/main/java/datadog/trace/api/Functions.java +++ b/internal-api/src/main/java/datadog/trace/api/Functions.java @@ -4,6 +4,7 @@ import static java.util.function.Function.identity; import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString; +import datadog.trace.util.AdaptiveLatch; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; @@ -190,26 +191,23 @@ public T apply(Object input) { }; /** - * Base64-decodes bytes that occasionally aren't valid Base64 at all (e.g. a misconfigured - * upstream producer sending mixed encodings), without paying for {@link Base64}'s decoder - * throwing and filling in a stack trace on every one of those failures. + * Base64-decodes bytes that are occasionally not Base64 at all, for example header values from a + * misconfigured producer that mixes encodings, without paying for {@link Base64}'s decoder to + * throw, and fill in a stack trace, on every one of them. Returns {@code null} for input that + * does not decode, like {@link #BASE64_DECODE}. * - *

Tracks a simple hysteresis "breaker": after a real decode failure it switches into a guarded - * state and cheaply pre-checks the Base64 alphabet before calling into {@link Base64}'s decoder, - * until enough consecutive inputs look valid again to close the guard. A precheck-detected - * failure throws {@link DefinitelyNotBase64Exception}, a stack-trace-free stand-in, so the {@link - * #decode} contract still matches {@link Base64}'s own (throws {@link IllegalArgumentException} - * on failure); a failure the precheck misses still throws the JDK's own exception, unmodified, - * with its real message and stack trace. + *

After a real decode failure, input is pre-checked until {@link #closeAfter} consecutive + * inputs decode again (see {@link AdaptiveLatch}). The pre-check turns away only input the + * decoder would definitely reject: a byte outside the Base64 alphabet, or a length that leaves a + * single character in the final unit. Padding is optional for {@link Base64#getDecoder()}, so + * unpadded and empty input are never turned away. * - *

Not safe for concurrent use by multiple threads expecting independent guard state — the - * guard state is a plain (non-volatile, non-atomic) field on purpose: it's advisory hysteresis, - * not correctness-critical, so a stale read across threads just costs one extra precheck or one - * extra real exception. + *

One instance can be shared across threads: its state is advisory, so a stale read costs one + * extra pre-check or one extra real failure, never a wrong result. */ - public static final class GuardedBase64Decode { - - private static final int CLOSE_THRESHOLD = 20; + public static final class GuardedBase64Decode + extends AdaptiveLatch { + /** The basic Base64 alphabet, plus the padding character. */ private static final boolean[] BASE64_ALPHABET = buildAlphabetTable(); private static boolean[] buildAlphabetTable() { @@ -221,62 +219,36 @@ private static boolean[] buildAlphabetTable() { return table; } - // Branch-free per byte on purpose: a data-dependent early exit helps the rare "bad byte - // early" case but hurts the common valid case, which always scans the whole buffer anyway. - private static boolean looksLikeBase64(byte[] bytes) { - if (bytes.length == 0 || (bytes.length & 3) != 0) { - return false; - } - boolean valid = true; - for (byte b : bytes) { - valid &= BASE64_ALPHABET[b & 0xFF]; - } - return valid; + public GuardedBase64Decode() { + super(IllegalArgumentException.class); } - /** Thrown only when the alphabet precheck already knows decoding would fail. */ - public static final class DefinitelyNotBase64Exception extends IllegalArgumentException { - private static final String MESSAGE = "Input is not valid Base64"; - - DefinitelyNotBase64Exception() { - super(MESSAGE); - } - - // No caller needs a stack trace for a failure we already knew about before calling into - // Base64's decoder, so skip the walk that gives java.util.Base64's own exception its cost. - @Override - public synchronized Throwable fillInStackTrace() { - return this; - } + @Override + protected String apply(byte[] bytes) { + return new String(Base64.getDecoder().decode(bytes), UTF_8); } - // Single-word countdown: 0 == closed (no precheck), >0 == guarded, counting down to close. - private int state; - - /** Decodes {@code bytes}, throwing {@link IllegalArgumentException} on failure. */ - public String decode(byte[] bytes) { - if (state > 0 && !looksLikeBase64(bytes)) { - throw new DefinitelyNotBase64Exception(); - } - try { - String result = new String(Base64.getDecoder().decode(bytes), UTF_8); - if (state > 0) { - state--; - } - return result; - } catch (IllegalArgumentException e) { - state = CLOSE_THRESHOLD; - throw e; + @Override + protected boolean isKnownToFail(byte[] bytes) { + return isDefinitelyNotBase64(bytes); + } + + /** + * Whether {@link Base64#getDecoder()} would definitely reject {@code bytes}. Lenient on + * purpose: misplaced padding, for example, is left for the decoder to reject. + */ + static boolean isDefinitelyNotBase64(byte[] bytes) { + // a final unit of one character can never be decoded, however it is padded + if ((bytes.length & 3) == 1) { + return true; } - } - - /** Decodes {@code bytes}, returning {@code null} on failure instead of throwing. */ - public String decodeOrNull(byte[] bytes) { - try { - return decode(bytes); - } catch (IllegalArgumentException e) { - return null; + // branch-free per byte on purpose: an early exit helps only the rare bad-byte-early case, + // and good input always scans the whole buffer anyway + boolean valid = true; + for (byte b : bytes) { + valid &= BASE64_ALPHABET[b & 0xFF]; } + return !valid; } } } diff --git a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java new file mode 100644 index 00000000000..b0b05f72dc1 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java @@ -0,0 +1,95 @@ +package datadog.trace.util; + +import javax.annotation.Nullable; + +/** + * A self-resetting latch for an operation whose failure depends on the input, such as parsing a + * value from a producer that sometimes sends garbage. Unlike {@code Latch} and {@link ClassLatch}, + * it never skips the operation outright: a bad input says nothing about the next one. Instead, a + * failure engages a cheap pre-check, {@link #isKnownToFail}, so that further bad input is turned + * away before the operation pays for throwing, and {@link #closeAfter} consecutive successes + * disengage it again. Disengaged, the only cost is one plain field read. + * + *

Intended as a {@code static final} field of a named final subclass, one per call site: the + * receiver is then a constant of a known exact type, so the JIT can inline the hooks. The subclass + * is the strategy: {@link #apply} does the operation optimistically and may throw {@code X} for bad + * input, and {@link #isKnownToFail} must be correct, returning {@code true} only for input that + * {@link #apply} would definitely reject. A pre-check that is too strict turns away good input; one + * that is too lenient only costs a real failure, which re-arms the latch. + * + *

{@link #fallback} is what a failed or turned-away input yields: {@code null} unless + * overridden, as on {@code Latch} and {@link ClassLatch}. + * + *

This is a hint, not a lock. The state is a plain counter, and it counts calls, not time. A + * stale or lost update only costs one more pre-check or one more real failure, never a wrong + * result. + * + * @param the type of the value the operation is applied to + * @param the type of the result + * @param the failure that engages the latch; anything else propagates unchanged + */ +public abstract class AdaptiveLatch { + /** Consecutive successes that disengage the latch, unless {@link #closeAfter} is overridden. */ + public static final int DEFAULT_CLOSE_AFTER = 20; + + private final Class failureType; + + /** 0 when disengaged; otherwise the successes left before disengaging. */ + private int remaining; + + protected AdaptiveLatch(Class failureType) { + this.failureType = failureType; + } + + /** Performs the operation optimistically. May throw {@code X} for bad input. */ + @Nullable + protected abstract R apply(T input); + + /** + * A cheap pre-check, consulted only while engaged: {@code true} only if {@link #apply} would + * definitely fail for {@code input}. Must not throw. + */ + protected abstract boolean isKnownToFail(T input); + + /** What a failed or turned-away input yields. {@code null} unless overridden. */ + @Nullable + protected R fallback(T input) { + return null; + } + + /** How many consecutive successes disengage the latch. */ + protected int closeAfter() { + return DEFAULT_CLOSE_AFTER; + } + + /** + * Performs the operation, returning {@link #fallback} if it fails with {@code X}. While engaged, + * input that {@link #isKnownToFail} is turned away without performing the operation, so no + * exception is built at all. + */ + @Nullable + public final R tryApply(T input) { + final int remaining = this.remaining; + if (remaining > 0 && isKnownToFail(input)) { + return fallback(input); + } + try { + final R result = apply(input); + if (remaining > 0) { + this.remaining = remaining - 1; + } + return result; + } catch (RuntimeException e) { + if (!failureType.isInstance(e)) { + throw e; + } + this.remaining = closeAfter(); + return fallback(input); + } + } + + /** Returns whether the pre-check is currently consulted. */ + public final boolean isEngaged() { + return remaining > 0; + } +} diff --git a/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java index a8ddbde2442..85cea677396 100644 --- a/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java +++ b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java @@ -2,13 +2,16 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.trace.api.Functions.GuardedBase64Decode; -import datadog.trace.api.Functions.GuardedBase64Decode.DefinitelyNotBase64Exception; +import datadog.trace.util.AdaptiveLatch; import java.util.Base64; import org.junit.jupiter.api.Test; +import org.tabletest.junit.TableTest; class GuardedBase64DecodeTest { @@ -16,71 +19,93 @@ class GuardedBase64DecodeTest { Base64.getEncoder().encode("x-datadog-trace-id".getBytes(UTF_8)); private static final byte[] INVALID = "not-valid-base64!@#".getBytes(UTF_8); - @Test - void decodeReturnsDecodedStringForValidInput() { + private static byte[] bytes(String s) { + return s.getBytes(UTF_8); + } + + private static GuardedBase64Decode engaged() { GuardedBase64Decode decode = new GuardedBase64Decode(); - assertEquals("x-datadog-trace-id", decode.decode(VALID)); + assertNull(decode.tryApply(INVALID)); + assertTrue(decode.isEngaged()); + return decode; } @Test - void decodeThrowsRealExceptionForInvalidInputWhenNotGuarded() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); - // the guard hasn't seen a failure yet, so this must be the JDK's own exception, not our - // stack-trace-free stand-in - assertEquals(IllegalArgumentException.class, e.getClass()); + void decodesValidInput() { + assertEquals("x-datadog-trace-id", new GuardedBase64Decode().tryApply(VALID)); } @Test - void decodeThrowsFastFailStandInWhileGuarded() { + void invalidInputYieldsNullAndEngages() { GuardedBase64Decode decode = new GuardedBase64Decode(); - assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); - DefinitelyNotBase64Exception e = - assertThrows(DefinitelyNotBase64Exception.class, () -> decode.decode(INVALID)); - assertEquals(0, e.getStackTrace().length); + assertNull(decode.tryApply(INVALID)); + assertTrue(decode.isEngaged()); } @Test - void decodeStillDecodesValidInputWhileGuarded() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + void invalidInputStillYieldsNullWhileEngaged() { + GuardedBase64Decode decode = engaged(); - assertEquals("x-datadog-trace-id", decode.decode(VALID)); + assertNull(decode.tryApply(INVALID)); } @Test - void decodeClosesGuardAfterEnoughValidInput() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); + void validInputStillDecodesWhileEngaged() { + GuardedBase64Decode decode = engaged(); - for (int i = 0; i < 50; i++) { - decode.decode(VALID); - } + assertEquals("x-datadog-trace-id", decode.tryApply(VALID)); + } - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> decode.decode(INVALID)); - // the guard has closed again, so this should be a real decode failure, not the stand-in - assertEquals(IllegalArgumentException.class, e.getClass()); + @TableTest({ + "scenario | input | expected", + "empty | '' | '' ", + "two-char unit | YQ | a ", + "three-char unit | YWI | ab ", + "padded | 'YQ==' | a " + }) + void unpaddedAndEmptyInputIsNotTurnedAwayWhileEngaged(String input, String expected) { + // Base64.getDecoder() accepts input without padding, so the pre-check must not reject it + GuardedBase64Decode decode = engaged(); + + assertEquals(expected, decode.tryApply(bytes(input))); } @Test - void decodeOrNullReturnsDecodedStringForValidInput() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - assertEquals("x-datadog-trace-id", decode.decodeOrNull(VALID)); + void disengagesAfterEnoughValidInput() { + GuardedBase64Decode decode = engaged(); + + for (int i = 0; i < AdaptiveLatch.DEFAULT_CLOSE_AFTER; i++) { + decode.tryApply(VALID); + } + + assertFalse(decode.isEngaged()); } @Test - void decodeOrNullReturnsNullForInvalidInput() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - assertNull(decode.decodeOrNull(INVALID)); + void otherFailuresPropagate() { + assertThrows(NullPointerException.class, () -> new GuardedBase64Decode().tryApply(null)); } - @Test - void decodeOrNullReturnsNullWhileGuarded() { - GuardedBase64Decode decode = new GuardedBase64Decode(); - assertNull(decode.decodeOrNull(INVALID)); - assertNull(decode.decodeOrNull(INVALID)); + @TableTest({ + "scenario | input | knownToFail", + "empty | '' | false ", + "two-char unit | YQ | false ", + "three-char unit | YWI | false ", + "full unit | YWJj | false ", + "padded | 'YQ==' | false ", + "misplaced padding is lenient | 'Y=Q=' | false ", + "one-char unit | Y | true ", + "one-char final unit | YWJjZ | true ", + "space | 'YW Jj' | true ", + "punctuation | not-valid-base64! | true ", + "url-safe alphabet | 'YQ-_' | true " + }) + void preCheckRejectsOnlyWhatTheDecoderWould(String input, boolean knownToFail) { + assertEquals(knownToFail, GuardedBase64Decode.isDefinitelyNotBase64(bytes(input))); + if (knownToFail) { + // the pre-check is only allowed to turn away input the decoder really rejects + assertThrows(IllegalArgumentException.class, () -> Base64.getDecoder().decode(input)); + } } } diff --git a/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java new file mode 100644 index 00000000000..af1070f2134 --- /dev/null +++ b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java @@ -0,0 +1,185 @@ +package datadog.trace.util; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.jupiter.api.Test; + +class AdaptiveLatchTest { + + /** + * Parses an int. "bad" is known to fail; "sneaky" also fails but passes the pre-check, as input + * the pre-check is too lenient for would. + */ + private static final class Parsing extends AdaptiveLatch { + final AtomicInteger applies = new AtomicInteger(); + final AtomicInteger checks = new AtomicInteger(); + + Parsing() { + super(NumberFormatException.class); + } + + @Override + protected Integer apply(String input) { + applies.incrementAndGet(); + if ("boom".equals(input)) { + throw new IllegalStateException("boom"); + } + return Integer.parseInt(input); + } + + @Override + protected boolean isKnownToFail(String input) { + checks.incrementAndGet(); + return "bad".equals(input); + } + + @Override + protected int closeAfter() { + return 3; + } + } + + @Test + void disengagedItAppliesEverythingWithoutPreChecking() { + Parsing latch = new Parsing(); + + assertEquals(42, latch.tryApply("42")); + assertEquals(7, latch.tryApply("7")); + + assertEquals(2, latch.applies.get()); + assertEquals(0, latch.checks.get(), "the pre-check is only consulted while engaged"); + assertFalse(latch.isEngaged()); + } + + @Test + void aFailureYieldsTheFallbackAndEngages() { + Parsing latch = new Parsing(); + + assertNull(latch.tryApply("bad")); + + assertEquals(1, latch.applies.get(), "disengaged, even known-bad input is attempted"); + assertTrue(latch.isEngaged()); + } + + @Test + void engagedItTurnsAwayKnownBadInputWithoutApplying() { + Parsing latch = new Parsing(); + latch.tryApply("bad"); + + assertNull(latch.tryApply("bad")); + assertNull(latch.tryApply("bad")); + + assertEquals(1, latch.applies.get()); + assertTrue(latch.isEngaged(), "turned-away input does not count towards disengaging"); + } + + @Test + void engagedItStillAppliesInputThatPassesThePreCheck() { + Parsing latch = new Parsing(); + latch.tryApply("bad"); + + assertEquals(42, latch.tryApply("42")); + assertEquals(2, latch.applies.get()); + } + + @Test + void disengagesAfterEnoughConsecutiveSuccesses() { + Parsing latch = new Parsing(); + latch.tryApply("bad"); + + latch.tryApply("1"); + latch.tryApply("2"); + assertTrue(latch.isEngaged()); + latch.tryApply("3"); + + assertFalse(latch.isEngaged()); + } + + @Test + void aFailureThePreCheckMissesReArmsTheLatch() { + Parsing latch = new Parsing(); + latch.tryApply("bad"); + latch.tryApply("1"); + latch.tryApply("2"); + + // fails, but the pre-check is too lenient to know it + assertNull(latch.tryApply("sneaky")); + + latch.tryApply("3"); + latch.tryApply("4"); + assertTrue(latch.isEngaged(), "the countdown restarted from closeAfter"); + } + + @Test + void otherExceptionsPropagateWithoutEngaging() { + Parsing latch = new Parsing(); + + assertThrows(IllegalStateException.class, () -> latch.tryApply("boom")); + assertFalse(latch.isEngaged()); + } + + @Test + void fallbackIsUsedForFailedAndTurnedAwayInputButNotARealNull() { + AdaptiveLatch latch = + new AdaptiveLatch( + IllegalArgumentException.class) { + @Override + protected String apply(String input) { + if (input.isEmpty()) { + throw new IllegalArgumentException(); + } + return "null".equals(input) ? null : input; + } + + @Override + protected boolean isKnownToFail(String input) { + return input.isEmpty(); + } + + @Override + protected String fallback(String input) { + return "fallback"; + } + }; + + // failed + assertEquals("fallback", latch.tryApply("")); + // turned away while engaged + assertEquals("fallback", latch.tryApply("")); + // an operation that succeeds with null is not a failure + assertNull(latch.tryApply("null")); + } + + @Test + void closeAfterDefaultsToTheDefaultConstant() { + AdaptiveLatch latch = + new AdaptiveLatch( + IllegalArgumentException.class) { + @Override + protected String apply(String input) { + if (input.isEmpty()) { + throw new IllegalArgumentException(); + } + return input; + } + + @Override + protected boolean isKnownToFail(String input) { + return input.isEmpty(); + } + }; + latch.tryApply(""); + + for (int i = 1; i < AdaptiveLatch.DEFAULT_CLOSE_AFTER; i++) { + latch.tryApply("ok"); + } + assertTrue(latch.isEngaged()); + latch.tryApply("ok"); + assertFalse(latch.isEngaged()); + } +} From 6397d3108b0d60f7e13a4b58a81c0d7571e7d44d Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 5 Oct 2026 14:17:18 -0400 Subject: [PATCH 09/12] Add AdaptiveLatchBenchmark Measures the primitive itself, on Integer.parseInt rather than Base64: the status quo, an always-on pre-check, and the latch disengaged, engaged and in a mixed stream, at stack depth 0 and 50. Records one run on Zulu 17, and narrows AdaptiveLatch's Javadoc to what was measured: the disengaged path is not free, and the latch only helps while bad input arrives more often than once per closeAfter calls. Co-Authored-By: Claude Opus 5.5 --- .../trace/util/AdaptiveLatchBenchmark.java | 271 ++++++++++++++++++ .../datadog/trace/util/AdaptiveLatch.java | 6 +- 2 files changed, 276 insertions(+), 1 deletion(-) create mode 100644 internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java diff --git a/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java new file mode 100644 index 00000000000..4d15ed908e2 --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java @@ -0,0 +1,271 @@ +package datadog.trace.util; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * What {@link AdaptiveLatch#tryApply} costs and saves for an operation whose failure depends on the + * input. The operation is {@link Integer#parseInt}, whose {@link NumberFormatException} fills in a + * full stack trace; the pre-check is a digit scan, which is correct (it flags only input {@code + * parseInt} rejects) but not complete (it lets an overflowing number through). + * + *

    + *
  • {@code unguarded*}: the status quo -- parse, and catch the exception for bad input. + *
  • {@code precheck*}: the non-adaptive alternative -- always scan before parsing. + *
  • {@code latchGood}: a disengaged latch on good input. The difference from {@code + * unguardedGood} is the latch's overhead on the path that works. + *
  • {@code latchEngagedGood}: good input while engaged, so it pays for the pre-check too. The + * latch is pinned engaged ({@code closeAfter} is {@link Integer#MAX_VALUE}) so that the state + * holds for the whole run. + *
  • {@code latchBad}: bad input while engaged, the steady state for a producer that keeps + * sending garbage: turned away without parsing. + *
  • {@code mix*}: one bad input in every {@code invalidEveryN}, with the default {@code + * closeAfter}, so the latch engages, disengages and re-engages as it would in production. + *
+ * + *

The cost of a throw grows with the depth of the stack it fills in, which is why {@code depth} + * is a parameter: a benchmark thread's stack is shallow, a request thread's is not. Each arm + * descends on its own, and each has its own {@code static final} latch, so that no arm's profile is + * shaped by another's. + * + *

Run with {@code ./gradlew :internal-api:jmh -Pjmh.includes=AdaptiveLatchBenchmark + * -Pjmh.profilers=gc}. + * + *

Results, one run: Zulu 17.0.7 (HotSpot), MacBook M1, single thread, 2 forks of 5 one-second + * iterations, on a laptop with normal background activity. JDK 8 and x86 are not measured. ns/op is + * derived from ops/s; B/op is from {@code -prof gc}. Good input allocates 16 B for the boxed result + * in every arm. + * + * 

+ * ns/op (B/op)          depth 0          depth 50
+ * unguardedGood         12.7  (16)        31.7  (16)
+ * precheckGood          15.8  (16)        40.1  (16)
+ * latchGood             15.7  (16)        36.2  (16)
+ * latchEngagedGood      11.3  (16)        40.3  (16)
+ * unguardedBad         908    (880)     2403    (2240)
+ * precheckBad            2.79  (0)        25.6   (0)
+ * latchBad               2.57  (0)        31.2   (0)
+ *
+ * Mix, ns/op, one bad input in every N
+ *           depth 0                         depth 50
+ *      N    unguarded  precheck  latch      unguarded  precheck  latch
+ *      2        452       9.85    31.9         1238      33.7     98.7
+ *    100       18.1      14.1     19.7         55.3      37.0     62.5
+ *  10000       13.5      13.8     11.9         31.7      38.8     38.6
+ *
+ * + * Engaged, bad input costs about 2.6 ns where the status quo costs about 908 ns (31 ns against 2.4 + * us at depth 50), and allocates nothing where the status quo allocates 880 B (2,240 B). + * + *

Disengaged, the latch is not free on good input: about 3 ns over {@code unguardedGood} at + * depth 0 and about 4.5 ns at depth 50, more than one field read should cost. The depth-0 + * good-input arms have errors of 9 to 11%, which is also why {@code latchEngagedGood} appears + * faster than {@code latchGood} there; the depth-50 gap is outside the error (2 to 3%). The cause + * was not investigated. + * + *

The mix shows when the latch pays off. At one bad input in 2 it is about 14 times cheaper than + * the status quo, though always pre-checking is cheaper still. At one in 100 it saves nothing and + * costs a little: with the default {@code closeAfter} of 20, it disengages between bad inputs, so + * every bad input still throws. It only helps while bad input arrives more often than once per + * {@code closeAfter} calls; at one in 10,000 the three are within a few ns of each other. + */ +@Fork(2) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@Threads(1) +@State(Scope.Benchmark) +public class AdaptiveLatchBenchmark { + + static final String GOOD = "1234567"; + static final String BAD = "12x4567"; + + @Param({"0", "50"}) + int depth; + + /** Parses an int; the pre-check flags anything that is not all ASCII digits. */ + static class Parse extends AdaptiveLatch { + Parse() { + super(NumberFormatException.class); + } + + @Override + protected Integer apply(String input) { + return Integer.parseInt(input); + } + + @Override + protected boolean isKnownToFail(String input) { + return !isAllDigits(input); + } + } + + /** Never disengages, so that the engaged state holds for a whole run of good input. */ + static final class PinnedParse extends Parse { + @Override + protected int closeAfter() { + return Integer.MAX_VALUE; + } + } + + static boolean isAllDigits(String input) { + if (input.isEmpty()) { + return false; + } + // branch-free per char, as a pre-check over good input always scans it all anyway + boolean digits = true; + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + digits &= c >= '0' & c <= '9'; + } + return digits; + } + + static Integer unguarded(String input) { + try { + return Integer.parseInt(input); + } catch (NumberFormatException e) { + return null; + } + } + + static Integer precheck(String input) { + return isAllDigits(input) ? Integer.parseInt(input) : null; + } + + static final Parse LATCH_GOOD = new Parse(); + static final PinnedParse LATCH_ENGAGED_GOOD = new PinnedParse(); + static final Parse LATCH_BAD = new Parse(); + static final Parse LATCH_MIX = new Parse(); + + /** Puts each latch in the state its arm measures, and fails fast if one does not get there. */ + @Setup + public void setup() { + if (LATCH_GOOD.tryApply(GOOD) == null || LATCH_GOOD.isEngaged()) { + throw new IllegalStateException("good input must parse without engaging"); + } + LATCH_ENGAGED_GOOD.tryApply(BAD); + if (!LATCH_ENGAGED_GOOD.isEngaged() || LATCH_ENGAGED_GOOD.tryApply(GOOD) == null) { + throw new IllegalStateException("good input must still parse while engaged"); + } + LATCH_BAD.tryApply(BAD); + if (!LATCH_BAD.isEngaged() || LATCH_BAD.tryApply(BAD) != null) { + throw new IllegalStateException("bad input must engage the latch and be turned away"); + } + } + + @Benchmark + public Object unguardedGood() { + return unguardedGood(depth); + } + + @Benchmark + public Object precheckGood() { + return precheckGood(depth); + } + + @Benchmark + public Object latchGood() { + return latchGood(depth); + } + + @Benchmark + public Object latchEngagedGood() { + return latchEngagedGood(depth); + } + + @Benchmark + public Object unguardedBad() { + return unguardedBad(depth); + } + + @Benchmark + public Object precheckBad() { + return precheckBad(depth); + } + + @Benchmark + public Object latchBad() { + return latchBad(depth); + } + + /** + * A deterministic stream: one {@link #BAD} in every {@code invalidEveryN}, else {@link #GOOD}. + */ + @State(Scope.Thread) + public static class Mix { + @Param({"2", "100", "10000"}) + int invalidEveryN; + + int counter; + + String next() { + if (++counter >= invalidEveryN) { + counter = 0; + return BAD; + } + return GOOD; + } + } + + @Benchmark + public Object mixUnguarded(Mix mix) { + return mixUnguarded(depth, mix.next()); + } + + @Benchmark + public Object mixPrecheck(Mix mix) { + return mixPrecheck(depth, mix.next()); + } + + @Benchmark + public Object mixLatch(Mix mix) { + return mixLatch(depth, mix.next()); + } + + private Object unguardedGood(int remaining) { + return remaining > 0 ? unguardedGood(remaining - 1) : unguarded(GOOD); + } + + private Object precheckGood(int remaining) { + return remaining > 0 ? precheckGood(remaining - 1) : precheck(GOOD); + } + + private Object latchGood(int remaining) { + return remaining > 0 ? latchGood(remaining - 1) : LATCH_GOOD.tryApply(GOOD); + } + + private Object latchEngagedGood(int remaining) { + return remaining > 0 ? latchEngagedGood(remaining - 1) : LATCH_ENGAGED_GOOD.tryApply(GOOD); + } + + private Object unguardedBad(int remaining) { + return remaining > 0 ? unguardedBad(remaining - 1) : unguarded(BAD); + } + + private Object precheckBad(int remaining) { + return remaining > 0 ? precheckBad(remaining - 1) : precheck(BAD); + } + + private Object latchBad(int remaining) { + return remaining > 0 ? latchBad(remaining - 1) : LATCH_BAD.tryApply(BAD); + } + + private Object mixUnguarded(int remaining, String input) { + return remaining > 0 ? mixUnguarded(remaining - 1, input) : unguarded(input); + } + + private Object mixPrecheck(int remaining, String input) { + return remaining > 0 ? mixPrecheck(remaining - 1, input) : precheck(input); + } + + private Object mixLatch(int remaining, String input) { + return remaining > 0 ? mixLatch(remaining - 1, input) : LATCH_MIX.tryApply(input); + } +} diff --git a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java index b0b05f72dc1..f22aec700ba 100644 --- a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java +++ b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java @@ -8,7 +8,11 @@ * it never skips the operation outright: a bad input says nothing about the next one. Instead, a * failure engages a cheap pre-check, {@link #isKnownToFail}, so that further bad input is turned * away before the operation pays for throwing, and {@link #closeAfter} consecutive successes - * disengage it again. Disengaged, the only cost is one plain field read. + * disengage it again. Disengaged, it adds one plain field read to the operation's own work; {@code + * AdaptiveLatchBenchmark} measures what that costs. + * + *

It only helps while bad input arrives more often than once per {@link #closeAfter} calls. + * Spaced further apart, the latch disengages between them and every bad input still throws. * *

Intended as a {@code static final} field of a named final subclass, one per call site: the * receiver is then a constant of a known exact type, so the JIT can inline the hooks. The subclass From 70784c11a280bb3ba78a0e5ce752ed12c9c34845 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 5 Oct 2026 15:25:23 -0400 Subject: [PATCH 10/12] Switch the Base64 guard between the JDK decoder and an exception-free one AdaptiveLatch now switches between two paths instead of guarding one with a pre-check: apply, the optimistic path, which may throw, and applySafely, a cautious path that must not, which reports bad input by returning reject(input). A pre-check is one way to build the cautious path; an exception-free implementation is another. closeAfter becomes abstract, and its Javadoc gives the rent-or-buy rule for choosing it. GuardedBase64Decode's cautious path is now decodeOrNull, an exception-free decoder that accepts exactly what Base64.getDecoder() accepts, checked against it by differential tests on JDK 8, 17 and 25. It allocates its output only once the first unit is clean, so input that is bad from the start costs no allocation. closeAfter is 64, from Base64DecodeBenchmark's costs on JDK 17. Co-Authored-By: Claude Opus 5.5 --- .../trace/api/Base64DecodeBenchmark.java | 137 ++++++++++++++++ .../trace/api/FunctionsBase64Benchmark.java | 20 +-- .../trace/util/AdaptiveLatchBenchmark.java | 16 +- .../java/datadog/trace/api/Functions.java | 124 +++++++++++---- .../datadog/trace/util/AdaptiveLatch.java | 97 +++++++----- .../trace/api/GuardedBase64DecodeTest.java | 147 +++++++++++++----- .../datadog/trace/util/AdaptiveLatchTest.java | 120 +++++++------- 7 files changed, 478 insertions(+), 183 deletions(-) create mode 100644 internal-api/src/jmh/java/datadog/trace/api/Base64DecodeBenchmark.java diff --git a/internal-api/src/jmh/java/datadog/trace/api/Base64DecodeBenchmark.java b/internal-api/src/jmh/java/datadog/trace/api/Base64DecodeBenchmark.java new file mode 100644 index 00000000000..c7197995615 --- /dev/null +++ b/internal-api/src/jmh/java/datadog/trace/api/Base64DecodeBenchmark.java @@ -0,0 +1,137 @@ +package datadog.trace.api; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import datadog.trace.api.Functions.GuardedBase64Decode; +import java.util.Base64; +import java.util.Random; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * {@link GuardedBase64Decode#decodeOrNull}, the exception-free decoder, against {@link + * Base64#getDecoder()}. These are the two costs that set the guard's {@code closeAfter}: the + * exception-free decoder's overhead on valid input (the cost of staying engaged), and the JDK + * decoder's failure on invalid input (the cost of disengaging too early). + * + *

    + *
  • {@code size}: {@code header} is a trace-header-sized value, {@code long} is about 1.4 KB of + * Base64, where the JDK's block decoding, and any intrinsic for it, has room to pay off. + *
  • {@code depth}: the stack the JDK decoder's exception fills in; a consumer thread's stack is + * deeper than a benchmark thread's. + *
+ * + *

Run with {@code ./gradlew :internal-api:jmh -Pjmh.includes=Base64DecodeBenchmark + * -Pjmh.profilers=gc}, and with {@code -PtestJvm=17} for a newer JDK. + * + *

Results, one run each: Zulu 8.0.382 and Zulu 17.0.7 (HotSpot), MacBook M1, single thread, 2 + * forks of 5 one-second iterations, on a laptop with normal background activity. x86 is not + * measured. ns/op is derived from ops/s; B/op is from {@code -prof gc}. + * + * 

+ * ns/op (B/op)                  JDK 8                  JDK 17
+ *                          header      long       header      long
+ * jdkValid,  depth 0       87.7 (160)  3631       53.4 (104)  1598
+ * exceptionFreeValid       86.3 (160)  3904       80.3 (104)  3072
+ * jdkInvalid, depth 0       889 (928)   926        921 (1024)  956
+ * jdkInvalid, depth 50     2080 (1904) 2189       2498 (2384) 2512
+ * exceptionFreeInvalid      6.4 (0)     6.4        6.2 (0)     6.3
+ *
+ * + * On JDK 8 the exception-free decoder is about as fast as the JDK's on header-sized input, and + * about 7% slower on long input. On JDK 17 the JDK's decoder, which decodes in blocks and has an + * intrinsic on this platform, is about 27 ns faster on header-sized input and about twice as fast + * on long input; that difference is why the guard switches between the two rather than always using + * the exception-free one. Invalid input costs the exception-free decoder about 6 ns and no + * allocation, at any length, where the JDK's costs about 0.9 us, or 2.1 to 2.5 us at depth 50. + * + *

The exception-free decoder allocates its output only once the first unit is clean. Against a + * copy that allocated up front, in the same run, that cost about 7 ns (JDK 17) to 12 ns (JDK 8) on + * valid header-sized input at depth 0, and nothing measurable at depth 50 or on long input, while + * saving 2 to 4 ns and 40 B on invalid header-sized input and about 55 ns and 1 KB on invalid long + * input. It runs only while the guard is engaged. The {@code jdk*} rows are from an earlier run of + * the same day, the {@code exceptionFree*} rows from the later one. + */ +@Fork(2) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +@Threads(1) +@State(Scope.Benchmark) +public class Base64DecodeBenchmark { + + @Param({"header", "long"}) + String size; + + @Param({"0", "50"}) + int depth; + + byte[] valid; + byte[] invalid; + + @Setup + public void setup() { + byte[] data; + if ("header".equals(size)) { + data = "1234567890123456789".getBytes(UTF_8); + } else { + data = new byte[1024]; + new Random(12672).nextBytes(data); + } + valid = Base64.getEncoder().encode(data); + // a plain-text value where Base64 was expected, bad from its fourth byte + invalid = valid.clone(); + invalid[3] = '-'; + String expected = new String(data, UTF_8); + if (!expected.equals(GuardedBase64Decode.decodeOrNull(valid)) + || !expected.equals(jdk(valid)) + || GuardedBase64Decode.decodeOrNull(invalid) != null + || jdk(invalid) != null) { + throw new IllegalStateException("the two decoders must agree on the benchmark inputs"); + } + } + + static String jdk(byte[] src) { + try { + return new String(Base64.getDecoder().decode(src), UTF_8); + } catch (IllegalArgumentException e) { + return null; + } + } + + @Benchmark + public Object jdkValid() { + return jdkAt(depth, valid); + } + + @Benchmark + public Object exceptionFreeValid() { + return exceptionFreeAt(depth, valid); + } + + @Benchmark + public Object jdkInvalid() { + return jdkAt(depth, invalid); + } + + @Benchmark + public Object exceptionFreeInvalid() { + return exceptionFreeAt(depth, invalid); + } + + private static Object jdkAt(int remaining, byte[] src) { + return remaining > 0 ? jdkAt(remaining - 1, src) : jdk(src); + } + + private static Object exceptionFreeAt(int remaining, byte[] src) { + return remaining > 0 + ? exceptionFreeAt(remaining - 1, src) + : GuardedBase64Decode.decodeOrNull(src); + } +} diff --git a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java index e10bab8f792..c9e1ec14b9f 100644 --- a/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/api/FunctionsBase64Benchmark.java @@ -26,17 +26,19 @@ * a full stack trace fill-in every time. * *

It also measures the reusable form of the guard, {@link AdaptiveLatch} (APMLP-1884): an - * abstract class whose subclass is the strategy (an optimistic parse and a cheap, correct - * pre-check), held in a {@code static final}. The arms use the production subclass, {@link - * Functions.GuardedBase64Decode}. {@link AdaptiveLatch#tryApply} converts a failure to {@code null} - * and never builds an exception at all while engaged. + * abstract class whose subclass is the strategy, held in a {@code static final}. The arms + * use the production subclass, {@link Functions.GuardedBase64Decode}, whose cautious path is an + * exception-free decoder; {@code Base64DecodeBenchmark} compares that decoder with the JDK's + * directly. {@link AdaptiveLatch#tryApply} converts a failure to {@code null} and never builds an + * exception at all while engaged. * *

The numbers below were measured on an earlier, benchmark-local sketch of the latch, then named - * {@code DynamicLatch}, with {@code tryGetOrNull} for {@code tryApply}, before {@code fallback} was - * added and before the pre-check stopped rejecting unpadded input. The sketch also had a - * flow-through flavor, {@code get}, which threw a stackless stand-in while engaged; it was dropped - * when the latch moved into production because no caller needs it, and its arms were removed. The - * hand-written throwing {@link Breaker} still measures that shape. Not re-measured since. + * {@code DynamicLatch}, with {@code tryGetOrNull} for {@code tryApply}, while its engaged path was + * an alphabet pre-check in front of the JDK decoder, before {@code fallback} was added and before + * the pre-check stopped rejecting unpadded input. The sketch also had a flow-through flavor, {@code + * get}, which threw a stackless stand-in while engaged; it was dropped when the latch moved into + * production because no caller needs it, and its arms were removed. The hand-written throwing + * {@link Breaker} still measures that shape. Not re-measured since. * *

The hand-written {@link Breaker} is the specialized baseline the latch is compared against. * diff --git a/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java index 4d15ed908e2..0d69d744fa3 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java @@ -38,6 +38,10 @@ *

Run with {@code ./gradlew :internal-api:jmh -Pjmh.includes=AdaptiveLatchBenchmark * -Pjmh.profilers=gc}. * + *

The latch here takes the pre-check form of a cautious path: the digit scan, then {@code + * parseInt} only if it passes. The results were recorded when that was the latch's only form; it + * does the same work now. + * *

Results, one run: Zulu 17.0.7 (HotSpot), MacBook M1, single thread, 2 forks of 5 one-second * iterations, on a laptop with normal background activity. JDK 8 and x86 are not measured. ns/op is * derived from ops/s; B/op is from {@code -prof gc}. Good input allocates 16 B for the boxed result @@ -89,7 +93,7 @@ public class AdaptiveLatchBenchmark { @Param({"0", "50"}) int depth; - /** Parses an int; the pre-check flags anything that is not all ASCII digits. */ + /** Parses an int; while engaged, a digit scan turns away anything that is not plain digits. */ static class Parse extends AdaptiveLatch { Parse() { super(NumberFormatException.class); @@ -101,8 +105,14 @@ protected Integer apply(String input) { } @Override - protected boolean isKnownToFail(String input) { - return !isAllDigits(input); + protected Integer applySafely(String input) { + return isAllDigits(input) ? Integer.parseInt(input) : reject(input); + } + + /** The value the recorded results were measured with; not tuned for this operation. */ + @Override + protected int closeAfter() { + return 20; } } diff --git a/internal-api/src/main/java/datadog/trace/api/Functions.java b/internal-api/src/main/java/datadog/trace/api/Functions.java index 7f2b87af8e2..fab4a4e86c7 100644 --- a/internal-api/src/main/java/datadog/trace/api/Functions.java +++ b/internal-api/src/main/java/datadog/trace/api/Functions.java @@ -8,10 +8,12 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; +import java.util.Arrays; import java.util.Base64; import java.util.Locale; import java.util.function.BiFunction; import java.util.function.Function; +import javax.annotation.Nullable; public final class Functions { @@ -196,26 +198,29 @@ public T apply(Object input) { * throw, and fill in a stack trace, on every one of them. Returns {@code null} for input that * does not decode, like {@link #BASE64_DECODE}. * - *

After a real decode failure, input is pre-checked until {@link #closeAfter} consecutive - * inputs decode again (see {@link AdaptiveLatch}). The pre-check turns away only input the - * decoder would definitely reject: a byte outside the Base64 alphabet, or a length that leaves a - * single character in the final unit. Padding is optional for {@link Base64#getDecoder()}, so - * unpadded and empty input are never turned away. + *

Decodes with {@link Base64#getDecoder()} until it fails, then with {@link + * #decodeOrNull(byte[])}, an exception-free decoder that accepts exactly what {@link + * Base64#getDecoder()} accepts, until {@link #closeAfter} consecutive inputs decode again (see + * {@link AdaptiveLatch}). * *

One instance can be shared across threads: its state is advisory, so a stale read costs one - * extra pre-check or one extra real failure, never a wrong result. + * extra cautious decode or one extra real failure, never a wrong result. */ public static final class GuardedBase64Decode extends AdaptiveLatch { - /** The basic Base64 alphabet, plus the padding character. */ - private static final boolean[] BASE64_ALPHABET = buildAlphabetTable(); + /** Each byte's 6-bit value in the basic Base64 alphabet, {@code -2} for padding, else -1. */ + private static final byte[] FROM_BASE64 = buildDecodeTable(); - private static boolean[] buildAlphabetTable() { - boolean[] table = new boolean[256]; - String alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/="; + private static final int PADDING = -2; + + private static byte[] buildDecodeTable() { + byte[] table = new byte[256]; + Arrays.fill(table, (byte) -1); + String alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; for (int i = 0; i < alphabet.length(); i++) { - table[alphabet.charAt(i)] = true; + table[alphabet.charAt(i)] = (byte) i; } + table['='] = PADDING; return table; } @@ -229,26 +234,93 @@ protected String apply(byte[] bytes) { } @Override - protected boolean isKnownToFail(byte[] bytes) { - return isDefinitelyNotBase64(bytes); + protected String applySafely(byte[] bytes) { + String decoded = decodeOrNull(bytes); + return decoded != null ? decoded : reject(bytes); + } + + @Override + protected int closeAfter() { + return CLOSE_AFTER; } /** - * Whether {@link Base64#getDecoder()} would definitely reject {@code bytes}. Lenient on - * purpose: misplaced padding, for example, is left for the decoder to reject. + * The rent-or-buy break-even (see {@link AdaptiveLatch}), from {@code Base64DecodeBenchmark} on + * JDK 17 for header-sized values: a JDK decoder failure costs about 913 ns more than a + * rejection here at stack depth 0 and about 2,468 ns at depth 50, and on valid input this + * decoder costs about 27 ns more than the JDK's, giving 34 to 91. A consumer's stack is deeper + * than a benchmark's. On JDK 8 the two decoders cost about the same on valid input, so the + * value barely matters there. */ - static boolean isDefinitelyNotBase64(byte[] bytes) { - // a final unit of one character can never be decoded, however it is padded - if ((bytes.length & 3) == 1) { - return true; + private static final int CLOSE_AFTER = 64; + + /** + * Decodes {@code src} as {@link Base64#getDecoder()} would, returning {@code null} where it + * would throw. Follows the same rules: padding is optional, but if present must complete the + * final unit, and nothing may follow it; a final unit of one character is rejected. + */ + @Nullable + static String decodeOrNull(byte[] src) { + final int length = src.length; + if (length == 0) { + return ""; + } + // allocated up front only if the first unit is clean, so input that is bad from the start + // costs no allocation; input that goes bad later still does, since one pass cannot know in + // advance. If the first unit is not clean, no unit can complete before the loop meets its + // bad or padding byte, so the loop never writes to a null dst. + byte[] dst = + length >= 4 + && (FROM_BASE64[src[0] & 0xFF] + | FROM_BASE64[src[1] & 0xFF] + | FROM_BASE64[src[2] & 0xFF] + | FROM_BASE64[src[3] & 0xFF]) + >= 0 + ? new byte[3 * ((length + 3) / 4)] + : null; + int dp = 0; + int bits = 0; + // the bit position of the next character within its 4-character unit + int shift = 18; + int sp = 0; + while (sp < length) { + final int b = FROM_BASE64[src[sp++] & 0xFF]; + if (b < 0) { + // padding is only legal after two or three characters of a unit, and after two it + // must be doubled + if (b != PADDING || shift == 18 || (shift == 6 && (sp == length || src[sp++] != '='))) { + return null; + } + break; + } + bits |= b << shift; + shift -= 6; + if (shift < 0) { + dst[dp++] = (byte) (bits >> 16); + dst[dp++] = (byte) (bits >> 8); + dst[dp++] = (byte) bits; + shift = 18; + bits = 0; + } + } + // a dangling single character, or anything after the padding + if (shift == 12 || sp < length) { + return null; + } + if (shift != 18) { + if (dst == null) { + // no full unit: at most two bytes + dst = new byte[2]; + } + dst[dp++] = (byte) (bits >> 16); + if (shift == 0) { + dst[dp++] = (byte) (bits >> 8); + } } - // branch-free per byte on purpose: an early exit helps only the rare bad-byte-early case, - // and good input always scans the whole buffer anyway - boolean valid = true; - for (byte b : bytes) { - valid &= BASE64_ALPHABET[b & 0xFF]; + if (dst == null) { + return ""; } - return !valid; + return new String(dst, 0, dp, UTF_8); } } } diff --git a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java index f22aec700ba..799b2a377fe 100644 --- a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java +++ b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java @@ -3,96 +3,107 @@ import javax.annotation.Nullable; /** - * A self-resetting latch for an operation whose failure depends on the input, such as parsing a - * value from a producer that sometimes sends garbage. Unlike {@code Latch} and {@link ClassLatch}, - * it never skips the operation outright: a bad input says nothing about the next one. Instead, a - * failure engages a cheap pre-check, {@link #isKnownToFail}, so that further bad input is turned - * away before the operation pays for throwing, and {@link #closeAfter} consecutive successes - * disengage it again. Disengaged, it adds one plain field read to the operation's own work; {@code - * AdaptiveLatchBenchmark} measures what that costs. + * A self-resetting switch between two ways of performing an operation whose failure depends on the + * input, such as parsing a value from a producer that sometimes sends garbage: an optimistic path, + * {@link #apply}, that is fastest on good input but throws {@code X} on bad input, and a cautious + * path, {@link #applySafely}, that never throws {@code X}. Unlike {@code Latch} and {@link + * ClassLatch}, it never skips the operation: a bad input says nothing about the next one. * - *

It only helps while bad input arrives more often than once per {@link #closeAfter} calls. - * Spaced further apart, the latch disengages between them and every bad input still throws. + *

Disengaged, every call takes the optimistic path. A failure there engages the latch, and from + * then on calls take the cautious path until {@link #closeAfter} consecutive inputs pass it + * cleanly. The cautious path reports bad input by returning {@link #reject}, which restarts that + * count; anything else it returns counts as clean. * - *

Intended as a {@code static final} field of a named final subclass, one per call site: the - * receiver is then a constant of a known exact type, so the JIT can inline the hooks. The subclass - * is the strategy: {@link #apply} does the operation optimistically and may throw {@code X} for bad - * input, and {@link #isKnownToFail} must be correct, returning {@code true} only for input that - * {@link #apply} would definitely reject. A pre-check that is too strict turns away good input; one - * that is too lenient only costs a real failure, which re-arms the latch. + *

The cautious path can be built in several ways: + * + *

    + *
  • an exception-free implementation of the same operation; + *
  • a cheap, correct pre-check in front of the optimistic path: {@code return isKnownBad(input) + * ? reject(input) : apply(input);} + *
  • a repair that turns common bad input into good input before the optimistic path. + *
* - *

{@link #fallback} is what a failed or turned-away input yields: {@code null} unless - * overridden, as on {@code Latch} and {@link ClassLatch}. + *

Choosing {@link #closeAfter} is a rent-or-buy decision. Staying engaged costs the cautious + * path's overhead on every good input; disengaging costs one optimistic failure when the next bad + * input arrives. Disengaging once the accumulated overhead would match one failure, that is after + * about {@code failureCost / cautiousOverhead} good inputs, is never worse than twice the best + * possible schedule, whatever the input. Measure both costs at a realistic stack depth: the cost of + * a throw grows with the stack it fills in. + * + *

Intended as a {@code static final} field of a named final subclass, one per call site: the + * receiver is then a constant of a known exact type, so the JIT can inline the hooks. Disengaged, + * the latch adds one plain field read to the optimistic path. * *

This is a hint, not a lock. The state is a plain counter, and it counts calls, not time. A - * stale or lost update only costs one more pre-check or one more real failure, never a wrong - * result. + * stale or lost update only costs one more cautious call or one more optimistic failure, never a + * wrong result. * * @param the type of the value the operation is applied to * @param the type of the result * @param the failure that engages the latch; anything else propagates unchanged */ public abstract class AdaptiveLatch { - /** Consecutive successes that disengage the latch, unless {@link #closeAfter} is overridden. */ - public static final int DEFAULT_CLOSE_AFTER = 20; - private final Class failureType; - /** 0 when disengaged; otherwise the successes left before disengaging. */ + /** 0 when disengaged; otherwise the clean calls left before disengaging. */ private int remaining; protected AdaptiveLatch(Class failureType) { this.failureType = failureType; } - /** Performs the operation optimistically. May throw {@code X} for bad input. */ + /** The optimistic path: fastest on good input, but may throw {@code X} for bad input. */ @Nullable protected abstract R apply(T input); /** - * A cheap pre-check, consulted only while engaged: {@code true} only if {@link #apply} would - * definitely fail for {@code input}. Must not throw. + * The cautious path: must not throw {@code X}. Returns {@link #reject} for bad input; any other + * return counts as clean, including {@code null}. */ - protected abstract boolean isKnownToFail(T input); + @Nullable + protected abstract R applySafely(T input); + + /** How many consecutive clean calls disengage the latch; see the class comment. */ + protected abstract int closeAfter(); - /** What a failed or turned-away input yields. {@code null} unless overridden. */ + /** What a rejected input yields. {@code null} unless overridden. */ @Nullable protected R fallback(T input) { return null; } - /** How many consecutive successes disengage the latch. */ - protected int closeAfter() { - return DEFAULT_CLOSE_AFTER; + /** Reports bad input from {@link #applySafely}: restarts the count, and returns the fallback. */ + @Nullable + protected final R reject(T input) { + remaining = closeAfter(); + return fallback(input); } /** - * Performs the operation, returning {@link #fallback} if it fails with {@code X}. While engaged, - * input that {@link #isKnownToFail} is turned away without performing the operation, so no - * exception is built at all. + * Performs the operation: optimistically while disengaged, cautiously while engaged. An + * optimistic failure engages the latch, and the same input is then retried cautiously, so a + * cautious path that can repair it still gets the chance. */ @Nullable public final R tryApply(T input) { final int remaining = this.remaining; - if (remaining > 0 && isKnownToFail(input)) { - return fallback(input); + if (remaining > 0) { + // counted as clean up front; reject() re-arms the count if it is not + this.remaining = remaining - 1; + return applySafely(input); } try { - final R result = apply(input); - if (remaining > 0) { - this.remaining = remaining - 1; - } - return result; + return apply(input); } catch (RuntimeException e) { if (!failureType.isInstance(e)) { throw e; } this.remaining = closeAfter(); - return fallback(input); + return applySafely(input); } } - /** Returns whether the pre-check is currently consulted. */ + /** Returns whether calls currently take the cautious path. */ public final boolean isEngaged() { return remaining > 0; } diff --git a/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java index 85cea677396..d2f67b4a74e 100644 --- a/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java +++ b/internal-api/src/test/java/datadog/trace/api/GuardedBase64DecodeTest.java @@ -6,10 +6,11 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import datadog.trace.api.Functions.GuardedBase64Decode; -import datadog.trace.util.AdaptiveLatch; import java.util.Base64; +import java.util.Random; import org.junit.jupiter.api.Test; import org.tabletest.junit.TableTest; @@ -30,6 +31,35 @@ private static GuardedBase64Decode engaged() { return decode; } + /** What {@link Base64#getDecoder()} makes of {@code src}: the decoded string, or null. */ + private static String jdk(byte[] src) { + try { + return new String(Base64.getDecoder().decode(src), UTF_8); + } catch (IllegalArgumentException e) { + return null; + } + } + + private static void assertAgreesWithTheJdk(byte[] src) { + String expected; + try { + expected = jdk(src); + } catch (RuntimeException e) { + // the guard relies on the JDK decoder failing only with IllegalArgumentException + throw new AssertionError("JDK decoder threw " + e + " for " + new String(src, UTF_8), e); + } + String actual = GuardedBase64Decode.decodeOrNull(src); + if (expected == null ? actual != null : !expected.equals(actual)) { + fail( + "for \"" + + new String(src, UTF_8) + + "\": JDK " + + (expected == null ? "rejects" : "gives \"" + expected + "\"") + + ", decodeOrNull " + + (actual == null ? "rejects" : "gives \"" + actual + "\"")); + } + } + @Test void decodesValidInput() { assertEquals("x-datadog-trace-id", new GuardedBase64Decode().tryApply(VALID)); @@ -44,38 +74,18 @@ void invalidInputYieldsNullAndEngages() { } @Test - void invalidInputStillYieldsNullWhileEngaged() { - GuardedBase64Decode decode = engaged(); - - assertNull(decode.tryApply(INVALID)); - } - - @Test - void validInputStillDecodesWhileEngaged() { + void whileEngagedItStillDecodesValidInputAndRejectsInvalidInput() { GuardedBase64Decode decode = engaged(); assertEquals("x-datadog-trace-id", decode.tryApply(VALID)); - } - - @TableTest({ - "scenario | input | expected", - "empty | '' | '' ", - "two-char unit | YQ | a ", - "three-char unit | YWI | ab ", - "padded | 'YQ==' | a " - }) - void unpaddedAndEmptyInputIsNotTurnedAwayWhileEngaged(String input, String expected) { - // Base64.getDecoder() accepts input without padding, so the pre-check must not reject it - GuardedBase64Decode decode = engaged(); - - assertEquals(expected, decode.tryApply(bytes(input))); + assertNull(decode.tryApply(INVALID)); } @Test void disengagesAfterEnoughValidInput() { GuardedBase64Decode decode = engaged(); - for (int i = 0; i < AdaptiveLatch.DEFAULT_CLOSE_AFTER; i++) { + for (int i = 0; i < decode.closeAfter(); i++) { decode.tryApply(VALID); } @@ -88,24 +98,79 @@ void otherFailuresPropagate() { } @TableTest({ - "scenario | input | knownToFail", - "empty | '' | false ", - "two-char unit | YQ | false ", - "three-char unit | YWI | false ", - "full unit | YWJj | false ", - "padded | 'YQ==' | false ", - "misplaced padding is lenient | 'Y=Q=' | false ", - "one-char unit | Y | true ", - "one-char final unit | YWJjZ | true ", - "space | 'YW Jj' | true ", - "punctuation | not-valid-base64! | true ", - "url-safe alphabet | 'YQ-_' | true " + "scenario | input | expected", + "empty | '' | '' ", + "two-char unit, unpadded | YQ | a ", + "three-char unit, unpadded | YWI | ab ", + "full unit | YWJj | abc ", + "two-char unit, padded | 'YQ==' | a ", + "three-char unit, padded | 'YWI=' | ab ", + "non-zero trailing bits | 'YR==' | a ", + "one char | Y | ", + "one-char final unit | YWJjZ | ", + "dangling char, padded | 'Y=' | ", + "lone padding | '=' | ", + "padding starts a unit | 'YWJj=' | ", + "two-char unit, one pad | 'YQ=' | ", + "two-char unit, pad, char | 'YQ=Y' | ", + "char after padding | 'YQ==Y' | ", + "unit after padding | 'YQ==YQ==' | ", + "space | 'YW Jj' | ", + "url-safe alphabet | 'YQ-_' | " }) - void preCheckRejectsOnlyWhatTheDecoderWould(String input, boolean knownToFail) { - assertEquals(knownToFail, GuardedBase64Decode.isDefinitelyNotBase64(bytes(input))); - if (knownToFail) { - // the pre-check is only allowed to turn away input the decoder really rejects - assertThrows(IllegalArgumentException.class, () -> Base64.getDecoder().decode(input)); + void decodeOrNullFollowsTheJdkRules(String input, String expected) { + assertEquals(expected, GuardedBase64Decode.decodeOrNull(bytes(input))); + assertAgreesWithTheJdk(bytes(input)); + } + + @Test + void decodeOrNullAgreesWithTheJdkOnEveryShortInputOverAReducedAlphabet() { + // every placement of padding, of alphabet characters with low and high bits set, and of an + // illegal character, up to two full units + byte[] symbols = bytes("AQ/=!"); + for (int length = 0; length <= 8; length++) { + byte[] src = new byte[length]; + int[] digits = new int[length]; + while (true) { + for (int i = 0; i < length; i++) { + src[i] = symbols[digits[i]]; + } + assertAgreesWithTheJdk(src.clone()); + int i = 0; + while (i < length && ++digits[i] == symbols.length) { + digits[i++] = 0; + } + if (i == length) { + break; + } + } + } + } + + @Test + void decodeOrNullAgreesWithTheJdkOnRandomEncodingsAndTheirMutations() { + Random random = new Random(12672); + Base64.Encoder[] encoders = {Base64.getEncoder(), Base64.getEncoder().withoutPadding()}; + byte[] mutations = bytes("=!-_ \nA/"); + for (int n = 0; n < 20_000; n++) { + byte[] data = new byte[random.nextInt(48)]; + random.nextBytes(data); + byte[] encoded = encoders[n & 1].encode(data); + assertAgreesWithTheJdk(encoded); + + if (encoded.length > 0) { + byte[] mutated = encoded.clone(); + mutated[random.nextInt(mutated.length)] = mutations[random.nextInt(mutations.length)]; + assertAgreesWithTheJdk(mutated); + + byte[] truncated = new byte[random.nextInt(encoded.length)]; + System.arraycopy(encoded, 0, truncated, 0, truncated.length); + assertAgreesWithTheJdk(truncated); + } + + byte[] noise = new byte[random.nextInt(16)]; + random.nextBytes(noise); + assertAgreesWithTheJdk(noise); } } } diff --git a/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java index af1070f2134..6d4aa43fb2c 100644 --- a/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java +++ b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java @@ -12,12 +12,13 @@ class AdaptiveLatchTest { /** - * Parses an int. "bad" is known to fail; "sneaky" also fails but passes the pre-check, as input - * the pre-check is too lenient for would. + * Parses an int. The cautious path accepts only plain digits and rejects anything else, so + * "sneaky" input such as an overflowing number passes neither path; "1_000" is repaired by the + * cautious path, which the optimistic path rejects. */ private static final class Parsing extends AdaptiveLatch { - final AtomicInteger applies = new AtomicInteger(); - final AtomicInteger checks = new AtomicInteger(); + final AtomicInteger optimistic = new AtomicInteger(); + final AtomicInteger cautious = new AtomicInteger(); Parsing() { super(NumberFormatException.class); @@ -25,7 +26,7 @@ private static final class Parsing extends AdaptiveLatch 9) { + return reject(input); + } + int value = 0; + for (int i = 0; i < digits.length(); i++) { + char c = digits.charAt(i); + if (c < '0' || c > '9') { + return reject(input); + } + value = value * 10 + (c - '0'); + } + return value; } @Override @@ -45,50 +58,50 @@ protected int closeAfter() { } @Test - void disengagedItAppliesEverythingWithoutPreChecking() { + void disengagedItTakesOnlyTheOptimisticPath() { Parsing latch = new Parsing(); assertEquals(42, latch.tryApply("42")); assertEquals(7, latch.tryApply("7")); - assertEquals(2, latch.applies.get()); - assertEquals(0, latch.checks.get(), "the pre-check is only consulted while engaged"); + assertEquals(2, latch.optimistic.get()); + assertEquals(0, latch.cautious.get()); assertFalse(latch.isEngaged()); } @Test - void aFailureYieldsTheFallbackAndEngages() { + void anOptimisticFailureEngagesAndRetriesTheSameInputCautiously() { Parsing latch = new Parsing(); assertNull(latch.tryApply("bad")); - assertEquals(1, latch.applies.get(), "disengaged, even known-bad input is attempted"); + assertEquals(1, latch.optimistic.get()); + assertEquals(1, latch.cautious.get()); assertTrue(latch.isEngaged()); } @Test - void engagedItTurnsAwayKnownBadInputWithoutApplying() { + void theCautiousRetryCanRepairWhatTheOptimisticPathRejected() { Parsing latch = new Parsing(); - latch.tryApply("bad"); - - assertNull(latch.tryApply("bad")); - assertNull(latch.tryApply("bad")); - assertEquals(1, latch.applies.get()); - assertTrue(latch.isEngaged(), "turned-away input does not count towards disengaging"); + assertEquals(1000, latch.tryApply("1_000")); + assertTrue(latch.isEngaged()); } @Test - void engagedItStillAppliesInputThatPassesThePreCheck() { + void engagedItTakesOnlyTheCautiousPath() { Parsing latch = new Parsing(); latch.tryApply("bad"); assertEquals(42, latch.tryApply("42")); - assertEquals(2, latch.applies.get()); + assertNull(latch.tryApply("bad")); + + assertEquals(1, latch.optimistic.get(), "no optimistic call, and so no throw, while engaged"); + assertEquals(3, latch.cautious.get()); } @Test - void disengagesAfterEnoughConsecutiveSuccesses() { + void disengagesAfterEnoughConsecutiveCleanCalls() { Parsing latch = new Parsing(); latch.tryApply("bad"); @@ -96,23 +109,26 @@ void disengagesAfterEnoughConsecutiveSuccesses() { latch.tryApply("2"); assertTrue(latch.isEngaged()); latch.tryApply("3"); - assertFalse(latch.isEngaged()); + + assertEquals(4, latch.tryApply("4")); + assertEquals(2, latch.optimistic.get(), "disengaged again, so back on the optimistic path"); } @Test - void aFailureThePreCheckMissesReArmsTheLatch() { + void aRejectionWhileEngagedRestartsTheCount() { Parsing latch = new Parsing(); latch.tryApply("bad"); latch.tryApply("1"); latch.tryApply("2"); - // fails, but the pre-check is too lenient to know it - assertNull(latch.tryApply("sneaky")); + assertNull(latch.tryApply("bad")); latch.tryApply("3"); latch.tryApply("4"); - assertTrue(latch.isEngaged(), "the countdown restarted from closeAfter"); + assertTrue(latch.isEngaged(), "the count restarted from closeAfter"); + latch.tryApply("5"); + assertFalse(latch.isEngaged()); } @Test @@ -121,10 +137,11 @@ void otherExceptionsPropagateWithoutEngaging() { assertThrows(IllegalStateException.class, () -> latch.tryApply("boom")); assertFalse(latch.isEngaged()); + assertEquals(0, latch.cautious.get()); } @Test - void fallbackIsUsedForFailedAndTurnedAwayInputButNotARealNull() { + void rejectYieldsTheFallbackAndARealNullCountsAsClean() { AdaptiveLatch latch = new AdaptiveLatch( IllegalArgumentException.class) { @@ -137,8 +154,16 @@ protected String apply(String input) { } @Override - protected boolean isKnownToFail(String input) { - return input.isEmpty(); + protected String applySafely(String input) { + if (input.isEmpty()) { + return reject(input); + } + return "null".equals(input) ? null : input; + } + + @Override + protected int closeAfter() { + return 1; } @Override @@ -147,39 +172,12 @@ protected String fallback(String input) { } }; - // failed + // failed optimistically, then rejected cautiously assertEquals("fallback", latch.tryApply("")); - // turned away while engaged + // rejected while engaged assertEquals("fallback", latch.tryApply("")); - // an operation that succeeds with null is not a failure + // a cautious call that succeeds with null is clean, and with closeAfter 1 disengages assertNull(latch.tryApply("null")); - } - - @Test - void closeAfterDefaultsToTheDefaultConstant() { - AdaptiveLatch latch = - new AdaptiveLatch( - IllegalArgumentException.class) { - @Override - protected String apply(String input) { - if (input.isEmpty()) { - throw new IllegalArgumentException(); - } - return input; - } - - @Override - protected boolean isKnownToFail(String input) { - return input.isEmpty(); - } - }; - latch.tryApply(""); - - for (int i = 1; i < AdaptiveLatch.DEFAULT_CLOSE_AFTER; i++) { - latch.tryApply("ok"); - } - assertTrue(latch.isEngaged()); - latch.tryApply("ok"); assertFalse(latch.isEngaged()); } } From 73424022029dd8a4754d75e5db71040e4102f584 Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 5 Oct 2026 15:33:04 -0400 Subject: [PATCH 11/12] Pass AdaptiveLatch's closeAfter to the constructor It is fixed policy for a call site, like failureType, and is read only on the failure path, so a final field costs nothing over an inlined constant. The constructor rejects a value below 1, which would engage and immediately disengage the latch, and a closeAfter() accessor remains. Co-Authored-By: Claude Opus 5.5 --- .../trace/util/AdaptiveLatchBenchmark.java | 18 +++++----- .../java/datadog/trace/api/Functions.java | 29 +++++++--------- .../datadog/trace/util/AdaptiveLatch.java | 30 ++++++++++++----- .../datadog/trace/util/AdaptiveLatchTest.java | 33 ++++++++++++------- 4 files changed, 62 insertions(+), 48 deletions(-) diff --git a/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java index 0d69d744fa3..66d0c9e4e24 100644 --- a/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java +++ b/internal-api/src/jmh/java/datadog/trace/util/AdaptiveLatchBenchmark.java @@ -96,7 +96,12 @@ public class AdaptiveLatchBenchmark { /** Parses an int; while engaged, a digit scan turns away anything that is not plain digits. */ static class Parse extends AdaptiveLatch { Parse() { - super(NumberFormatException.class); + // the value the recorded results were measured with; not tuned for this operation + this(20); + } + + Parse(int closeAfter) { + super(NumberFormatException.class, closeAfter); } @Override @@ -108,19 +113,12 @@ protected Integer apply(String input) { protected Integer applySafely(String input) { return isAllDigits(input) ? Integer.parseInt(input) : reject(input); } - - /** The value the recorded results were measured with; not tuned for this operation. */ - @Override - protected int closeAfter() { - return 20; - } } /** Never disengages, so that the engaged state holds for a whole run of good input. */ static final class PinnedParse extends Parse { - @Override - protected int closeAfter() { - return Integer.MAX_VALUE; + PinnedParse() { + super(Integer.MAX_VALUE); } } diff --git a/internal-api/src/main/java/datadog/trace/api/Functions.java b/internal-api/src/main/java/datadog/trace/api/Functions.java index fab4a4e86c7..bd9aee0b6f3 100644 --- a/internal-api/src/main/java/datadog/trace/api/Functions.java +++ b/internal-api/src/main/java/datadog/trace/api/Functions.java @@ -200,7 +200,7 @@ public T apply(Object input) { * *

Decodes with {@link Base64#getDecoder()} until it fails, then with {@link * #decodeOrNull(byte[])}, an exception-free decoder that accepts exactly what {@link - * Base64#getDecoder()} accepts, until {@link #closeAfter} consecutive inputs decode again (see + * Base64#getDecoder()} accepts, until {@code CLOSE_AFTER} consecutive inputs decode again (see * {@link AdaptiveLatch}). * *

One instance can be shared across threads: its state is advisory, so a stale read costs one @@ -224,8 +224,18 @@ private static byte[] buildDecodeTable() { return table; } + /** + * The rent-or-buy break-even (see {@link AdaptiveLatch}), from {@code Base64DecodeBenchmark} on + * JDK 17 for header-sized values: a JDK decoder failure costs about 913 ns more than a + * rejection here at stack depth 0 and about 2,468 ns at depth 50, and on valid input this + * decoder costs about 27 ns more than the JDK's, giving 34 to 91. A consumer's stack is deeper + * than a benchmark's. On JDK 8 the two decoders cost about the same on valid input, so the + * value barely matters there. + */ + private static final int CLOSE_AFTER = 64; + public GuardedBase64Decode() { - super(IllegalArgumentException.class); + super(IllegalArgumentException.class, CLOSE_AFTER); } @Override @@ -239,21 +249,6 @@ protected String applySafely(byte[] bytes) { return decoded != null ? decoded : reject(bytes); } - @Override - protected int closeAfter() { - return CLOSE_AFTER; - } - - /** - * The rent-or-buy break-even (see {@link AdaptiveLatch}), from {@code Base64DecodeBenchmark} on - * JDK 17 for header-sized values: a JDK decoder failure costs about 913 ns more than a - * rejection here at stack depth 0 and about 2,468 ns at depth 50, and on valid input this - * decoder costs about 27 ns more than the JDK's, giving 34 to 91. A consumer's stack is deeper - * than a benchmark's. On JDK 8 the two decoders cost about the same on valid input, so the - * value barely matters there. - */ - private static final int CLOSE_AFTER = 64; - /** * Decodes {@code src} as {@link Base64#getDecoder()} would, returning {@code null} where it * would throw. Follows the same rules: padding is optional, but if present must complete the diff --git a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java index 799b2a377fe..7d64b860c55 100644 --- a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java +++ b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java @@ -10,9 +10,9 @@ * ClassLatch}, it never skips the operation: a bad input says nothing about the next one. * *

Disengaged, every call takes the optimistic path. A failure there engages the latch, and from - * then on calls take the cautious path until {@link #closeAfter} consecutive inputs pass it - * cleanly. The cautious path reports bad input by returning {@link #reject}, which restarts that - * count; anything else it returns counts as clean. + * then on calls take the cautious path until {@code closeAfter} consecutive inputs pass it cleanly. + * The cautious path reports bad input by returning {@link #reject}, which restarts that count; + * anything else it returns counts as clean. * *

The cautious path can be built in several ways: * @@ -23,7 +23,7 @@ *

  • a repair that turns common bad input into good input before the optimistic path. * * - *

    Choosing {@link #closeAfter} is a rent-or-buy decision. Staying engaged costs the cautious + *

    Choosing {@code closeAfter} is a rent-or-buy decision. Staying engaged costs the cautious * path's overhead on every good input; disengaging costs one optimistic failure when the next bad * input arrives. Disengaging once the accumulated overhead would match one failure, that is after * about {@code failureCost / cautiousOverhead} good inputs, is never worse than twice the best @@ -44,12 +44,22 @@ */ public abstract class AdaptiveLatch { private final Class failureType; + private final int closeAfter; /** 0 when disengaged; otherwise the clean calls left before disengaging. */ private int remaining; - protected AdaptiveLatch(Class failureType) { + /** + * @param failureType the failure of {@link #apply} that engages the latch + * @param closeAfter how many consecutive clean calls disengage it, at least 1; see the class + * comment for how to choose it + */ + protected AdaptiveLatch(Class failureType, int closeAfter) { + if (closeAfter < 1) { + throw new IllegalArgumentException("closeAfter must be at least 1: " + closeAfter); + } this.failureType = failureType; + this.closeAfter = closeAfter; } /** The optimistic path: fastest on good input, but may throw {@code X} for bad input. */ @@ -63,8 +73,10 @@ protected AdaptiveLatch(Class failureType) { @Nullable protected abstract R applySafely(T input); - /** How many consecutive clean calls disengage the latch; see the class comment. */ - protected abstract int closeAfter(); + /** How many consecutive clean calls disengage the latch. */ + public final int closeAfter() { + return closeAfter; + } /** What a rejected input yields. {@code null} unless overridden. */ @Nullable @@ -75,7 +87,7 @@ protected R fallback(T input) { /** Reports bad input from {@link #applySafely}: restarts the count, and returns the fallback. */ @Nullable protected final R reject(T input) { - remaining = closeAfter(); + remaining = closeAfter; return fallback(input); } @@ -98,7 +110,7 @@ public final R tryApply(T input) { if (!failureType.isInstance(e)) { throw e; } - this.remaining = closeAfter(); + this.remaining = closeAfter; return applySafely(input); } } diff --git a/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java index 6d4aa43fb2c..cf8422325e0 100644 --- a/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java +++ b/internal-api/src/test/java/datadog/trace/util/AdaptiveLatchTest.java @@ -21,7 +21,7 @@ private static final class Parsing extends AdaptiveLatch latch = new AdaptiveLatch( - IllegalArgumentException.class) { + IllegalArgumentException.class, 1) { @Override protected String apply(String input) { if (input.isEmpty()) { @@ -161,11 +156,6 @@ protected String applySafely(String input) { return "null".equals(input) ? null : input; } - @Override - protected int closeAfter() { - return 1; - } - @Override protected String fallback(String input) { return "fallback"; @@ -180,4 +170,23 @@ protected String fallback(String input) { assertNull(latch.tryApply("null")); assertFalse(latch.isEngaged()); } + + @Test + void closeAfterMustBeAtLeastOne() { + assertThrows( + IllegalArgumentException.class, + () -> + new AdaptiveLatch( + IllegalArgumentException.class, 0) { + @Override + protected String apply(String input) { + return input; + } + + @Override + protected String applySafely(String input) { + return input; + } + }); + } } From 3c7c25fc04417c77418119d20ee9ab713189051a Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Mon, 5 Oct 2026 15:46:38 -0400 Subject: [PATCH 12/12] Mark AdaptiveLatch as @ThreadSafe Its state is a plain counter by design: a stale or lost update costs one more cautious call or one more optimistic failure, never a wrong result. SpotBugs reports nothing for it, so no suppression is needed. Co-Authored-By: Claude Opus 5.5 --- .../src/main/java/datadog/trace/util/AdaptiveLatch.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java index 7d64b860c55..96e5408d957 100644 --- a/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java +++ b/internal-api/src/main/java/datadog/trace/util/AdaptiveLatch.java @@ -1,6 +1,7 @@ package datadog.trace.util; import javax.annotation.Nullable; +import javax.annotation.concurrent.ThreadSafe; /** * A self-resetting switch between two ways of performing an operation whose failure depends on the @@ -42,6 +43,7 @@ * @param the type of the result * @param the failure that engages the latch; anything else propagates unchanged */ +@ThreadSafe public abstract class AdaptiveLatch { private final Class failureType; private final int closeAfter;