(skywalking-banyandb) 02/03: perf(tracesampler): reject non-matching tag entries before the string compare

[ha...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

hanahmily pushed a commit to branch perf/sampler-scalar-reads
in repository https://gitbox.apache.org/repos/asf/skywalking-banyandb.git

commit 90e9310b161cd9eabd9acc15bc3760679cbf6a63
Author: Hongtao Gao <ha...@gmail.com>
AuthorDate: Mon Aug 10 05:33:29 2026 +0000

    perf(tracesampler): reject non-matching tag entries before the string compare
    
    With the scalar-column copy gone, matchEntries became the largest cost in the
    realistic scenario at 41.2% cumulative. Line-level profiling put 250ms of its
    540ms in a single line: strings.HasPrefix.
    
    It is called once per (rule x entry) — 48 times per trace for the sw scenario's
    2 rules over 24 entries — and the overwhelmingly common outcome is rejection,
    because most entries belong to some other tag. Check length and first byte
    inline before letting HasPrefix reach its memequal call, and hoist the
    prefix-empty test out of the loop: a rule naming the array column itself
    matches the whole entry, so it needs neither prefix stripping nor a bare-key
    case, and now takes a separate branch.
    
    The bare-key test stays AHEAD of the new guard, which is load-bearing rather
    than stylistic: prefix is TagKey+"=", so a bare key is exactly one byte too
    short and the length check would skip it. Moving the guard first fails the
    pre-existing TestDecide_ExistsMatchesBareKey; breaking the array-column branch
    fails two more. Both new branches are covered by tests that already existed.
    
    Measured A/B/A against the previous commit, 5 counts per arm:
    
      Decide/sw/tagRulesOnly           24.67 -> 20.32 us  -17.6%
      Decide/zipkin/tagRulesOnly       24.57 -> 20.19 us  -17.8%
      Decide/sw/scenario               37.75 -> 27.70 us  -26.6%
      Decide/zipkin/scenario           71.08 -> 62.26 us  -12.4%
      Decide_TagMatchPosition/sw/31    71.89 -> 65.17 us   -9.4%
      Decide_TagMatchPosition/sw/16    66.98 -> 63.61 us   -5.0%
      Decide_TagMatchPosition/sw/0     61.18 -> 61.88 us   +1.1%
    
    The last row is a real regression, not noise: when the very first entry matches,
    the guard is pure overhead. It is the deliberate trade — the win grows with how
    far the matching entry sits from the front, and a rule that matches the first
    entry of every trace is not the case worth tuning for.
---
 CHANGES.md                                         |  1 +
 .../skywalking/internal/tracesampler/sampler.go    | 28 ++++++++++++++++------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/CHANGES.md b/CHANGES.md
index fb338eebc..d3a781da1 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -173,6 +173,7 @@ Release Notes.
 - Bump Go and ui/mcp dependencies to clear Dependabot advisories: `golang.org/x/net` v0.52.0→v0.56.0 (CVE-2026-25680), `opencontainers/runc` v1.3.3→v1.3.6 (CVE-2026-41579). Refresh mcp/ui lockfiles and license attribution.
 - Defer the sw-trace-sampler tag-array decode to first use: 60% per-trace cost reduction on the realistic-mix path (`duration+err+tag` 2,710 → 1,015 ns/trace, 2,380 → 664 allocs/op on the SkyWalking schema).
 - Alias decoded entry strings via `pkg/convert.BytesToString`: 408 → 1 alloc/op on SkyWalking tag paths and 1,536 → 1 on Zipkin tag paths (the realistic 3-row × 8-entry trace); 24-40% ns/trace reduction on tag-using paths. Also fixes a pre-existing nil-deref in `keepTrace` when the array column is absent.
+- Reject non-matching tag entries in the sampler's rule matcher before the string comparison. Most entries belong to some other tag, so the common outcome is rejection; a length and first-byte check now precedes `strings.HasPrefix`, and rules naming the array column itself take a branch with no prefix handling at all. Tag-rule decisions drop a further 18% and full-scenario decisions 12-27%.
 - Read the trace sampler's scalar tag columns without materializing the SDK's decoded value. `sdk.Value` is 112 bytes and `TagColumn.At` returns it by value, so reading the duration, start-time and error columns per row cost more in `runtime.duffcopy` than in decoding. The sampler now reads those 8-byte cells directly, gated narrowly on the column's exact type so a malformed cell in a wrong-typed column still fails open with the same error. Duration-envelope decisions drop 80% (`duration [...]
 - Decode escape-free trace tag rows without copying them. The sw/zipkin trace sampler copied every tag row before decoding, because the var-array decode rewrites its input in place and the engine hands the same batch to every link of a sampler chain. That copy is only needed for a row that actually contains an escape, so rows without one now decode straight from the block with the entries aliasing it — safe because `assembleTraceBlock` already deep-copies each value into block-owned stor [...]
 - Precompute each tag rule's `"tagKey="` entry prefix at admission instead of rebuilding it on every `matchEntries` call (once per rule per trace).
diff --git a/plugins/skywalking/internal/tracesampler/sampler.go b/plugins/skywalking/internal/tracesampler/sampler.go
index 8026d5a66..a0829091a 100644
--- a/plugins/skywalking/internal/tracesampler/sampler.go
+++ b/plugins/skywalking/internal/tracesampler/sampler.go
@@ -1160,18 +1160,32 @@ func (s *Sampler) hasErrorColumn(b *sdk.TraceBlock) (bool, error) {
 // two cannot drift apart.
 func matchEntries(entries []string, r *rule) bool {
 	prefix := r.prefix
+	if prefix == "" {
+		// A rule naming the array column itself matches the whole entry, so there
+		// is no prefix to strip and no bare-key case distinct from it.
+		for _, entry := range entries {
+			if r.Exists || matchValue(r, entry) {
+				return true
+			}
+		}
+		return false
+	}
+	// Most entries belong to some other tag, so the common outcome is rejection.
+	// Reject on length and first byte before HasPrefix's memequal call. The bare-key
+	// test must stay ahead of that guard: prefix is TagKey+"=", so a bare key is one
+	// byte too short and the length check would skip it.
+	first, plen := prefix[0], len(prefix)
 	for _, entry := range entries {
 		if r.Exists && entry == r.TagKey {
 			return true
 		}
-		candidate := entry
-		if prefix != "" {
-			if !strings.HasPrefix(entry, prefix) {
-				continue
-			}
-			candidate = entry[len(prefix):]
+		if len(entry) < plen || entry[0] != first {
+			continue
+		}
+		if !strings.HasPrefix(entry, prefix) {
+			continue
 		}
-		if r.Exists || matchValue(r, candidate) {
+		if r.Exists || matchValue(r, entry[plen:]) {
 			return true
 		}
 	}