From 87b67a49a43e3ed6e00f50135b075166b24d5633 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 28 Oct 2026 11:44:31 +0100 Subject: [PATCH] structdiff: compare scalars and string maps without reflect allocations Leaves of basic kinds are compared directly instead of boxing both sides for reflect.DeepEqual, and map[string]string is compared natively; reflect copied every key and value to the heap. IsEqual on equal JobSettings goes from 198 allocations to 0, GetStructDiff from 560 to 356. Co-authored-by: Isaac --- libs/structs/structdiff/bench_test.go | 12 ++++++ libs/structs/structdiff/diff.go | 57 +++++++++++++++++++++++++- libs/structs/structdiff/diff_test.go | 58 +++++++++++++++++++++++++++ libs/structs/structdiff/equal.go | 9 ++++- 4 files changed, 134 insertions(+), 2 deletions(-) diff --git a/libs/structs/structdiff/bench_test.go b/libs/structs/structdiff/bench_test.go index 08b386416ef..875c5a6b180 100644 --- a/libs/structs/structdiff/bench_test.go +++ b/libs/structs/structdiff/bench_test.go @@ -44,3 +44,15 @@ func BenchmarkZero(b *testing.B) { func BenchmarkNils(b *testing.B) { bench(b, jobExampleResponse, jobExampleResponseNils) } + +func BenchmarkIsEqual(b *testing.B) { + var x, y jobs.JobSettings + require.NoError(b, json.Unmarshal([]byte(jobExampleResponse), &x)) + require.NoError(b, json.Unmarshal([]byte(jobExampleResponse), &y)) + + for b.Loop() { + if !IsEqual(&x, &y) { + b.Fatal("expected equal") + } + } +} diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 00f1bdcaa34..cd37fff886b 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -193,11 +193,30 @@ func diffValues(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Valu } func deepEqualValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Change) { - if !reflect.DeepEqual(v1.Interface(), v2.Interface()) { + if !equalLeaf(v1, v2) { *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: v2.Interface()}) } } +// equalLeaf compares values that are not walked into. Basic kinds are compared +// directly: reflect.DeepEqual needs Interface(), which allocates for each of them. +func equalLeaf(v1, v2 reflect.Value) bool { + switch v1.Kind() { + case reflect.String: + return v1.String() == v2.String() + case reflect.Bool: + return v1.Bool() == v2.Bool() + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return v1.Int() == v2.Int() + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr: + return v1.Uint() == v2.Uint() + case reflect.Float32, reflect.Float64: + return v1.Float() == v2.Float() + default: + return reflect.DeepEqual(v1.Interface(), v2.Interface()) + } +} + func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Value, changes *[]Change) error { t := s1.Type() forced1 := getForceSendFields(s1) @@ -272,6 +291,12 @@ func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Valu } func diffMapStringKey(ctx *diffContext, path *structpath.PathNode, m1, m2 reflect.Value, changes *[]Change) error { + if s1, ok := reflect.TypeAssert[map[string]string](m1); ok { + s2, _ := reflect.TypeAssert[map[string]string](m2) + diffStringMaps(path, s1, s2, changes) + return nil + } + keySet := map[string]reflect.Value{} for _, k := range m1.MapKeys() { // Caller guarantees the key kind is String; use Value.String() rather @@ -297,6 +322,36 @@ func diffMapStringKey(ctx *diffContext, path *structpath.PathNode, m1, m2 reflec return nil } +// diffStringMaps is diffMapStringKey for map[string]string, the most common map +// type in configs (tags, spark_conf, ...). Going through reflect would copy every +// key and value to the heap. +func diffStringMaps(path *structpath.PathNode, m1, m2 map[string]string, changes *[]Change) { + keys := slices.Sorted(maps.Keys(m1)) + for k := range m2 { + if _, ok := m1[k]; !ok { + keys = append(keys, k) + } + } + slices.Sort(keys) + + for _, k := range keys { + s1, ok1 := m1[k] + s2, ok2 := m2[k] + if ok1 && ok2 && s1 == s2 { + continue + } + node := structpath.NewBracketString(path, k) + switch { + case !ok1: + *changes = append(*changes, Change{Path: node, Old: nil, New: s2}) + case !ok2: + *changes = append(*changes, Change{Path: node, Old: s1, New: nil}) + default: + *changes = append(*changes, Change{Path: node, Old: s1, New: s2}) + } + } +} + func getForceSendFields(v reflect.Value) []string { if !v.IsValid() || v.Kind() != reflect.Struct { return nil diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index 9bd9bb34a81..fa875bccbbb 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -1067,3 +1067,61 @@ func TestGetStructDiffDashNamedField(t *testing.T) { assert.Equal(t, len(changes) == 0, IsEqual(before, after)) } + +type pathsInner struct { + X string `json:"x"` + Y string `json:"y"` + M map[string]string `json:"m"` +} + +type pathsOuter struct { + A pathsInner `json:"a"` + B pathsInner `json:"b"` + L []pathsInner `json:"l"` + Tasks []jobs.Task `json:"tasks"` +} + +// TestGetStructDiffPathsSharingPrefix covers paths of changes in sibling subtrees, +// including map[string]string keys present on only one side. +func TestGetStructDiffPathsSharingPrefix(t *testing.T) { + before := pathsOuter{ + A: pathsInner{X: "1", Y: "1", M: map[string]string{"k1": "1", "k-2": "1", "gone": "1", "same": "1"}}, + B: pathsInner{X: "1"}, + L: []pathsInner{{X: "1"}, {Y: "1"}}, + Tasks: []jobs.Task{{TaskKey: "t1", Description: "1"}, {TaskKey: "t2", Description: "1"}}, + } + after := pathsOuter{ + A: pathsInner{X: "2", Y: "2", M: map[string]string{"k1": "2", "k-2": "2", "added": "2", "same": "1"}}, + B: pathsInner{Y: "2"}, + L: []pathsInner{{X: "2"}, {Y: "2"}}, + Tasks: []jobs.Task{{TaskKey: "t2", Description: "2"}, {TaskKey: "t3"}}, + } + + changes, err := GetStructDiff(before, after, map[string]KeyFunc{ + "tasks": func(task jobs.Task) (string, string) { return "task_key", task.TaskKey }, + }) + require.NoError(t, err) + + var paths []string + for _, ch := range changes { + paths = append(paths, ch.Path.String()) + } + assert.Equal(t, []string{ + "a.x", + "a.y", + "a.m['added']", + "a.m['gone']", + "a.m['k-2']", + "a.m['k1']", + "b.x", + "b.y", + "l[0].x", + "l[1].y", + "tasks[task_key='t1']", + "tasks[task_key='t2'].description", + "tasks[task_key='t3']", + }, paths) + + assert.Equal(t, Change{Path: changes[2].Path, Old: nil, New: "2"}, changes[2]) + assert.Equal(t, Change{Path: changes[3].Path, Old: "1", New: nil}, changes[3]) +} diff --git a/libs/structs/structdiff/equal.go b/libs/structs/structdiff/equal.go index 6ed8243d40b..dd0589d7a7e 100644 --- a/libs/structs/structdiff/equal.go +++ b/libs/structs/structdiff/equal.go @@ -1,6 +1,7 @@ package structdiff import ( + "maps" "reflect" "slices" @@ -92,7 +93,7 @@ func equalValues(v1, v2 reflect.Value) bool { } return reflect.DeepEqual(v1.Interface(), v2.Interface()) default: - return reflect.DeepEqual(v1.Interface(), v2.Interface()) + return equalLeaf(v1, v2) } return true } @@ -153,6 +154,12 @@ func equalStruct(s1, s2 reflect.Value) bool { } func equalMapStringKey(m1, m2 reflect.Value) bool { + // map[string]string is the common case; reflect would copy every key and value to the heap. + if s1, ok := reflect.TypeAssert[map[string]string](m1); ok { + s2, _ := reflect.TypeAssert[map[string]string](m2) + return maps.Equal(s1, s2) + } + keySet := map[string]reflect.Value{} for _, k := range m1.MapKeys() { // Key kind is always String at this point; Value.String() also handles a