Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
dominiquelefevre opened this issue Aug 24, 2023 · 3 comments
Closed

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

dominiquelefevre opened this issue Aug 24, 2023 · 3 comments
Assignees
@dominiquelefevre
Labels
binary-size compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@dominiquelefevre
Copy link
Contributor

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().
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 24, 2023
@gopherbot
Copy link
Contributor

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

@thanm thanm added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 24, 2023
@thanm thanm added this to the Go1.22 milestone Aug 24, 2023
@thanm
Copy link
Contributor

thanm commented Aug 24, 2023

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

jquirke pushed a commit to jquirke/go that referenced this issue Sep 16, 2023
Add linker debug log to hunt down reflect methods
686bc3a 
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.
jquirke pushed a commit to jquirke/go that referenced this issue Sep 16, 2023
cmd/link: add debug log to identify deadcode blockers
6d6d8a4 
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.
@jquirke jquirke mentioned this issue Sep 16, 2023
Closed
jquirke added a commit to jquirke/go that referenced this issue Sep 16, 2023
@jquirke
cmd/link: add debug log to identify deadcode blockers
a76c7df 
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.
@jquirke jquirke mentioned this issue Sep 16, 2023
Closed
@gopherbot
Copy link
Contributor

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

jquirke added a commit to jquirke/go that referenced this issue Sep 16, 2023
@jquirke
cmd/link: add debug log to identify deadcode blockers
9d417d5 
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.
jquirke added a commit to jquirke/go that referenced this issue Sep 16, 2023
@jquirke
cmd/link: add debug log to identify deadcode blockers
ae70225 
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.
jquirke added a commit to jquirke/go that referenced this issue Sep 16, 2023
@jquirke
cmd/link: add debug log to identify deadcode blockers
2f3458a 
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.
jquirke added a commit to jquirke/gorm that referenced this issue Sep 17, 2023
@jquirke
Don't call MethodByName with a variable arg
8113e2d 
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)
%
```
@jquirke jquirke mentioned this issue Sep 17, 2023
Merged
jquirke added a commit to jquirke/gorm that referenced this issue Sep 17, 2023
@jquirke
Don't call MethodByName with a variable arg
0db0c43 
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)
%
```
jquirke added a commit to jquirke/gorm that referenced this issue Sep 17, 2023
@jquirke
Don't call MethodByName with a variable arg
7fcaec8 
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)
%
```
jquirke added a commit to jquirke/go that referenced this issue Sep 18, 2023
@jquirke
cmd/link: add debug log to identify deadcode blockers
91f3f6f 
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.
jquirke added a commit to jquirke/go that referenced this issue Sep 18, 2023
@jquirke
cmd/link: log use of reflect
85b6e77 
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.
jquirke added a commit to jquirke/go that referenced this issue Sep 18, 2023
@jquirke
cmd/link: log use of reflect
3429e71 
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.
jquirke added a commit to jquirke/go that referenced this issue Sep 18, 2023
@jquirke
cmd/link: log use of reflect
4a1b970 
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.
jquirke added a commit to jquirke/gorm that referenced this issue Sep 19, 2023
@jquirke
Don't call MethodByName with a variable arg
d8164c3 
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)
```
jquirke added a commit to jquirke/gorm that referenced this issue Sep 19, 2023
@jquirke
Don't call MethodByName with a variable arg
4133824 
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)
```
jquirke added a commit to jquirke/gorm that referenced this issue Sep 19, 2023
@jquirke
Don't call MethodByName with a variable arg
bd2ee81 
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)
```
jquirke added a commit to jquirke/gorm that referenced this issue Sep 19, 2023
@jquirke
Don't call MethodByName with a variable arg
63e1c10 
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)
```
jquirke added a commit to jquirke/gorm that referenced this issue Sep 19, 2023
@jquirke
Don't call MethodByName with a variable arg
a6e25fb 
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)
```
jinzhu pushed a commit to go-gorm/gorm that referenced this issue Oct 10, 2023
@jquirke
Don't call MethodByName with a variable arg (#6602)
8c18714 
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)
```
@golang golang locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dominiquelefevre dominiquelefevre

Labels
binary-size compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
4 participants
@bcmills @gopherbot @thanm @dominiquelefevre