Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create some lightweight forks of text/template and html/template #6594

Closed
bep opened this issue Dec 9, 2019 · 4 comments
Closed

Create some lightweight forks of text/template and html/template #6594

bep opened this issue Dec 9, 2019 · 4 comments
Assignees
Milestone

Comments

@bep
Copy link
Member

bep commented Dec 9, 2019

Note that any changes will only touch the template execution, but I suspect that it's interconnected.

While working on the thing I'm working on, I have created a new design for template changes (a dependency graph). All of this was fine and dandy until I came to the .Render method. I could probably have pushed that problem to another day, but I have spent a stupid amount of time today bending a working solution together, only to see my benchmarks slow down 20+ %.

I have thought about this before, but imagined it would be too hard to do, maintain etc. But I suspect that if I script this and patch lightly, the future time and gray hair saved should be massive.

Here are some motivation points as to "why":

  • The template funcs are set on the templates, which means that Hugo clones the entire template set to all languages.
  • There are no (easy) way to pass an execution context to methods/functions without changing the method signatures.
  • There is no (easy) way to inject hooks to control how map lookup/methods gets invoked. One example is the special maps.Params in Hugo, which has methods for lower case lookups.
  • The method/function invocation is entirely reflection based, which is still pretty fast, but with proper interfaces, we can do better with code generation.

Note that I have been fairly active in wishing stuff on Go's issue tracker, but while I feel they listen, that project, and especially these packages, is moving slower than I would like.

What do you think, @moorereason ?

@bep bep added the Proposal label Dec 9, 2019
@bep bep added this to the v0.61 milestone Dec 9, 2019
@bep bep self-assigned this Dec 9, 2019
@moorereason
Copy link
Contributor

I share your concern with maintaining a fork and compatibility, but if you can come up with some lightweight interfaces that are easy to maintain, it could make many things easier.

Also, if we could come up with a really tight, generally useful change to the stdlib API, it may have a shot of getting pushed upstream. Long shot, but we can aim for it.

@bep
Copy link
Member Author

bep commented Dec 10, 2019

This would also, short term, probably mean deprecation/removal of Ace/Amber, which I don't use. I suspect some people will scream at me for that, but those people are not paying me anything to spend the countless hours needed to maintain/develop it.

So, this is picking what I think it's the least amount of friction. I have spent countless hours trying to work around the limitation of those packages, and I swill cannot do what I want.

bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Fixes gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 10, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
@bep bep modified the milestones: v0.61, v0.62 Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
@bep bep added Enhancement and removed Proposal labels Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 11, 2019
bep added a commit to bep/hugo that referenced this issue Dec 12, 2019
This is a big commit, but it deletes lots of code and simplifies a lot.

* Resolving the template funcs at execution time means we don't have to create template clones per site
* Having a custom map resolver means that we can remove the AST lower case transformation for the special lower case Params map

Not only is the above easier to reason about, it's also faster, especially if you have more than one language, as in the benchmark below:

```
name                          old time/op    new time/op    delta
SiteNew/Deep_content_tree-16    53.7ms ± 0%    48.1ms ± 2%  -10.38%  (p=0.029 n=4+4)

name                          old alloc/op   new alloc/op   delta
SiteNew/Deep_content_tree-16    41.0MB ± 0%    36.8MB ± 0%  -10.26%  (p=0.029 n=4+4)

name                          old allocs/op  new allocs/op  delta
SiteNew/Deep_content_tree-16      481k ± 0%      410k ± 0%  -14.66%  (p=0.029 n=4+4)
```

This should be even better if you also have lots of templates.

Closes gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 12, 2019
This commit also removes support for Ace and Amber templates.

Updates gohugoio#6594
bep added a commit to bep/hugo that referenced this issue Dec 12, 2019
This is a big commit, but it deletes lots of code and simplifies a lot.

* Resolving the template funcs at execution time means we don't have to create template clones per site
* Having a custom map resolver means that we can remove the AST lower case transformation for the special lower case Params map

Not only is the above easier to reason about, it's also faster, especially if you have more than one language, as in the benchmark below:

```
name                          old time/op    new time/op    delta
SiteNew/Deep_content_tree-16    53.7ms ± 0%    48.1ms ± 2%  -10.38%  (p=0.029 n=4+4)

name                          old alloc/op   new alloc/op   delta
SiteNew/Deep_content_tree-16    41.0MB ± 0%    36.8MB ± 0%  -10.26%  (p=0.029 n=4+4)

name                          old allocs/op  new allocs/op  delta
SiteNew/Deep_content_tree-16      481k ± 0%      410k ± 0%  -14.66%  (p=0.029 n=4+4)
```

This should be even better if you also have lots of templates.

Closes gohugoio#6594
@bep bep closed this as completed in a03c631 Dec 12, 2019
@bep
Copy link
Member Author

bep commented Dec 20, 2019

@moorereason just a quick note: I will write up a proposal early next year for Go. The thing we have done here is surprisingly unintrusive, and it's basically a stricter separation of parsing (the AST) and template execution, which I think was a mistake in the original design -- and should be a possible sell for a minor Go redesign in this area. There are many issues on Go's issue tracker that would be solved by this.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants