robfig / soy

Go implementation for Soy templates (Google Closure templates)
MIT License
172 stars 41 forks source link

Reimplement ObligatoryPrintDirectives from #52,#53 with less performa… #54

Closed bhainesva closed 6 years ago

bhainesva commented 6 years ago

…nce impact

Results from running the tests from @meirf.

Before my initial change:

Requests: 60, Rate: 3, Success: 1.00
Overall: 165.448656, 136.608661, 214.184631
Success: 165.448656, 136.608661, 214.184631
Failure: -9223372036854.777344, 0.000000, 0.000000
Errors: map[]
Duration: 19.666666565s, Wait: 134.146821ms

After my first change which caused problems:

Requests: 60, Rate: 3, Success: 0.57
Overall: 17300.647065, 20130.723387, 30005.494537
Success: 7586.369193, 2750.153252, 26260.900133
Failure: 30003.933512, 30003.723974, 30006.367406
Errors: map[Get net/http: timeout awaiting response headers:26]
Duration: 19.66835212s, Wait: 30.006367406s

This updated version:

Requests: 60, Rate: 3, Success: 1.00
Overall: 136.550875, 132.565442, 162.489595
Success: 136.550875, 132.565442, 162.489595
Failure: -9223372036854.777344, 0.000000, 0.000000
Errors: map[]
Duration: 19.666666585s, Wait: 123.45296ms
robfig commented 6 years ago

I see. Because this was done at execution time instead of compile time it was adding print directive each time the template was executed. Can the adding of a print directive be done at compile time instead, activated via an option? I'm a little dubious that this mechanism needs to be more general than a "print empty string instead of null" option, so maybe it can be more specific too. There are few print directives that would make sense to apply to EVERY node. I think this is pre-mature generalization

mhupman commented 6 years ago

@robfig I'm wondering how we would make that work with SoyJS. Directives seemed like a good path to take since they are already plugged in to all the right places of soyhtml and soyjs.

mhupman commented 6 years ago

@robfig bump!

robfig commented 6 years ago

Whoops, sorry for missing the reply.

I'm suggesting that we would use print directives, but we would apply them while compiling instead of at runtime by checking a global variable.

Looking at the code, bundle.go:147 applies a few rounds of post-compilation modifications ("parsepasses"). This could easily fit into that model, so e.g.

  1. Add a method: func (*Bundle) AddParsePass(func(template.Registry))
  2. Run any parse passes provided in Compile
  3. Write this as a parse pass in your own codebase and pass it in, which I think should be doable in about 12 lines of code. See the existing ones e.g. parsepasses/msgids.go for how to visit every Soy node looking only for one particular type (in our case, *ast.PrintNode).

How about that?