Skip to content

Restore dead code elimination for callbacks by reverting to constant MethodByName lookups#7643

Open
jquirke wants to merge 2 commits intogo-gorm:masterfrom
jquirke:deadcodereflectworkaround
Open

Restore dead code elimination for callbacks by reverting to constant MethodByName lookups#7643
jquirke wants to merge 2 commits intogo-gorm:masterfrom
jquirke:deadcodereflectworkaround

Conversation

@jquirke
Copy link
Copy Markdown
Contributor

@jquirke jquirke commented Nov 8, 2025

Summary

Problem

Commit 725aa5b replaced the specialized function with MethodByName(string(cbName)) to fix data race issues, but inadvertently broke dead code elimination. Using "MethodByName with a variable parameter prevents optimization, potentially increasing binary size" for large projects.

Solution

  • Restored callBackToMethodValue function with explicit string constants
  • Replaced dynamic calls with callBackToMethodValue(modelValue, cbName)
  • Added strong documentation explaining why optimization is critical
  • Preserved all data race fixes from the original commit

Impact

  • Enables dead code elimination for enterprise customers with large binaries
  • Maintains thread safety fixes from the original commit
  • No functional API changes

Test Plan

  • All existing schema tests pass
  • Data race test continues to pass
  • Code compiles successfully
  • No breaking changes to public API

@propel-code-bot
Copy link
Copy Markdown
Contributor

propel-code-bot bot commented Nov 8, 2025 •
edited
Loading

Re-introduce callBackToMethodValue to re-enable dead-code elimination in callback detection

Restores a specialized reflection helper that was removed in commit 725aa5b, switching callback resolution back to constant-based MethodByName invocations. This change prevents the Go linker from retaining unused callback methods, recovering binary size optimizations while keeping the earlier data-race fixes. A comprehensive test suite (schema/deadcode_test.go) is added to guard the behaviour and document the optimisation rationale.

Key Changes

• Added callBackToMethodValue, getCRUDCallbackMethod, and getLifecycleCallbackMethod in schema/schema.go with exhaustive switches over callbackType string constants
• Replaced dynamic modelValue.MethodByName(string(cbName)) with callBackToMethodValue(modelValue, cbName) inside the callback detection loop
• Inserted detailed comments explaining compiler/linker behaviour and referencing go#62257
• Added ~130-line test file schema/deadcode_test.go including three focused tests: detection correctness, optimisation preservation, and documentation guard
• Incremented callback detection unit tests to validate true/false flags across models with and without hooks

Affected Areas

• schema/schema.go callback resolution logic
• Unit test coverage in schema/deadcode_test.go

This summary was automatically generated by @propel-code-bot

@jquirke jquirke mentioned this pull request Nov 8, 2025
Closed
4 tasks
@jquirke jquirke marked this pull request as draft November 8, 2025 01:59
@jquirke jquirke force-pushed the deadcodereflectworkaround branch from df6d123 to eb88346 Compare November 8, 2025 02:06
@jquirke
Restore callBackToMethodValue function to fix dead code elimination (c…
08cdeab 
…loses go-gorm#7622)

## Summary
- Restores the `callBackToMethodValue` function removed in commit 725aa5b
- Fixes dead code elimination optimization while preserving data race fixes
- Adds comprehensive documentation warning against future modifications
- Closes issue go-gorm#7622

## Problem
Commit 725aa5b replaced the specialized function with `MethodByName(string(cbName))` to fix data race issues, but inadvertently broke dead code elimination. Using "MethodByName with a variable parameter prevents optimization, potentially increasing binary size" for large projects.

## Solution
- Restored `callBackToMethodValue` function with explicit string constants
- Replaced dynamic calls with `callBackToMethodValue(modelValue, cbName)`
- Added strong documentation explaining why optimization is critical
- Preserved all data race fixes from the original commit

## Impact
- Enables dead code elimination for enterprise customers with large binaries
- Maintains thread safety fixes from the original commit
- No functional API changes

## Test Plan
- All existing schema tests pass
- Data race test continues to pass
- Code compiles successfully
- No breaking changes to public API
@jquirke jquirke force-pushed the deadcodereflectworkaround branch from eb88346 to 08cdeab Compare November 8, 2025 02:13
@jquirke jquirke marked this pull request as ready for review November 8, 2025 02:27
vincentbernat added a commit to akvorado/akvorado that referenced this pull request Nov 21, 2025
@vincentbernat
build: disable tracing support for gRPC
5ae5d96 
The goal is to be able to reenable full dead code elimination that is
prevented by non-constant calls to reflect.MethodByName(). There are
many other culprits for this:

```
 08:04 ❱ go  build -o /dev/null -tags release,grpcnotrace -ldflags=-dumpdep  .  |& go run github.com/aarzilli/whydeadcode@latest
github.com/go-playground/validator/v10.tryCallValidateFn reachable from:
         github.com/go-playground/validator/v10.isValidateFn
         github.com/go-playground/validator/v10.isValidateFn·f
         github.com/go-playground/validator/v10.map.init.11
         github.com/go-playground/validator/v10.bakedInValidators
         github.com/go-playground/validator/v10.New
         akvorado/common/helpers.init.1
         akvorado/common/helpers..inittask
         go:main.inittasks
         _

github.com/expr-lang/expr/builtin.get reachable from:
         github.com/expr-lang/expr/builtin.get·f
         github.com/expr-lang/expr/builtin..stmp_53
         github.com/expr-lang/expr/builtin..stmp_0
         github.com/expr-lang/expr/builtin.init
         github.com/expr-lang/expr/builtin..inittask
         go:main.inittasks
         _

github.com/expr-lang/expr/checker/nature.Nature.All reachable from:
         type:github.com/expr-lang/expr/checker/nature.Nature
         type:func() github.com/expr-lang/expr/checker/nature.Nature
         type:github.com/expr-lang/expr/ast.Node
         type:github.com/expr-lang/expr/vm.Program
         type:*github.com/expr-lang/expr/vm.Program
         type:akvorado/outlet/core.ExporterClassifierRule
         type:[]akvorado/outlet/core.ExporterClassifierRule
         type:akvorado/outlet/core.Configuration
         type:akvorado/cmd.OutletConfiguration
         type:[]akvorado/cmd.OutletConfiguration
         type:akvorado/cmd.OrchestratorConfiguration
         type:*akvorado/cmd.OrchestratorConfiguration
         internal/abi..dict.TypeFor[akvorado/cmd.OrchestratorConfiguration]
         reflect..dict.TypeFor[akvorado/cmd.OrchestratorConfiguration]
         akvorado/cmd.init.6.orchestratorClickHouseMigrationHook.func2
         akvorado/cmd.init.6.orchestratorClickHouseMigrationHook.func2·f
         akvorado/cmd.init.6
         akvorado/cmd..inittask
         go:main.inittasks
         _

text/template.(*state).evalField reachable from:
         text/template.(*state).evalFieldChain
         text/template.(*state).evalCommand
         text/template.(*state).evalPipeline
         text/template.(*state).walk
         text/template.(*Template).execute
         akvorado/console/authentication.(*Component).UserAuthentication.func1
         akvorado/console/authentication.(*Component).UserAuthentication
         type:*akvorado/console/authentication.Component
         akvorado/cmd.consoleStart
         akvorado/cmd.init.func3
         akvorado/cmd.init.func3·f
         akvorado/cmd..stmp_2
         akvorado/cmd.init
         akvorado/cmd..inittask
         go:main.inittasks
         _

gorm.io/gorm/schema.ParseWithSpecialTableName reachable from:
         gorm.io/gorm.(*DB).assignInterfacesToValue
         gorm.io/gorm.(*DB).FirstOrInit
         type:*gorm.io/gorm.DB
         type:akvorado/console/database.Component
         akvorado/cmd.consoleStart
         akvorado/cmd.init.func3
         akvorado/cmd.init.func3·f
         akvorado/cmd..stmp_2
         akvorado/cmd.init
         akvorado/cmd..inittask
         go:main.inittasks
         _

gorm.io/gorm/schema.ParseWithSpecialTableName reachable from:
         gorm.io/gorm.(*DB).assignInterfacesToValue
         gorm.io/gorm.(*DB).FirstOrInit
         type:*gorm.io/gorm.DB
         type:akvorado/console/database.Component
         type:*akvorado/console/database.Component
         akvorado/cmd.consoleStart
         akvorado/cmd.init.func3
         akvorado/cmd.init.func3·f
         akvorado/cmd..stmp_2
         akvorado/cmd.init
         akvorado/cmd..inittask
         go:main.inittasks
         _

github.com/expr-lang/expr/vm/runtime.FetchMethod reachable from:
         github.com/expr-lang/expr/vm.(*VM).Run
         github.com/expr-lang/expr/vm.Run
         akvorado/outlet/core.(*ExporterClassifierRule).exec
         akvorado/outlet/core.(*Component).classifyExporter
         akvorado/outlet/core.(*worker).enrichFlow
         akvorado/outlet/core.(*worker).processIncomingFlow.func1
         akvorado/outlet/core.(*worker).processIncomingFlow
         akvorado/outlet/core.(*worker).processIncomingFlow-fm
         akvorado/outlet/core.(*Component).newWorker
         akvorado/outlet/core.(*Component).newWorker-fm
         akvorado/outlet/core.(*Component).Start
         type:*akvorado/outlet/core.Component
         akvorado/cmd.outletStart
         akvorado/cmd.init.func7
         akvorado/cmd.init.func7·f
         akvorado/cmd..stmp_6
         akvorado/cmd.init
         akvorado/cmd..inittask
         go:main.inittasks
         _
```

For gorm, we have go-gorm/gorm#7643. We could
replace text/template by something else as our usage is quite light. For
validator, it may be addressable upstream by disallowing calls to
configurable validator function through a tag. For expr, it looks more
complex.

Then, regression should be detected by CI.
@vincentbernat vincentbernat mentioned this pull request Nov 22, 2025
Draft
7 tasks
@artem-anisimov-0x7f
Copy link
Copy Markdown

artem-anisimov-0x7f commented Mar 14, 2026

Hey, does anyone still work on gorm? @jinzhu, @miladev95, @a631807682, @demoManito?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jquirke @artem-anisimov-0x7f