Speed up tpl

[greed42]

What this PR does / why we need it:

This dramatically improves the performance of the tpl function, resolving the issues described in #8002. Our typical charts go from 2-3 minute renders to 1-2 seconds. A particularly pathological one went from 37 minutes to 23 seconds.

It uses a clone of the invoking Go Template instance, rather that re-building everything from scratch, to isolate the effects of parsing the tpl text from the calling context. To allow includes of defines from the tpl text to continue to work, it also re-defines include within the clone so it can close over the clone itself.

Special notes for your reviewer:

The only user-noticeable change should be to performance: actual chart output is unaffected by this change. All tests are unaffected.

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[yxxhero]

@greed42 good job.

[StephanDollberg]

@yxxhero can this be triaged/merged/looked at?

Seems like a massive benefit for a little change.

[yxxhero]

@bacongobbler @hickeyma @joejulian @scottrigby could you have free time to review this feature? this is a good change. Thanks very much.

[joejulian]

How does this differ from #8371?

[yxxhero]

@joejulian main logic is the same. I think we should discuss the PR in the meeting. WDYT?

[joejulian]

Sounds good

[yxxhero]

@joejulian my English is not good. So looking forward to your discussion. Thanks very much.

[joejulian]

Actually, no meetings the rest of October.

If one isn't any better than the other, we should just add the older one to the next minor release milestone and close the newer.

[yxxhero]

@joejulian I'm not familiar with this core logic of tpl. So I can't pick out which is better.

[joejulian]

@greed42 @diafour any comment about the difference of these two PRs?

[greed42]

#8371 permutes the existing code less, so is trivially better on that measure. It's also better for DRY-ness.

Mine also has a bug with define inside tpl that then calls tpl and includes that define, so #8371 is also better there.

[greed42]

Though #8371 has an infinite recursion bug with a case that involves .Files.Get which my approach (just not re-using the filename:string render interface) doesn't suffer. I haven't gotten it isolated yet.

[greed42]

.Files.Get is a possible red-herring for #8371; it's an empty string to tpl that's the problem, basically:

{{ tpl "" . }}

Except that PR looks to do .New(name).Parse(tpl) which I would expect to not suffer from that problem... except the sign you haven't done .New(name) is that:

 {{ tpl `{{ "" }` . }}

also recurses and it doesn't.

[greed42]

OK I've worked out that the difference between t.Execute() and t.ExecuteTemplate() is critical to how empty behaves. The latter will let "earlier" definitions leak through, as t.Parse() will always grab the previous value for a given name for the named association... while the "plain" template t.Execute() uses will see the empty text.

I think that means it is impractical to share Execute code between tpl and regular manifest expansion. tpl needs to be a special case, like include is a special case.

I will see if I can get some tests for the various edge cases I've observed with my chart library regression suite, and set them up as a separate PR so that this PR does not have additions for tests that cover existing behaviour.

associate: https://cs.opensource.google/go/go/+/refs/tags/go1.17.13:src/text/template/template.go;l=227
grab the previous if empty: https://cs.opensource.google/go/go/+/refs/tags/go1.17.13:src/text/template/template.go;l=138

[joejulian]

Based on that analysis, I'll add #8371 to the v3.11.0 milestone and hope that the core maintainers have time to get it reviewed and merged.

[greed42]

Based on that analysis, I'll add #8371 to the v3.11.0 milestone and hope that the core maintainers have time to get it reviewed and merged.

I've written #11456 to add tests for existing tpl behaviour. They show an expected change from #8371, but also show an infinite recursion problem with an empty string which I don't think can be solved while still using (t) ExecuteTemplate(...). Only the "implied" template, the one that's run with (t) Execute(...), can be cleared out with (t) New(name) and remain empty with (t) Parse("").

This PR will also need some more commits if #11456 is merged.

[joejulian]

I believe you're saying that #11456 is the better solution now? I'm happy to switch which PR's in the milestone. I can't duplicate the issue so I have to count on you folks that see the problem to guide me here.

[greed42]

I believe you're saying that #11456 is the better solution now? I'm happy to switch which PR's in the milestone. I can't duplicate the issue so I have to count on you folks that see the problem to guide me here.

No; #11456 is just tests for existing behaviour. It doesn't fix anything. #11351 (this one) is the fix I would now recommend.

The fault in #8371 is with this manifest or anything containing:

---
kind: Test
data: {{ tpl "" . }}

Or other define-only things like:

---
kind: Test
data: {{ tpl `{{define "foo"}}test{{end}}`  . }}

You get a stack overflow, as the empty template never becomes the template ExecuteTemplate sees, and so:

% bin/helm template test
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc0eb46a380 stack=[0xc0eb46a000, 0xc10b46a000]
fatal error: stack overflow
...

[mattfarina]

I like the speedup. We'll need to take a very careful look at this change as there is potential to break existing charts.

[greed42]

It works for me. I don't have anything that takes more than 2 seconds, anyway, so I can't see the improvement, but there's enough feedback to give me confidence that it works for others.

Thanks. The new tests will fail after merging in main, so I've pushed a merge and the appropriate test changes to this branch.

[stavros-k]

Here is some numbers from someone what has something big :)

/tmp/helm/bin/helm version   
version.BuildInfo{Version:"v3.12+unreleased", GitCommit:"b261a1b1bee93343cf9fe92335d3f1ccf3e24558", GitTreeState:"clean", GoVersion:"go1.20.6"}

time /tmp/helm/bin/helm template . > /dev/null
/tmp/helm/bin/helm template . > /dev/null  0,51s user 0,04s system 159% cpu 0,344 total

helm version
version.BuildInfo{Version:"v3.12.2", GitCommit:"1e210a2c8cc5117d1055bfaa5d40f51bbc2e345e", GitTreeState:"clean", GoVersion:"go1.20.5"}

time helm template . > /dev/null 
helm template . > /dev/null  22,30s user 0,45s system 147% cpu 15,452 total

Considering that a test suite of tests can include up to 800 apps. It will same A TON of time!
(https://github.com/truecharts/charts)

Unit tests did not have any speed increase, but should be an indicator that it does not break something :)

Helm built from this PR:

Charts:              1 passed, 1 total
Test Suites:   133 passed, 133 total
Tests:            805 passed, 805 total
Snapshot:         0 passed, 0 total
Time:        8m26.098330718s

Helm v3.12.2

Charts:             1 passed, 1 total
Test Suites:  133 passed, 133 total
Tests:           805 passed, 805 total
Snapshot:        0 passed, 0 total
Time:        8m39.303807066s

[yxxhero]

speed up!!!!

[yxxhero]

@joejulian would you mind to talk the PR in the dev meeting?

[starwarsfan]

What's the current state here? Will this be fixed with 3.13?

[starwarsfan]

As 3.13.0 is released, the fix did not make it into it, right?

[Kuzmenko-Pavel]

We look forward to speeding up!!!

[HrechykhinVlad]

Please speed up, we need those changes. Thank you!

[Lokker29]

We really need this improvement. Please, merge it!

[Mixxxxerr]

Need this asap. Merge pls

[DaniilOmelianenko]

Hey! This really cool! Speed up it, please.

[hrekov]

Looks cool! Speed it up, please.

[FlamingFlowair]

No news on this, is it still under review ? we're almost 3 months after the first approval.
Hope this gets merged soon

[Ornias1993]

Nicely structured and documented.
Speed improvement is crucial for bigger repositories and charts.

LGTM.

[Ornias1993]

@scottrigby and @mattfarina Can we get a final review from you two as well?

It seems most issues including previous upstream blockers have been dealth with and this PR would give a significant boost to making Helm more usefull for projects with more complex requirements relying on heavy tpl usage.

[swythan]

Any update on getting this merged?

[sabre1041]

LGTM

[yxxhero]

Cool.