From ca2493f43c299bbaed5f7e5b70f66cc763ff9802 Mon Sep 17 00:00:00 2001 From: dominiquelefevre <54854047+dominiquelefevre@users.noreply.github.com> Date: Tue, 14 Nov 2023 10:07:49 +0200 Subject: [PATCH] fix(storage): do not inhibit the dead code elimination. (#8543) When the go compiler encounters a call to MethodByName(), it assumes that every method of every reachable type can be accessed, and nothing may be eliminated. Go 1.22 has gained support for calls like MethodByName("Foobar"). If the argument to MethodByName() is a compile-time constant, then the compiler no longer disables the DCE, and only keeps methods named Foobar. Rewrite setConditionField() so that MethodByName() is only called with compile-time constants. This way importing cloud.google.com/go/storage will not inhibit the DCE. Co-authored-by: Brenna N Epp --- storage/bucket.go | 4 ++-- storage/storage.go | 55 +++++++++++++++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index 3818c4498c7c..bc4bf101da1c 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -1360,11 +1360,11 @@ func applyBucketConds(method string, conds *BucketConditions, call interface{}) cval := reflect.ValueOf(call) switch { case conds.MetagenerationMatch != 0: - if !setConditionField(cval, "IfMetagenerationMatch", conds.MetagenerationMatch) { + if !setIfMetagenerationMatch(cval, conds.MetagenerationMatch) { return fmt.Errorf("storage: %s: ifMetagenerationMatch not supported", method) } case conds.MetagenerationNotMatch != 0: - if !setConditionField(cval, "IfMetagenerationNotMatch", conds.MetagenerationNotMatch) { + if !setIfMetagenerationNotMatch(cval, conds.MetagenerationNotMatch) { return fmt.Errorf("storage: %s: ifMetagenerationNotMatch not supported", method) } } diff --git a/storage/storage.go b/storage/storage.go index a16e512f5e7d..eb83597c840f 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1806,7 +1806,7 @@ func (c *Conditions) isMetagenerationValid() bool { func applyConds(method string, gen int64, conds *Conditions, call interface{}) error { cval := reflect.ValueOf(call) if gen >= 0 { - if !setConditionField(cval, "Generation", gen) { + if !setGeneration(cval, gen) { return fmt.Errorf("storage: %s: generation not supported", method) } } @@ -1818,25 +1818,25 @@ func applyConds(method string, gen int64, conds *Conditions, call interface{}) e } switch { case conds.GenerationMatch != 0: - if !setConditionField(cval, "IfGenerationMatch", conds.GenerationMatch) { + if !setIfGenerationMatch(cval, conds.GenerationMatch) { return fmt.Errorf("storage: %s: ifGenerationMatch not supported", method) } case conds.GenerationNotMatch != 0: - if !setConditionField(cval, "IfGenerationNotMatch", conds.GenerationNotMatch) { + if !setIfGenerationNotMatch(cval, conds.GenerationNotMatch) { return fmt.Errorf("storage: %s: ifGenerationNotMatch not supported", method) } case conds.DoesNotExist: - if !setConditionField(cval, "IfGenerationMatch", int64(0)) { + if !setIfGenerationMatch(cval, int64(0)) { return fmt.Errorf("storage: %s: DoesNotExist not supported", method) } } switch { case conds.MetagenerationMatch != 0: - if !setConditionField(cval, "IfMetagenerationMatch", conds.MetagenerationMatch) { + if !setIfMetagenerationMatch(cval, conds.MetagenerationMatch) { return fmt.Errorf("storage: %s: ifMetagenerationMatch not supported", method) } case conds.MetagenerationNotMatch != 0: - if !setConditionField(cval, "IfMetagenerationNotMatch", conds.MetagenerationNotMatch) { + if !setIfMetagenerationNotMatch(cval, conds.MetagenerationNotMatch) { return fmt.Errorf("storage: %s: ifMetagenerationNotMatch not supported", method) } } @@ -1897,16 +1897,45 @@ func applySourceCondsProto(gen int64, conds *Conditions, call *storagepb.Rewrite return nil } -// setConditionField sets a field on a *raw.WhateverCall. +// setGeneration sets Generation on a *raw.WhateverCall. // We can't use anonymous interfaces because the return type is // different, since the field setters are builders. -func setConditionField(call reflect.Value, name string, value interface{}) bool { - m := call.MethodByName(name) - if !m.IsValid() { - return false +// We also make sure to supply a compile-time constant to MethodByName; +// otherwise, the Go Linker will disable dead code elimination, leading +// to larger binaries for all packages that import storage. +func setGeneration(cval reflect.Value, value interface{}) bool { + return setCondition(cval.MethodByName("Generation"), value) +} + +// setIfGenerationMatch sets IfGenerationMatch on a *raw.WhateverCall. +// See also setGeneration. +func setIfGenerationMatch(cval reflect.Value, value interface{}) bool { + return setCondition(cval.MethodByName("IfGenerationMatch"), value) +} + +// setIfGenerationNotMatch sets IfGenerationNotMatch on a *raw.WhateverCall. +// See also setGeneration. +func setIfGenerationNotMatch(cval reflect.Value, value interface{}) bool { + return setCondition(cval.MethodByName("IfGenerationNotMatch"), value) +} + +// setIfMetagenerationMatch sets IfMetagenerationMatch on a *raw.WhateverCall. +// See also setGeneration. +func setIfMetagenerationMatch(cval reflect.Value, value interface{}) bool { + return setCondition(cval.MethodByName("IfMetagenerationMatch"), value) +} + +// setIfMetagenerationNotMatch sets IfMetagenerationNotMatch on a *raw.WhateverCall. +// See also setGeneration. +func setIfMetagenerationNotMatch(cval reflect.Value, value interface{}) bool { + return setCondition(cval.MethodByName("IfMetagenerationNotMatch"), value) +} + +func setCondition(setter reflect.Value, value interface{}) bool { + if setter.IsValid() { + setter.Call([]reflect.Value{reflect.ValueOf(value)}) } - m.Call([]reflect.Value{reflect.ValueOf(value)}) - return true + return setter.IsValid() } // Retryer returns an object handle that is configured with custom retry