fix(storage): do not inhibit the dead code elimination. (#8543)

dominiquelefevre · BrennaEpp · web-flow · commit ca2493f43c29 · 2023-11-14T00:07:49.000-08:00
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 &lt;brennae@google.com&gt;
diff --git 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -1360,11 +1360,11 @@ func applyBucketConds(method string, conds *BucketConditions, call interface{})`
`1360`	`1360`	`cval := reflect.ValueOf(call)`
`1361`	`1361`	`switch {`
`1362`	`1362`	`case conds.MetagenerationMatch != 0:`
`1363`		`- if !setConditionField(cval, "IfMetagenerationMatch", conds.MetagenerationMatch) {`
	`1363`	`+ if !setIfMetagenerationMatch(cval, conds.MetagenerationMatch) {`
`1364`	`1364`	`return fmt.Errorf("storage: %s: ifMetagenerationMatch not supported", method)`
`1365`	`1365`	`}`
`1366`	`1366`	`case conds.MetagenerationNotMatch != 0:`
`1367`		`- if !setConditionField(cval, "IfMetagenerationNotMatch", conds.MetagenerationNotMatch) {`
	`1367`	`+ if !setIfMetagenerationNotMatch(cval, conds.MetagenerationNotMatch) {`
`1368`	`1368`	`return fmt.Errorf("storage: %s: ifMetagenerationNotMatch not supported", method)`
`1369`	`1369`	`}`
`1370`	`1370`	`}`