cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled. #62257

dominiquelefevre · 2023-08-24T07:33:17Z

Normally, a call to MethodByName() disables the DCE because the linker assumes that any method can be accessed this way. This pessimises the code generation for k8s.io/apimachinery which needs MethodByName() to verify whether or not a struct implements DeepCopyInto(). It cannot cast a struct to interface { DeepCopyInto() Foo } because the return type may vary. Instead, it does the following:

      if m := reflect.ValueOf(obj).MethodByName("DeepCopyInto"); ... {

In this case there is no need to disable the DCE altogether. It suffices to add a relocation to keep methods named DeepCopyInto().

The text was updated successfully, but these errors were encountered:

gopherbot · 2023-08-24T08:16:54Z

Change https://go.dev/cl/522436 mentions this issue: cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled.

thanm · 2023-08-24T13:11:03Z

Thanks for sending this. I touched up the reviewer set for your CLs and ran the trybots.

In deadcode removal in the linker, use of .Method(int) and MethodByName(string) cause the deadcode analysis to give up, due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted optimisation added to special case MethodByName and Method with constant arguments, which at Uber has resulted in fewer transitive packages breaking DCE. However, there are still a number of non trivial cases which we would like to easily identify in our monorepos, and loggign would be useful here.

gopherbot · 2023-09-16T22:58:17Z

Change https://go.dev/cl/528895 mentions this issue: cmd/link: add debug log to identify deadcode blockers

In deadcode removal in the linker, use of .Method(int) and MethodByName(string)cause the deadcode analysis to give up, due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted optimisation added to special case MethodByName and Method with constant arguments, which at Uber has resulted in fewer transitive packages breaking DCE. However, there are still a number of non trivial cases which we would like to easily identify in our monorepos, and logging would be useful here.

In deadcode removal in the linker, use of .Method(int) and MethodByName(string) cause the deadcode analysis to give up, due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted optimisation added to special case MethodByName and Method with constant arguments, which at Uber has resulted in fewer transitive packages breaking DCE. However, there are still a number of non trivial cases which we would like to easily identify in our monorepos, and logging would be useful here.

In deadcode removal in the linker, use of .Method(int) and MethodByName(string) cause the deadcode analysis to give up, due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted logic added to special case MethodByName and Method with constant arguments, and avoid marking those as AttrReflectMethod. At Uber has resulted in fewer transitive packages breaking DCE. However, there are still a number of non trivial cases which we would like to easily identify in our monorepos, and logging would be useful here.

Go 1.22 goes somewhat toward addressing the issue of calls to reflection MethodByName disabling linker deadcode elimination (DCE) resulting in huge binaries by using GORM beause the linker cannot prune unused code that *could* be reached via reflection. Go Issue golang/go#62257 reduces the number of incidences by leveraging a compiler assist to avoid marking functions containing calls to MethodByName as ReflectMethods. An analysis of Uber Technologies code base however shows that a number of transitive imports still contain calls to MethodByName with a variable argument. In the case of GORM, the solution we are proposing is because the number of possible methods is finite, we will "unroll" this. This demonstrably shows that GORM is not longer a problem for DCE. ``` Before ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 % go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<3938> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<119948> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<119949> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<49693> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117898> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117899> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<54029> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117635> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117636> ok gorm.io/gorm (cached) ok gorm.io/gorm/callbacks (cached) ? gorm.io/gorm/migrator [no test files] ok gorm.io/gorm/clause (cached) ok gorm.io/gorm/logger (cached) ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm/schema (cached) ok gorm.io/gorm/utils (cached) % ``` After ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 %go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') ? gorm.io/gorm/migrator [no test files] ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm 0.474s ok gorm.io/gorm/callbacks 0.372s ok gorm.io/gorm/clause 0.605s ok gorm.io/gorm/logger (cached) ok gorm.io/gorm/schema 0.689s ok gorm.io/gorm/utils (cached) % ```

Go 1.22 goes somewhat toward addressing the issue of calls to reflection MethodByName disabling linker deadcode elimination (DCE) resulting in huge binaries by using GORM beause the linker cannot prune unused code that *could* be reached via reflection. Go Issue golang/go#62257 reduces the number of incidences by leveraging a compiler assist to avoid marking functions containing calls to MethodByName as ReflectMethods. An analysis of Uber Technologies code base however shows that a number of transitive imports still contain calls to MethodByName with a variable argument. In the case of GORM, the solution we are proposing is because the number of possible methods is finite, we will "unroll" this. This demonstrably shows that GORM is not longer a problem for DCE. Before ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 % go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<3938> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<119948> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<119949> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<49693> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117898> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117899> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<54029> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117635> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117636> ok gorm.io/gorm (cached) ok gorm.io/gorm/callbacks (cached) ? gorm.io/gorm/migrator [no test files] ok gorm.io/gorm/clause (cached) ok gorm.io/gorm/logger (cached) ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm/schema (cached) ok gorm.io/gorm/utils (cached) % ``` After ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 %go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') ? gorm.io/gorm/migrator [no test files] ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm 0.474s ok gorm.io/gorm/callbacks 0.372s ok gorm.io/gorm/clause 0.605s ok gorm.io/gorm/logger (cached) ok gorm.io/gorm/schema 0.689s ok gorm.io/gorm/utils (cached) % ```

Go 1.22 goes somewhat toward addressing the issue using reflect MethodByName disabling linker deadcode elimination (DCE) and the resultant large increase in binary size because the linker cannot prune unused code because it might be reached via reflection. Go Issue golang/go#62257 reduces the number of incidences of this problem by leveraging a compiler assist to avoid marking functions containing calls to MethodByName as ReflectMethods as long as the arguments are constants. An analysis of Uber Technologies code base however shows that a number of transitive imports still contain calls to MethodByName with a variable argument, including GORM. In the case of GORM, the solution we are proposing is because the number of possible methods is finite, we will "unroll" this. This demonstrably shows that GORM is not longer a problem for DCE. Before ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 % go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<3938> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<119948> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<119949> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<49693> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117898> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117899> deadcode: giving up on static analysis due to relect method symbol gorm.io/gorm/schema.ParseWithSpecialTableName<54029> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).Method<117635> deadcode: giving up on static analysis due to relect method symbol reflect.(*Value).MethodByName<117636> ok gorm.io/gorm (cached) ok gorm.io/gorm/callbacks (cached) ? gorm.io/gorm/migrator [no test files] ok gorm.io/gorm/clause (cached) ok gorm.io/gorm/logger (cached) ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm/schema (cached) ok gorm.io/gorm/utils (cached) % ``` After ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 %go test ./... -ldflags='-v=2' 2> >(grep -i 'deadcode: giving up') ? gorm.io/gorm/migrator [no test files] ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm 0.474s ok gorm.io/gorm/callbacks 0.372s ok gorm.io/gorm/clause 0.605s ok gorm.io/gorm/logger (cached) ok gorm.io/gorm/schema 0.689s ok gorm.io/gorm/utils (cached) % ```

In deadcode removal in the linker, use of reflect.Type.Method/MethodByName result in an increased number of reachability of false positives of assumed reachable code, due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted improvement added to special case MethodByName and Method with constant arguments, and avoid marking those as AttrReflectMethod. At Uber this optimisation has resulted in smaller binary sizes when the the usage of reflect.Type.Method/ByName had constant arguments. However, there are still a number of less trivial cases which we would like to easily identify in our codebase, and logging would be helpful here.

In deadcode removal in the linker, any use of reflect.Type.Method/MethodByName result in an increased amount of incorrectly assumed reachable code (specifically methods), due to the intractability of knowing what methods might be used. In golang#62257 there was some compiler assisted improvement added to special case MethodByName and Method with constant arguments, and avoid marking those as AttrReflectMethod. At Uber this optimisation has resulted in smaller binary sizes when the the usage of reflect.Type.Method/ByName had constant arguments. However, there are still a number of less trivial cases which we would like to easily identify in our codebase, and logging would be helpful here.

Go 1.22 goes somewhat toward addressing the issue using reflect MethodByName disabling linker deadcode elimination (DCE) and the resultant large increase in binary size because the linker cannot prune unused code because it might be reached via reflection. Go Issue golang/go#62257 reduces the number of incidences of this problem by leveraging a compiler assist to avoid marking functions containing calls to MethodByName as ReflectMethods as long as the arguments are constants. An analysis of Uber Technologies code base however shows that a number of transitive imports still contain calls to MethodByName with a variable argument, including GORM. In the case of GORM, the solution we are proposing is because the number of possible methods is finite, we will "unroll" this. This demonstrably shows that GORM is not longer a problem for DCE. Before ``` % go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 % go test ./... -ldflags=-dumpdep 2> >(grep -i -e '->.*<reflectmethod>') gorm.io/gorm.(*Statement).BuildCondition -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod> ok gorm.io/gorm (cached) ok gorm.io/gorm/callbacks (cached) gorm.io/gorm/clause_test.BenchmarkComplexSelect -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod> ? gorm.io/gorm/migrator [no test files] ok gorm.io/gorm/clause (cached) ok gorm.io/gorm/logger (cached) gorm.io/gorm/schema_test.TestAdvancedDataTypeValuerAndSetter -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod> type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod> ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm/schema (cached) ok gorm.io/gorm/utils (cached) ``` After ``` %go version go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64 %go test ./... -ldflags=-dumpdep 2> >(grep -i -e '->.*<reflectmethod>') ok gorm.io/gorm (cached) ok gorm.io/gorm/callbacks (cached) ? gorm.io/gorm/migrator [no test files] ? gorm.io/gorm/utils/tests [no test files] ok gorm.io/gorm/clause (cached) ok gorm.io/gorm/logger (cached) ok gorm.io/gorm/schema (cached) ok gorm.io/gorm/utils (cached) ```

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 24, 2023

thanm added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 24, 2023

thanm added this to the Go1.22 milestone Aug 24, 2023

bcmills added the binary-size label Aug 25, 2023

mknyszek assigned dominiquelefevre Aug 30, 2023

gopherbot closed this as completed in e802f21 Sep 1, 2023

jquirke mentioned this issue Sep 16, 2023

cmd/link: add debug log to identify deadcode blockers #62680

Closed

jquirke mentioned this issue Sep 16, 2023

cmd/link: add debug log to identify deadcode blockers #62681

Closed

jquirke mentioned this issue Sep 17, 2023

Don't call MethodByName with a variable arg go-gorm/gorm#6602

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled. #62257

cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled. #62257

dominiquelefevre commented Aug 24, 2023

gopherbot commented Aug 24, 2023

thanm commented Aug 24, 2023

gopherbot commented Sep 16, 2023

cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled. #62257

cmd/compile: special-case MethodByName(string literal) to keep the DCE enabled. #62257

Comments

dominiquelefevre commented Aug 24, 2023

gopherbot commented Aug 24, 2023

thanm commented Aug 24, 2023

gopherbot commented Sep 16, 2023