From 668c02bcf9a383b837c77749984594ac472f5fd5 Mon Sep 17 00:00:00 2001 From: George Pollard Date: Wed, 14 Jul 2021 12:14:54 +1200 Subject: [PATCH] Short-circuit "element" updates in one place (#1635) --- hack/generator/pkg/astmodel/array_type.go | 13 +++++++ hack/generator/pkg/astmodel/flagged_type.go | 4 ++ hack/generator/pkg/astmodel/map_type.go | 20 ++++++++++ hack/generator/pkg/astmodel/optional_type.go | 38 ++++++++++++++++++- hack/generator/pkg/astmodel/resource_type.go | 8 ++++ hack/generator/pkg/astmodel/type_visitor.go | 20 ++-------- hack/generator/pkg/astmodel/validated_type.go | 10 +++++ .../convert_allof_and_oneof_to_objects.go | 2 +- .../pkg/codegen/pipeline/status_augment.go | 34 ++++------------- 9 files changed, 102 insertions(+), 47 deletions(-) diff --git a/hack/generator/pkg/astmodel/array_type.go b/hack/generator/pkg/astmodel/array_type.go index b57be2e6585..2c917d7e769 100644 --- a/hack/generator/pkg/astmodel/array_type.go +++ b/hack/generator/pkg/astmodel/array_type.go @@ -29,6 +29,19 @@ func (array *ArrayType) Element() Type { return array.element } +// WithElement returns an ArrayType with the specified element. +// the benefit of this is it allows reusing the same value if the +// element type is the same +func (array *ArrayType) WithElement(t Type) *ArrayType { + if array.element.Equals(t) { + return array + } + + result := *array + result.element = t + return &result +} + // assert we implemented Type correctly var _ Type = (*ArrayType)(nil) diff --git a/hack/generator/pkg/astmodel/flagged_type.go b/hack/generator/pkg/astmodel/flagged_type.go index 96bd368c7f0..f4e65efadfb 100644 --- a/hack/generator/pkg/astmodel/flagged_type.go +++ b/hack/generator/pkg/astmodel/flagged_type.go @@ -86,6 +86,10 @@ func (ft *FlaggedType) WithoutFlag(flag TypeFlag) Type { // WithElement returns the flagged type with the same flags but a different element func (ft *FlaggedType) WithElement(t Type) *FlaggedType { + if ft.element.Equals(t) { + return ft // short-circuit + } + var flags []TypeFlag for f := range ft.flags { flags = append(flags, f) diff --git a/hack/generator/pkg/astmodel/map_type.go b/hack/generator/pkg/astmodel/map_type.go index 2701a619ff2..b57cb7a3d85 100644 --- a/hack/generator/pkg/astmodel/map_type.go +++ b/hack/generator/pkg/astmodel/map_type.go @@ -30,6 +30,26 @@ func (m *MapType) ValueType() Type { return m.value } +func (m *MapType) WithKeyType(t Type) *MapType { + if m.key.Equals(t) { + return m + } + + result := *m + result.key = t + return &result +} + +func (m *MapType) WithValueType(t Type) *MapType { + if m.value.Equals(t) { + return m + } + + result := *m + result.value = t + return &result +} + // NewMapType creates a new map with the specified key and value types func NewMapType(key Type, value Type) *MapType { return &MapType{key, value} diff --git a/hack/generator/pkg/astmodel/optional_type.go b/hack/generator/pkg/astmodel/optional_type.go index 78d8590aa62..3444c36c8f8 100644 --- a/hack/generator/pkg/astmodel/optional_type.go +++ b/hack/generator/pkg/astmodel/optional_type.go @@ -19,9 +19,21 @@ type OptionalType struct { element Type } -var _ Type = &OptionalType{} +// type assertions +var ( + _ Type = &OptionalType{} + _ MetaType = &OptionalType{} +) -var _ MetaType = &OptionalType{} +// cache of commonly-used values +var cachedOptionals = map[Type]*OptionalType{ + BoolType: {BoolType}, + FloatType: {FloatType}, + IntType: {IntType}, + StringType: {StringType}, + UInt32Type: {UInt32Type}, + UInt64Type: {UInt64Type}, +} // NewOptionalType creates a new optional type that may or may not have the specified 'element' type func NewOptionalType(element Type) Type { @@ -29,6 +41,10 @@ func NewOptionalType(element Type) Type { return element } + if result, ok := cachedOptionals[element]; ok { + return result + } + return &OptionalType{element} } @@ -52,6 +68,24 @@ func (optional *OptionalType) Element() Type { return optional.element } +func (optional *OptionalType) WithElement(t Type) Type { + if optional.element.Equals(t) { + return optional + } + + if isTypeOptional(t) { + return t + } + + if cached, ok := cachedOptionals[t]; ok { + return cached + } + + result := *optional + result.element = t + return &result +} + func (optional *OptionalType) AsDeclarations(codeGenerationContext *CodeGenerationContext, declContext DeclarationContext) []dst.Decl { return AsSimpleDeclarations(codeGenerationContext, declContext, optional) } diff --git a/hack/generator/pkg/astmodel/resource_type.go b/hack/generator/pkg/astmodel/resource_type.go index b6fdcf7c605..2fbe547bfe6 100644 --- a/hack/generator/pkg/astmodel/resource_type.go +++ b/hack/generator/pkg/astmodel/resource_type.go @@ -153,6 +153,10 @@ func (resource *ResourceType) IsStorageVersion() bool { // WithSpec returns a new resource that has the specified spec type func (resource *ResourceType) WithSpec(specType Type) *ResourceType { + if TypeEquals(resource.spec, specType) { + return resource // short-circuit + } + if specResource, ok := specType.(*ResourceType); ok { // type is a resource, take its SpecType instead // so we don't nest resources @@ -166,6 +170,10 @@ func (resource *ResourceType) WithSpec(specType Type) *ResourceType { // WithStatus returns a new resource that has the specified status type func (resource *ResourceType) WithStatus(statusType Type) *ResourceType { + if TypeEquals(resource.status, statusType) { + return resource // short-circuit + } + if specResource, ok := statusType.(*ResourceType); ok { // type is a resource, take its StatusType instead // so we don't nest resources diff --git a/hack/generator/pkg/astmodel/type_visitor.go b/hack/generator/pkg/astmodel/type_visitor.go index b4f344efc92..423677306d5 100644 --- a/hack/generator/pkg/astmodel/type_visitor.go +++ b/hack/generator/pkg/astmodel/type_visitor.go @@ -121,11 +121,7 @@ func IdentityVisitOfArrayType(this *TypeVisitor, it *ArrayType, ctx interface{}) return nil, errors.Wrap(err, "failed to visit type of array") } - if newElement == it.element { - return it, nil // short-circuit - } - - return NewArrayType(newElement), nil + return it.WithElement(newElement), nil } func IdentityVisitOfPrimitiveType(_ *TypeVisitor, it *PrimitiveType, _ interface{}) (Type, error) { @@ -186,11 +182,7 @@ func IdentityVisitOfMapType(this *TypeVisitor, it *MapType, ctx interface{}) (Ty return nil, errors.Wrapf(err, "failed to visit map value type %q", it.value) } - if visitedKey == it.key && visitedValue == it.value { - return it, nil // short-circuit - } - - return NewMapType(visitedKey, visitedValue), nil + return it.WithKeyType(visitedKey).WithValueType(visitedValue), nil } func IdentityVisitOfEnumType(_ *TypeVisitor, it *EnumType, _ interface{}) (Type, error) { @@ -205,11 +197,7 @@ func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interf return nil, errors.Wrapf(err, "failed to visit optional element type %q", it.element) } - if visitedElement == it.element { - return it, nil // short-circuit - } - - return NewOptionalType(visitedElement), nil + return it.WithElement(visitedElement), nil } func IdentityVisitOfResourceType(this *TypeVisitor, it *ResourceType, ctx interface{}) (Type, error) { @@ -241,7 +229,6 @@ func IdentityVisitOfOneOfType(this *TypeVisitor, it *OneOfType, ctx interface{}) newTypes = append(newTypes, newType) return nil }) - if err != nil { return nil, err } @@ -264,7 +251,6 @@ func IdentityVisitOfAllOfType(this *TypeVisitor, it *AllOfType, ctx interface{}) newTypes = append(newTypes, newType) return nil }) - if err != nil { return nil, err } diff --git a/hack/generator/pkg/astmodel/validated_type.go b/hack/generator/pkg/astmodel/validated_type.go index ca240e7cac9..a6b02cf932a 100644 --- a/hack/generator/pkg/astmodel/validated_type.go +++ b/hack/generator/pkg/astmodel/validated_type.go @@ -156,6 +156,16 @@ func (v *ValidatedType) ElementType() Type { return v.element } +func (v *ValidatedType) WithElement(t Type) Type { + if v.element.Equals(t) { + return v + } + + result := *v + result.element = t + return &result +} + func (v *ValidatedType) Validations() Validations { return v.validations } diff --git a/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go b/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go index 6ebaf99ed82..512a9624fde 100644 --- a/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go +++ b/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go @@ -407,7 +407,7 @@ func (s synthesizer) handleArrayArray(leftArray *astmodel.ArrayType, rightArray return nil, err } - return astmodel.NewArrayType(intersected), nil + return leftArray.WithElement(intersected), nil } func (s synthesizer) handleObjectObject(leftObj *astmodel.ObjectType, rightObj *astmodel.ObjectType) (astmodel.Type, error) { diff --git a/hack/generator/pkg/codegen/pipeline/status_augment.go b/hack/generator/pkg/codegen/pipeline/status_augment.go index 23a4e8b56bb..10ef982d7f2 100644 --- a/hack/generator/pkg/codegen/pipeline/status_augment.go +++ b/hack/generator/pkg/codegen/pipeline/status_augment.go @@ -160,17 +160,12 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter { merger.Add(func(spec *astmodel.OptionalType, status astmodel.Type) (astmodel.Type, error) { // we ignore optionality when matching things up, since there are // discordances between the JSON Schema/Swagger as some teams have handcrafted them - newSpecElement, err := merger.Merge(spec.Element(), status) + newElement, err := merger.Merge(spec.Element(), status) if err != nil { return nil, err } - // return original type if not changed - if newSpecElement.Equals(spec.Element()) { - return spec, nil - } - - return astmodel.NewOptionalType(newSpecElement), nil + return spec.WithElement(newElement), nil }) // handle (Type, Optional) @@ -183,17 +178,12 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter { // handle (FlaggedType, Type); there is no need to handle the opposite direction // as the swagger types won’t be flagged merger.Add(func(spec *astmodel.FlaggedType, status astmodel.Type) (astmodel.Type, error) { - newSpec, err := merger.Merge(spec.Element(), status) + newElement, err := merger.Merge(spec.Element(), status) if err != nil { return nil, err } - // return original type if not changed - if newSpec.Equals(spec.Element()) { - return spec, nil - } - - return spec.WithElement(newSpec), nil + return spec.WithElement(newElement), nil }) // handle (Map, Map) by matching up the keys and values @@ -208,27 +198,17 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter { return nil, err } - // return original type if not changed - if keyResult.Equals(spec.KeyType()) && valueResult.Equals(spec.ValueType()) { - return spec, nil - } - - return astmodel.NewMapType(keyResult, valueResult), nil + return spec.WithKeyType(keyResult).WithValueType(valueResult), nil }) // handle (Array, Array) by matching up the inner types merger.Add(func(spec, status *astmodel.ArrayType) (astmodel.Type, error) { - newSpecElement, err := merger.Merge(spec.Element(), status.Element()) + newElement, err := merger.Merge(spec.Element(), status.Element()) if err != nil { return nil, err } - // return original type if not changed - if newSpecElement.Equals(spec.Element()) { - return spec, nil - } - - return astmodel.NewArrayType(newSpecElement), nil + return spec.WithElement(newElement), nil }) // safety check, (AllOf, AllOf) should not occur