vendor a method-less text/template to restore whole-binary linker DCE#8844
Open
rchildress87 wants to merge 2 commits into
Open
vendor a method-less text/template to restore whole-binary linker DCE#8844rchildress87 wants to merge 2 commits into
rchildress87 wants to merge 2 commits into
Conversation
…/template text/template's field evaluator (evalField) calls reflect.Value.MethodByName with a non-constant name. The Go linker treats a reachable non-constant MethodByName as a signal to disable method-level dead-code elimination for the entire binary (golang/go#72895). gojsonschema is reached unconditionally from ast.Compiler.Compile via formatErrorDescription, so every embedder of OPA's compiler keeps the trigger live and retains its full reachable method surface, a large binary-size regression (open-policy-agent#7903). Vendor a self-contained, method-less copy of text/template under internal/methodlesstemplate (doc.go/exec.go/funcs.go/option.go/template.go from Go 1.25.8, plus internal/fmtsort), reusing stdlib text/template/parse unchanged. The only edit to the copied code is removing the MethodByName branch in exec.go's evalField (method resolution on the data value); the files are otherwise byte-identical to their Go release, so re-syncing to a newer Go is a diff-and-reapply of that single branch removal. Go's BSD LICENSE is preserved, and the package is added to the golangci-lint path exclusions (matching internal/gojsonschema) so upstream stdlib style is not linted against OPA's rules. Point the error formatter at it. Locale format strings expand placeholders over ErrorDetails (map[string]any), which has no methods, so this is behavior-identical; the engine is retained in full, so ErrorTemplateFuncs (its FuncMap extension point) keeps working unchanged. This adds no external dependency. TestNoStdlibTextTemplateImport guards against reintroducing the stdlib import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Dick Childress <dick.childress@icearp.net>
… template The strings.render_template builtin used stdlib text/template, whose evalField calls reflect.Value.MethodByName. A reachable non-constant MethodByName makes the Go linker disable method-level dead-code elimination for the entire binary (golang/go#72895); the builtin is registered in the topdown builtin table, so it is reachable in every embedder that links Rego evaluation (open-policy-agent#7903). Point the builtin at internal/methodlesstemplate (the vendored method-less copy added in the preceding commit; no external dependency). Rego values decode to map[string]any/[]any/scalars, which have no methods, so this is behavior-identical: the v0/v1 rendertemplate conformance cases (range/if/variables and missing-key -> <undefined>) pass unchanged. Full template syntax is preserved. Add TestNoStdlibTextTemplateImport to prevent silently reintroducing the import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Dick Childress <dick.childress@icearp.net>
Contributor
|
Thanks for contributing. This looks good, let's push it over the line. The test failure seems genuine? Please take a look. |
Author
|
Agreed. It does look genuine. However, I was unable to recreate locally yesterday or today: I'm admittedly not very familiar with testing and pipelines on this project. Anything else I should be doing besides |
Author
|
I think this may just be a one-off with that particular test. @srenatus Will you re-run failed jobs for me when you have a moment? |
Author
|
@srenatus, thank you. |
Contributor
|
Cool. Say, would you mind adding a script that replicates what needs to be done for pulling in the sources from Go? It would be nice to just use, say, |
Author
|
Sounds reasonable. Let me see what I can put together.
…On Friday, July 3rd, 2026 at 3:12 AM, Stephan Renatus ***@***.***> wrote:
srenatus left a comment (open-policy-agent/opa#8844)
Cool. Say, would you mind adding a script that replicates what needs to be done for pulling in the sources from Go? It would be nice to just use, say, `./refresh.sh` when there's a new stdlib with (potential) changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you authored the thread.
|
Contributor
|
Please don't give up. Would be a shame to leave this to rot. 😅 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why the changes in this PR are needed?
text/template's field evaluator (text/template.(*state).evalField,exec.go) callsreflect.Value.MethodByNamewith a non-constant name. The Go linker treats a reachable non-constantMethodByNameas a signal to disable method-level dead-code elimination for the whole binary (seecmd/link/internal/ld/deadcode.goand golang/go#72895). Two OPA code paths pull stdlibtext/templateinto the reachable graph of ordinary embedders:ast.Compiler.Compile → … → gojsonschema.formatErrorDescription → text/template. Reached unconditionally by anything that compiles Rego.strings.render_templatebuiltin (v1/topdown/template.go) — registered in the topdown builtin table, reachable in anything that links Rego evaluation.So an embedder of OPA's compiler/eval retains its entire reachable method surface — a large binary-size regression, hundreds of MB in the reporter's case (#7903). Both edges must go before the linker re-enables method-level DCE for that embedder.
What are the changes in this PR?
Vendor a self-contained, method-less copy of
text/templateunderinternal/methodlesstemplateand point both call sites at it. No external dependency (go.mod/go.sumunchanged).doc.go,exec.go,funcs.go,option.go,template.go, plusinternal/fmtsort/sort.go. Go's BSDLICENSEis preserved in the vendored directory and every file keeps itsThe Go Authorscopyright header.text/template/parseis reused unchanged (the parser has noMethodByName/evalFieldedge, so it does not defeat DCE).helper.go(ParseFiles/ParseGlob/ParseFS) is dropped — the OPA call sites only needNew/Parse/Execute, and nothing in the kept files references it.MethodByNamebranch inexec.go'sevalField(method resolution on the data value). Everything else is byte-identical, so re-syncing to a newer Go release is a diff-and-reapply of that single branch removal.internal/gojsonschema(commit 1) andv1/topdown(commit 2) import the vendored package. The gojsonschema engine is retained in full, soErrorTemplateFuncs(itsFuncMapextension point) keeps working — no public symbol is removed.Rego values and gojsonschema
ErrorDetailsdecode tomap[string]any/[]any/scalars, which have no methods, so removing method resolution is a provable no-op for these callers.Notes to assist PR review:
doc.go/funcs.go/option.go/template.go/internal/fmtsort/sort.goare byte-identical to the Go 1.25.8 originals. Onlyexec.godiffers, in exactly two hunks: theinternal/fmtsort→ vendored import path, and the removedMethodByNameblock (replaced by a comment explaining the DCE rationale).rendertemplateconformance cases (incl.complexrange/if/vars,simpleint%v,missingkey→<undefined>) pass unchanged.internal/gojsonschemaandv1/asttests pass.TestNoStdlibTextTemplateImportin bothinternal/gojsonschemaandv1/topdownscans every non-test file and asserts none import stdlibtext/template/html/template.go build ./...,go vet ./...OK;go mod tidyis a no-op.internal/gojsonschemaprecedent — the copy is verbatim stdlib, and linting it against OPA's house rules would force divergence from upstream Go (it trips ~31 stdlib-idiom issues) and break the diff-and-reapply re-sync.The Go Authors); it contains no third-party/DataDog code.Further comments:
fix/dce-datadogachieves the identical result with a tiny diff by depending ongithub.com/DataDog/datadog-agent/pkg/template/text(external module owns the re-sync). Both pass the same tests.opabinary additionally linksv1/server, which importshtml/template(a wrapper overtext/template) — a separate, independent edge left as a follow-up. Embedders that don't link the server (the common case) get the full win from this PR.