Skip to content

Commit

Permalink
Don't call MethodByName with a variable arg
Browse files Browse the repository at this point in the history
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)
```
  • Loading branch information
jquirke committed Sep 19, 2023
1 parent e57e5d8 commit d8164c3
Showing 1 changed file with 57 additions and 5 deletions.
62 changes: 57 additions & 5 deletions schema/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ import (
"gorm.io/gorm/logger"
)

type callbackType string

const (
_callbackTypeBeforeCreate callbackType = "BeforeCreate"
_callbackTypeBeforeUpdate callbackType = "BeforeUpdate"
_callbackTypeAfterCreate callbackType = "AfterCreate"
_callbackTypeAfterUpdate callbackType = "AfterUpdate"
_callbackTypeBeforeSave callbackType = "BeforeSave"
_callbackTypeAfterSave callbackType = "AfterSave"
_callbackTypeBeforeDelete callbackType = "BeforeDelete"
_callbakAfterDelete callbackType = "AfterDelete"
_callbackTypeAfterFind callbackType = "AfterFind"
)

// ErrUnsupportedDataType unsupported data type
var ErrUnsupportedDataType = errors.New("unsupported data type")

Expand Down Expand Up @@ -288,14 +302,20 @@ func ParseWithSpecialTableName(dest interface{}, cacheStore *sync.Map, namer Nam
}
}

callbacks := []string{"BeforeCreate", "AfterCreate", "BeforeUpdate", "AfterUpdate", "BeforeSave", "AfterSave", "BeforeDelete", "AfterDelete", "AfterFind"}
for _, name := range callbacks {
if methodValue := modelValue.MethodByName(name); methodValue.IsValid() {
callbackTypes := []callbackType{
_callbackTypeBeforeCreate, _callbackTypeAfterCreate,
_callbackTypeBeforeUpdate, _callbackTypeAfterUpdate,
_callbackTypeBeforeSave, _callbackTypeAfterSave,
_callbackTypeBeforeDelete, _callbakAfterDelete,
_callbackTypeAfterFind,
}
for _, cbName := range callbackTypes {
if methodValue := callBackToMethodValue(modelValue, cbName); methodValue.IsValid() {
switch methodValue.Type().String() {
case "func(*gorm.DB) error": // TODO hack
reflect.Indirect(reflect.ValueOf(schema)).FieldByName(name).SetBool(true)
reflect.Indirect(reflect.ValueOf(schema)).FieldByName(string(cbName)).SetBool(true)
default:
logger.Default.Warn(context.Background(), "Model %v don't match %vInterface, should be `%v(*gorm.DB) error`. Please see https://gorm.io/docs/hooks.html", schema, name, name)
logger.Default.Warn(context.Background(), "Model %v don't match %vInterface, should be `%v(*gorm.DB) error`. Please see https://gorm.io/docs/hooks.html", schema, cbName, cbName)
}
}
}
Expand Down Expand Up @@ -349,6 +369,38 @@ func ParseWithSpecialTableName(dest interface{}, cacheStore *sync.Map, namer Nam
return schema, schema.err
}

// This unrolling is needed to show to the compiler the exact set of methods
// that can be used on the modelType.
// Prior to go1.22 any use of MethodByName would cause the linker to
// abandon dead code elimination for the entire binary.
// As of go1.22 the compiler supports one special case of a string constant
// being passed to MethodByName. For enterprise customers or those building
// large binaries, this gives a significant reduction in binary size.
// https://github.com/golang/go/issues/62257
func callBackToMethodValue(modelType reflect.Value, cbType callbackType) reflect.Value {
switch cbType {
case _callbackTypeBeforeCreate:
return modelType.MethodByName(string(_callbackTypeBeforeCreate))
case _callbackTypeAfterCreate:
return modelType.MethodByName(string(_callbackTypeAfterCreate))
case _callbackTypeBeforeUpdate:
return modelType.MethodByName(string(_callbackTypeBeforeUpdate))
case _callbackTypeAfterUpdate:
return modelType.MethodByName(string(_callbackTypeAfterUpdate))
case _callbackTypeBeforeSave:
return modelType.MethodByName(string(_callbackTypeBeforeSave))
case _callbackTypeAfterSave:
return modelType.MethodByName(string(_callbackTypeAfterSave))
case _callbackTypeBeforeDelete:
return modelType.MethodByName(string(_callbackTypeBeforeDelete))
case _callbakAfterDelete:
return modelType.MethodByName(string(_callbakAfterDelete))
case _callbackTypeAfterFind:
return modelType.MethodByName(string(_callbackTypeAfterFind))
}
panic("unreachable")
}

func getOrParse(dest interface{}, cacheStore *sync.Map, namer Namer) (*Schema, error) {
modelType := reflect.ValueOf(dest).Type()
for modelType.Kind() == reflect.Slice || modelType.Kind() == reflect.Array || modelType.Kind() == reflect.Ptr {
Expand Down

0 comments on commit d8164c3

Please sign in to comment.