Don't call MethodByName with a variable arg

[jquirke]

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)

• [ X ] Do only one thing
• [ X ] Non breaking API changes
• [ X ] Tested

What did this pull request do?

It aims to stop any large Go project that imports gorm from having the link-level dead code elimination (DCE) turned off. This can result in the final binary being floated by 100+ MB.

While Go doesn't have a long term solution for this problem yet, the work done by Kubernetes can be leveraged in GORM to ensure GORM is not one of the packages that disable (DCE).

User Case Description

Users who use GORM in very large binaries, such as the ones used here at Uber (100s of MB) may notice a drop in binary size, provided no single package breaks the DCE.

[saeidee]

We shouldn't panic here otherwise it will be breaking changes; you may return an error to the caller.

[jquirke]

There is no breaking change here. None of this is consumer visible aside from the deadcode elimination benefits.

The only way this panic can happen is an internal issue with the GORM package. For example, if a new callback method is added but not added as a case in this switch block, you probably want this to panic during development.

And to me is seems the only reason this reflection was improperly used in the first place is due to a design issue - namely avoiding an import cycle; because the callback interfaces accept concrete types.

Believe me, I take no enjoyment in putting one workaround on top of another.

[saeidee]

We have a set of predefined callbacks, maybe returning just an empty reflect.value would be better instead of panic .

[ckganesan]

@jquirke

Could you please update the code by replacing multiple case statements with the specified approach? Additionally, consider adding a nil value to handle cases where there's no match to prevent potential panics.

func callBackToMethodValue(modelType reflect.Value, cbType callbackType) reflect.Value {
	switch cbType {
	case _callbackTypeBeforeCreate,
		_callbackTypeAfterCreate,
		_callbackTypeBeforeUpdate,
		_callbackTypeAfterUpdate,
		_callbackTypeBeforeSave,
		_callbackTypeAfterSave,
		_callbackTypeBeforeDelete,
		_callbackAfterDelete,
		_callbackTypeAfterFind:
		return modelType.MethodByName(string(cbType))
	default:
		return reflect.ValueOf(nil)
	}
}

[jquirke]

1/ I've switch it to reflect.ValueOf(nil)

2/ @ckganesan if I write the switch statement like this, cbType is no longer a constant, and it defeats the purpose of this PR in the first place. Why why would I write such convoluted code if there wasn't an actual benefit behind it?

Indeed, if I do this the Go1.22 compiler, as it stands, won't be able to prove that only specific methods are used. It will fallback to adding IsAttrReflect to the method's symbol flags:

gorm.io/gorm/schema_test.TestAdvancedDataTypeValuerAndSetter -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod>

Indeed, this is the optimisation we are trying to deliberately exploit:

https://go-review.googlesource.com/c/go/+/522436