From 4952d9c2bca7df6315492c2918debff335c3dcef Mon Sep 17 00:00:00 2001 From: tiendc Date: Sat, 23 Nov 2024 14:06:09 +0700 Subject: [PATCH] refactor: correcting the cache usage (#23) --- .golangci.yml | 2 +- base_copier.go | 6 +++ build_copier.go | 113 ++++++++++++++++++++++++++--------------------- deepcopy.go | 16 ------- iface_copier.go | 8 ++++ map_copier.go | 8 ++-- slice_copier.go | 28 +----------- struct_copier.go | 16 +------ 8 files changed, 85 insertions(+), 112 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index ed4ef5b..717880b 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,6 @@ linters-settings: funlen: - lines: 100 + lines: 120 statements: 80 gci: sections: diff --git a/base_copier.go b/base_copier.go index a29efb6..379ef27 100644 --- a/base_copier.go +++ b/base_copier.go @@ -18,6 +18,8 @@ func (c *nopCopier) Copy(dst, src reflect.Value) error { return nil } +var defaultNopCopier = &nopCopier{} + // value2PtrCopier data structure of copier that copies from a value to a pointer type value2PtrCopier struct { ctx *Context @@ -93,6 +95,8 @@ func (c *directCopier) Copy(dst, src reflect.Value) error { return nil } +var defaultDirectCopier = &directCopier{} + // convCopier copier that does copying with converting `src` value to `dst` type type convCopier struct { } @@ -101,3 +105,5 @@ func (c *convCopier) Copy(dst, src reflect.Value) error { dst.Set(src.Convert(dst.Type())) return nil } + +var defaultConvCopier = &convCopier{} diff --git a/build_copier.go b/build_copier.go index 92b07e0..b285038 100644 --- a/build_copier.go +++ b/build_copier.go @@ -98,96 +98,109 @@ func defaultContext() *Context { // buildCopier build copier for handling copy from `srcType` to `dstType` // //nolint:gocognit,gocyclo -func buildCopier(ctx *Context, dstType, srcType reflect.Type) (copier, error) { +func buildCopier(ctx *Context, dstType, srcType reflect.Type) (copier copier, err error) { + // Finds cached copier, returns it if found + cacheKey := ctx.createCacheKey(dstType, srcType) + ctx.mu.RLock() + cp, ok := ctx.copierCacheMap[*cacheKey] + ctx.mu.RUnlock() + if ok { + return cp, nil + } + dstKind, srcKind := dstType.Kind(), srcType.Kind() + + // Trivial case + if simpleKindMask&(1< 0 { + if dstType == srcType { + copier = defaultDirectCopier + goto OnComplete + } + if srcType.ConvertibleTo(dstType) { + copier = defaultConvCopier + goto OnComplete + } + } + if dstKind == reflect.Interface { - return &toIfaceCopier{ctx: ctx}, nil + cp := &toIfaceCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } if srcKind == reflect.Interface { - return &fromIfaceCopier{ctx: ctx}, nil + cp := &fromIfaceCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } //nolint:nestif if srcKind == reflect.Pointer { if dstKind == reflect.Pointer { // ptr -> ptr - copier := &ptr2PtrCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err - } - return copier, nil + cp := &ptr2PtrCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } else { // ptr -> value if !ctx.CopyBetweenPtrAndValue { - return onNonCopyable(ctx, dstType, srcType) - } - copier := &ptr2ValueCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err + goto OnNonCopyable } - return copier, nil + cp := &ptr2ValueCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } } else { if dstKind == reflect.Pointer { // value -> ptr if !ctx.CopyBetweenPtrAndValue { - return onNonCopyable(ctx, dstType, srcType) + goto OnNonCopyable } - copier := &value2PtrCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err - } - return copier, nil + cp := &value2PtrCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } } // Both are not Pointers if srcKind == reflect.Slice || srcKind == reflect.Array { if dstKind != reflect.Slice && dstKind != reflect.Array { - return onNonCopyable(ctx, dstType, srcType) - } - copier := &sliceCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err + goto OnNonCopyable } - return copier, nil + cp := &sliceCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } if srcKind == reflect.Struct { if dstKind != reflect.Struct { - return onNonCopyable(ctx, dstType, srcType) - } - copier := &structCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err + goto OnNonCopyable } - return copier, nil + cp := &structCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } if srcKind == reflect.Map { if dstKind != reflect.Map { - return onNonCopyable(ctx, dstType, srcType) + goto OnNonCopyable } - copier := &mapCopier{ctx: ctx} - if err := copier.init(dstType, srcType); err != nil { - return nil, err - } - return copier, nil + cp := &mapCopier{ctx: ctx} + copier, err = cp, cp.init(dstType, srcType) + goto OnComplete } - // Trivial case - if simpleKindMask&(1< 0 { - if dstType == srcType { - return &directCopier{}, nil - } - if srcType.ConvertibleTo(dstType) { - return &convCopier{}, nil +OnComplete: + if err == nil { + if copier != nil { + ctx.mu.Lock() + ctx.copierCacheMap[*cacheKey] = copier + ctx.mu.Unlock() + return copier, err } + } else { + return nil, err } - return onNonCopyable(ctx, dstType, srcType) -} - -func onNonCopyable(ctx *Context, dstType, srcType reflect.Type) (copier, error) { +OnNonCopyable: if ctx.IgnoreNonCopyableTypes { - return &nopCopier{}, nil + return defaultNopCopier, nil } return nil, fmt.Errorf("%w: %v -> %v", ErrTypeNonCopyable, srcType, dstType) } diff --git a/deepcopy.go b/deepcopy.go index 4360070..05d904d 100644 --- a/deepcopy.go +++ b/deepcopy.go @@ -92,26 +92,10 @@ func Copy(dst, src any, options ...Option) (err error) { } ctx.prepare() - cacheKey := ctx.createCacheKey(dstType, srcType) - if ctx.UseGlobalCache { - ctx.mu.RLock() - cp, ok := ctx.copierCacheMap[*cacheKey] - ctx.mu.RUnlock() - if ok { - return cp.Copy(dstVal, srcVal) - } - } - cp, err := buildCopier(ctx, dstType, srcType) if err != nil { return err } - if ctx.UseGlobalCache { - ctx.mu.Lock() - ctx.copierCacheMap[*cacheKey] = cp - ctx.mu.Unlock() - } - return cp.Copy(dstVal, srcVal) } diff --git a/iface_copier.go b/iface_copier.go index 1c9bead..196da6b 100644 --- a/iface_copier.go +++ b/iface_copier.go @@ -9,6 +9,10 @@ type fromIfaceCopier struct { ctx *Context } +func (c *fromIfaceCopier) init(dst, src reflect.Type) error { + return nil +} + // Copy implementation of Copy function for from-iface copier func (c *fromIfaceCopier) Copy(dst, src reflect.Value) error { for src.Kind() == reflect.Interface { @@ -30,6 +34,10 @@ type toIfaceCopier struct { ctx *Context } +func (c *toIfaceCopier) init(dst, src reflect.Type) error { + return nil +} + // Copy implementation of Copy function for to-iface copier func (c *toIfaceCopier) Copy(dst, src reflect.Value) error { for src.Kind() == reflect.Interface { diff --git a/map_copier.go b/map_copier.go index 6d84f8e..c7ed3c5 100644 --- a/map_copier.go +++ b/map_copier.go @@ -44,24 +44,24 @@ func (c *mapCopier) init(dstType, srcType reflect.Type) error { dstKeyType, dstValType := dstType.Key(), dstType.Elem() buildKeyCopier, buildValCopier := true, true - // OPTIMIZATION: buildCopier() can handle this nicely, but it will add another wrapping layer + // OPTIMIZATION: buildCopier() can handle this nicely if simpleKindMask&(1< 0 { if srcKeyType == dstKeyType { // Just keep c.keyCopier = nil buildKeyCopier = false } else if srcKeyType.ConvertibleTo(dstKeyType) { - c.keyCopier = &mapItemCopier{dstType: dstKeyType, copier: &convCopier{}} + c.keyCopier = &mapItemCopier{dstType: dstKeyType, copier: defaultConvCopier} buildKeyCopier = false } } - // OPTIMIZATION: buildCopier() can handle this nicely, but it will add another wrapping layer + // OPTIMIZATION: buildCopier() can handle this nicely if simpleKindMask&(1< 0 { if srcValType == dstValType { // Just keep c.valueCopier = nil buildValCopier = false } else if srcValType.ConvertibleTo(dstValType) { - c.valueCopier = &mapItemCopier{dstType: dstValType, copier: &convCopier{}} + c.valueCopier = &mapItemCopier{dstType: dstValType, copier: defaultConvCopier} buildValCopier = false } } diff --git a/slice_copier.go b/slice_copier.go index e84bfe5..fd227b9 100644 --- a/slice_copier.go +++ b/slice_copier.go @@ -48,32 +48,6 @@ func (c *sliceCopier) Copy(dst, src reflect.Value) error { } func (c *sliceCopier) init(dstType, srcType reflect.Type) (err error) { - dstType, srcType = dstType.Elem(), srcType.Elem() - srcKind := srcType.Kind() - - // OPTIMIZATION: buildCopier() can handle this nicely, but it will add another wrapping layer - if simpleKindMask&(1< 0 { - if srcType == dstType { - c.itemCopier = &directCopier{} - return nil - } - if srcType.ConvertibleTo(dstType) { - c.itemCopier = &convCopier{} - return nil - } - } - - // OPTIMIZATION: buildCopier() can handle this nicely, but it will add another wrapping layer - if srcKind == reflect.Struct { - c.ctx.mu.RLock() - cp, ok := c.ctx.copierCacheMap[*c.ctx.createCacheKey(dstType, srcType)] - c.ctx.mu.RUnlock() - if ok { - c.itemCopier = cp - return nil - } - } - - c.itemCopier, err = buildCopier(c.ctx, dstType, srcType) + c.itemCopier, err = buildCopier(c.ctx, dstType.Elem(), srcType.Elem()) return } diff --git a/struct_copier.go b/struct_copier.go index 3e37373..417abf8 100644 --- a/struct_copier.go +++ b/struct_copier.go @@ -29,15 +29,6 @@ func (c *structCopier) Copy(dst, src reflect.Value) error { //nolint:gocognit,gocyclo func (c *structCopier) init(dstType, srcType reflect.Type) (err error) { - cacheKey := c.ctx.createCacheKey(dstType, srcType) - c.ctx.mu.RLock() - cp, ok := c.ctx.copierCacheMap[*cacheKey] - c.ctx.mu.RUnlock() - if ok { - c.fieldCopiers = cp.(*structCopier).fieldCopiers //nolint:forcetypeassert - return nil - } - var dstCopyingMethods map[string]*reflect.Method if c.ctx.CopyBetweenStructFieldAndMethod { dstCopyingMethods = c.parseCopyingMethods(dstType) @@ -109,9 +100,6 @@ func (c *structCopier) init(dstType, srcType reflect.Type) (err error) { } } - c.ctx.mu.Lock() - c.ctx.copierCacheMap[*cacheKey] = c - c.ctx.mu.Unlock() return nil } @@ -206,7 +194,7 @@ func (c *structCopier) parseAllNestedFields(typ reflect.Type, index []int) map[s func (c *structCopier) buildCopier(dstType, srcType reflect.Type, dstDetail, srcDetail *fieldDetail) (copier, error) { df, sf := dstDetail.field, srcDetail.field - // OPTIMIZATION: buildCopier() can handle this nicely, but it will add another wrapping layer + // OPTIMIZATION: buildCopier() can handle this nicely if simpleKindMask&(1< 0 { if sf.Type == df.Type { // NOTE: pass nil to unset custom copier and trigger direct copying. @@ -214,7 +202,7 @@ func (c *structCopier) buildCopier(dstType, srcType reflect.Type, dstDetail, src return c.createField2FieldCopier(dstDetail, srcDetail, nil), nil } if sf.Type.ConvertibleTo(df.Type) { - return c.createField2FieldCopier(dstDetail, srcDetail, &convCopier{}), nil + return c.createField2FieldCopier(dstDetail, srcDetail, defaultConvCopier), nil } }