From a7c0467e011e13432571e6cee1340739b183449d Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Mon, 30 Jan 2023 09:46:23 -0800 Subject: [PATCH 1/3] apiextensions: Benchmark escaping in SchemaHas SchemaHas needs to escape the apiextensions.JSONSchemaProps pointer since it's calling an arbitrary predicate function that can keep a copy of the pointer (even though it shouldn't, but the compiler can't know that). The leak is resulting in a fair amount of memory allocation when installing many CRDs in a row (about 3% of the memory allocated happens in this method). --- .../validation/validation_test.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index 853110c2d6d..687d238b455 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -8624,6 +8624,30 @@ func TestSchemaHasDefaults(t *testing.T) { } } +func BenchmarkSchemaHas(b *testing.B) { + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + if err := apiextensions.AddToScheme(scheme); err != nil { + b.Fatal(err) + } + fuzzerFuncs := fuzzer.MergeFuzzerFuncs(apiextensionsfuzzer.Funcs) + seed := int64(5577006791947779410) + f := fuzzer.FuzzerFor(fuzzerFuncs, rand.NewSource(seed), codecs) + // fuzz internal types + schema := &apiextensions.JSONSchemaProps{} + f.NilChance(0).NumElements(10, 10).MaxDepth(10).Fuzz(schema) + + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if SchemaHas(schema, func(_ *apiextensions.JSONSchemaProps) bool { + return false + }) { + b.Errorf("Function returned true") + } + } +} + var example = apiextensions.JSON(`"This is an example"`) var validValidationSchema = &apiextensions.JSONSchemaProps{ From 7dea3409d4944b0d61108f9bd1b553d8cfed7e9c Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Mon, 30 Jan 2023 10:02:57 -0800 Subject: [PATCH 2/3] apiextensions: Pool schemas in SchemaHas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using a `sync.Pool` to re-use the buffers that have to escape in the predicate significantly reduces the number of allocations needed to run the SchemaHas method, as shown in the following example: ``` > benchstat old.bench new.bench name old time/op new time/op delta SchemaHas-8 11.0ms ± 0% 2.1ms ± 1% -80.60% (p=0.008 n=5+5) name old alloc/op new alloc/op delta SchemaHas-8 37.5MB ± 0% 0.0MB ± 0% -100.00% (p=0.008 n=5+5) name old allocs/op new allocs/op delta SchemaHas-8 73.1k ± 0% 0.0k -100.00% (p=0.008 n=5+5) ``` --- .../apiextensions/validation/validation.go | 41 +++++++++++++------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index be4252c14c4..d93cc932490 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -23,6 +23,7 @@ import ( "reflect" "regexp" "strings" + "sync" "unicode" "unicode/utf8" @@ -1364,6 +1365,22 @@ func HasSchemaWith(spec *apiextensions.CustomResourceDefinitionSpec, pred func(s return false } +var schemaPool = sync.Pool{ + New: func() any { + return new(apiextensions.JSONSchemaProps) + }, +} + +func schemaHasRecurse(s *apiextensions.JSONSchemaProps, pred func(s *apiextensions.JSONSchemaProps) bool) bool { + if s == nil { + return false + } + schema := schemaPool.Get().(*apiextensions.JSONSchemaProps) + defer schemaPool.Put(schema) + *schema = *s + return SchemaHas(schema, pred) +} + func SchemaHas(s *apiextensions.JSONSchemaProps, pred func(s *apiextensions.JSONSchemaProps) bool) bool { if s == nil { return false @@ -1374,60 +1391,60 @@ func SchemaHas(s *apiextensions.JSONSchemaProps, pred func(s *apiextensions.JSON } if s.Items != nil { - if s.Items != nil && SchemaHas(s.Items.Schema, pred) { + if s.Items != nil && schemaHasRecurse(s.Items.Schema, pred) { return true } for i := range s.Items.JSONSchemas { - if SchemaHas(&s.Items.JSONSchemas[i], pred) { + if schemaHasRecurse(&s.Items.JSONSchemas[i], pred) { return true } } } for i := range s.AllOf { - if SchemaHas(&s.AllOf[i], pred) { + if schemaHasRecurse(&s.AllOf[i], pred) { return true } } for i := range s.AnyOf { - if SchemaHas(&s.AnyOf[i], pred) { + if schemaHasRecurse(&s.AnyOf[i], pred) { return true } } for i := range s.OneOf { - if SchemaHas(&s.OneOf[i], pred) { + if schemaHasRecurse(&s.OneOf[i], pred) { return true } } - if SchemaHas(s.Not, pred) { + if schemaHasRecurse(s.Not, pred) { return true } for _, s := range s.Properties { - if SchemaHas(&s, pred) { + if schemaHasRecurse(&s, pred) { return true } } if s.AdditionalProperties != nil { - if SchemaHas(s.AdditionalProperties.Schema, pred) { + if schemaHasRecurse(s.AdditionalProperties.Schema, pred) { return true } } for _, s := range s.PatternProperties { - if SchemaHas(&s, pred) { + if schemaHasRecurse(&s, pred) { return true } } if s.AdditionalItems != nil { - if SchemaHas(s.AdditionalItems.Schema, pred) { + if schemaHasRecurse(s.AdditionalItems.Schema, pred) { return true } } for _, s := range s.Definitions { - if SchemaHas(&s, pred) { + if schemaHasRecurse(&s, pred) { return true } } for _, d := range s.Dependencies { - if SchemaHas(d.Schema, pred) { + if schemaHasRecurse(d.Schema, pred) { return true } } From 06aff8a546a5673ff44bdcf845d88ca94419a478 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Mon, 30 Jan 2023 11:08:16 -0800 Subject: [PATCH 3/3] apiextensions: SchemaHas predicate must be read-only/non-captured --- .../pkg/apis/apiextensions/validation/validation.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index d93cc932490..ab69389e675 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -1381,6 +1381,11 @@ func schemaHasRecurse(s *apiextensions.JSONSchemaProps, pred func(s *apiextensio return SchemaHas(schema, pred) } +// SchemaHas recursively traverses the Schema and calls the `pred` +// predicate to see if the schema contains specific values. +// +// The predicate MUST NOT keep a copy of the json schema NOR modify the +// schema. func SchemaHas(s *apiextensions.JSONSchemaProps, pred func(s *apiextensions.JSONSchemaProps) bool) bool { if s == nil { return false