From 523284ffd7da4a4d7f7cd72046565f900ee1fb3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Sun, 25 Jan 2026 18:52:48 +0000 Subject: [PATCH] internal/core/adt: avoid allocations in matchPattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feature.ToValue was called eagerly for every string label before pattern matching, allocating a *String even though the fast path only needs the string content. This change retrieves the string directly via ctx.IndexToString for fast-path cases, only allocating in the slow path. This also fixes a bug in jsonschema where error patterns were not handled correctly. The old code checked `!k.IsAnyOf(pattern.Kind())` before the switch statement, but *Bottom has Kind() == BottomKind (0), so this check always failed for error patterns, returning false before the *Bottom case could be reached. The new code handles *Bottom in the switch before any Kind filtering occurs. The bug fix cannot be easily separated from the optimization because both changes affect how pattern matching flows through the code, and attempts to isolate the fix while keeping the old allocation behavior were unsuccessful. │ old │ new │ │ B/op │ B/op vs base │ VetInventory 4.592Gi ± ∞ ¹ 4.565Gi ± ∞ ¹ -0.58% (p=1.000 n=1) │ old │ new │ │ allocs/op │ allocs/op vs base │ VetInventory 47.89M ± ∞ ¹ 47.29M ± ∞ ¹ -1.26% (p=1.000 n=1) Signed-off-by: Daniel Martí Change-Id: I2c815823ff2412e32f1fbc967c847bed3101291b Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1229642 TryBot-Result: CUEcueckoo Reviewed-by: Matthew Sackman Unity-Result: CUE porcuepine --- encoding/jsonschema/external_teststats.txt | 4 +- .../tests/draft2019-09/propertyNames.json | 1 - .../tests/draft2020-12/propertyNames.json | 1 - .../external/tests/draft6/propertyNames.json | 1 - .../external/tests/draft7/propertyNames.json | 1 - internal/core/adt/constraints.go | 42 +++++-------------- internal/core/adt/constraints_test.go | 8 +--- internal/core/adt/export_test.go | 4 +- 8 files changed, 17 insertions(+), 45 deletions(-) diff --git a/encoding/jsonschema/external_teststats.txt b/encoding/jsonschema/external_teststats.txt index d19bd710d..40e8d5814 100644 --- a/encoding/jsonschema/external_teststats.txt +++ b/encoding/jsonschema/external_teststats.txt @@ -4,8 +4,8 @@ Core tests: v3: schema extract (pass / total): 1072 / 1363 = 78.7% - tests (pass / total): 3913 / 4803 = 81.5% - tests on extracted schemas (pass / total): 3913 / 4041 = 96.8% + tests (pass / total): 3917 / 4803 = 81.6% + tests on extracted schemas (pass / total): 3917 / 4041 = 96.9% v3-roundtrip: schema extract (pass / total): 240 / 1363 = 17.6% diff --git a/encoding/jsonschema/testdata/external/tests/draft2019-09/propertyNames.json b/encoding/jsonschema/testdata/external/tests/draft2019-09/propertyNames.json index c7914100c..69cb6b51c 100644 --- a/encoding/jsonschema/testdata/external/tests/draft2019-09/propertyNames.json +++ b/encoding/jsonschema/testdata/external/tests/draft2019-09/propertyNames.json @@ -171,7 +171,6 @@ }, "valid": false, "skip": { - "v3": "unexpected success", "v3-roundtrip": "could not extract schema" } }, diff --git a/encoding/jsonschema/testdata/external/tests/draft2020-12/propertyNames.json b/encoding/jsonschema/testdata/external/tests/draft2020-12/propertyNames.json index aeaee88bc..05d976b4f 100644 --- a/encoding/jsonschema/testdata/external/tests/draft2020-12/propertyNames.json +++ b/encoding/jsonschema/testdata/external/tests/draft2020-12/propertyNames.json @@ -114,7 +114,6 @@ }, "valid": false, "skip": { - "v3": "unexpected success", "v3-roundtrip": "unexpected success" } }, diff --git a/encoding/jsonschema/testdata/external/tests/draft6/propertyNames.json b/encoding/jsonschema/testdata/external/tests/draft6/propertyNames.json index 02b5f92dc..cecd536aa 100644 --- a/encoding/jsonschema/testdata/external/tests/draft6/propertyNames.json +++ b/encoding/jsonschema/testdata/external/tests/draft6/propertyNames.json @@ -167,7 +167,6 @@ }, "valid": false, "skip": { - "v3": "unexpected success", "v3-roundtrip": "could not extract schema" } }, diff --git a/encoding/jsonschema/testdata/external/tests/draft7/propertyNames.json b/encoding/jsonschema/testdata/external/tests/draft7/propertyNames.json index 02b5f92dc..cecd536aa 100644 --- a/encoding/jsonschema/testdata/external/tests/draft7/propertyNames.json +++ b/encoding/jsonschema/testdata/external/tests/draft7/propertyNames.json @@ -167,7 +167,6 @@ }, "valid": false, "skip": { - "v3": "unexpected success", "v3-roundtrip": "could not extract schema" } }, diff --git a/internal/core/adt/constraints.go b/internal/core/adt/constraints.go index 9d68d880a..a9de171d1 100644 --- a/internal/core/adt/constraints.go +++ b/internal/core/adt/constraints.go @@ -127,35 +127,18 @@ func matchPattern(ctx *OpContext, pattern Value, f Feature) bool { return false } - // TODO(perf): this assumes that comparing an int64 against apd.Decimal - // is faster than converting this to a Num and using that for comparison. - // This may very well not be the case. But it definitely will be if we - // special-case integers that can fit in an int64 (or int32 if we want to - // avoid many bound checks), which we probably should. Especially when we - // allow list constraints, like [<10]: T. - var label Value - if f.IsString() && int64(f.Index()) != MaxIndex { - label = f.ToValue(ctx) - } - - return matchPatternValue(ctx, pattern, f, label) + return matchPatternValue(ctx, pattern, f) } -// matchPatternValue matches a concrete value against f. label must be the -// CUE value that is obtained from converting f. +// matchPatternValue matches a concrete value against f. // // This is an optimization an intended to be faster than regular CUE evaluation // for the majority of cases where pattern constraints are used. -func matchPatternValue(ctx *OpContext, pattern Value, f Feature, label Value) (result bool) { +func matchPatternValue(ctx *OpContext, pattern Value, f Feature) (result bool) { if v, ok := pattern.(*Vertex); ok { v.unify(ctx, Flags{condition: scalarKnown, mode: finalize, checkTypos: false}) } pattern = Unwrap(pattern) - label = Unwrap(label) - - if pattern == label { - return true - } k := IntKind if f.IsString() { @@ -193,10 +176,10 @@ func matchPatternValue(ctx *OpContext, pattern Value, f Feature, label Value) (r case *BoundValue: switch x.Kind() { case StringKind: - if label == nil { + if !f.IsString() || int64(f.Index()) == MaxIndex { return false } - str := label.(*String).Str + str := ctx.IndexToString(f.safeIndex()) return x.validateStr(ctx, str) case NumberKind: @@ -212,15 +195,15 @@ func matchPatternValue(ctx *OpContext, pattern Value, f Feature, label Value) (r return err == nil && xi == yi case *String: - if label == nil { + if !f.IsString() || int64(f.Index()) == MaxIndex { return false } - y, ok := label.(*String) - return ok && x.Str == y.Str + str := ctx.IndexToString(f.safeIndex()) + return x.Str == str case *Conjunction: for _, a := range x.Values { - if !matchPatternValue(ctx, a, f, label) { + if !matchPatternValue(ctx, a, f) { return false } } @@ -228,7 +211,7 @@ func matchPatternValue(ctx *OpContext, pattern Value, f Feature, label Value) (r case *Disjunction: for _, a := range x.Values { - if matchPatternValue(ctx, a, f, label) { + if matchPatternValue(ctx, a, f) { return true } } @@ -242,10 +225,7 @@ func matchPatternValue(ctx *OpContext, pattern Value, f Feature, label Value) (r // slow track. One way to signal this would be to have a "value thunk" at // the root that causes the fast track to be bypassed altogether. - if label == nil { - label = f.ToValue(ctx) - } - + label := f.ToValue(ctx) n := ctx.newInlineVertex(nil, nil, MakeConjunct(ctx.e, pattern, ctx.ci), MakeConjunct(ctx.e, label, ctx.ci)) diff --git a/internal/core/adt/constraints_test.go b/internal/core/adt/constraints_test.go index 912c1d266..b3ff248fc 100644 --- a/internal/core/adt/constraints_test.go +++ b/internal/core/adt/constraints_test.go @@ -135,16 +135,12 @@ func TestMatchPatternValue(t *testing.T) { expr := pv(t.T, tc.expr) var f adt.Feature - var label adt.Value if tc.label != "" { - f, label = str(tc.label) + f, _ = str(tc.label) } else { f = idx(tc.index) } - if tc.value != "" { - label = pv(t.T, tc.value) - } - t.Equal(adt.MatchPatternValue(ctx, expr, f, label), tc.result) + t.Equal(adt.MatchPatternValue(ctx, expr, f), tc.result) }) } diff --git a/internal/core/adt/export_test.go b/internal/core/adt/export_test.go index a1c9f790f..e5873f52f 100644 --- a/internal/core/adt/export_test.go +++ b/internal/core/adt/export_test.go @@ -18,6 +18,6 @@ package adt // fields_test.go and constraints_test. // MatchPatternValue exports matchPatternValue for testing. -func MatchPatternValue(ctx *OpContext, p Value, f Feature, label Value) bool { - return matchPatternValue(ctx, p, f, label) +func MatchPatternValue(ctx *OpContext, p Value, f Feature) bool { + return matchPatternValue(ctx, p, f) } -- 2.51.2