I really love the current Tilt api because it quite simple, flexible, and has a clear caching story. Tilt emphasizes templates as the main model. So its very easy to grasp.
A context has no requirements. It could be a controller, a template instance, or even a model record. Super flexible, no contract. Well, it just depends on "instance_eval" which all Objects support.
Template instances are responsible for their own cache. If a template is stale, needs to be reloaded or whatever, just drop the reference. 'cache["foo"] ||= Tilt::ERBTemplate.new("foo.erb")' is a viable caching strategy.
Flip-Flopped Context Focused API --------------------------------
This is how we do template compiling caching in Rails. I loathe it. However, its quite fast ;)
We could break alway from generic contexts (just any object) and require them to implement some behavior. Contexts become the main model.
* Contexts have a stricter definition and can't be any object. So you need to be sure you are rendering against some "complaint" context. Tilt needs to provide "Tilt::Template" and "Tilt::Context". (twice as many as one simple class)
* Templates along with their locals need to be hashed into some sort of unique key to be used as a method. Context method pollution is another minor concern.
* Contexts hold a set of cached templates. If difficult and not completely clear how to expire single templates. This usually leads to memory leaks if you are using one off templates and throwing them away. Expiring template requires some sort of Tilt.expire(context, template) api, you can't just forget about it. Template lifespan is now a prominent concern people need to be more aware of.
----
I have issues with the design philosophy of the context focused approached. I mainly complaining about using "CompiledTemplates" as a public API. It doesn't really matter to me how Tilt templates work at the end of the day. If we could somehow implement the module caching approach internally and keep the same simple Tilt API, I'm all for that.
On Tue, Mar 2, 2010 at 7:16 PM, Joshua Peek <j...@37signals.com> wrote: > Template Focused API > --------------------
> I really love the current Tilt api because it quite simple, flexible, > and has a clear caching story. Tilt emphasizes templates as the main > model. So its very easy to grasp.
> A context has no requirements. It could be a controller, a template > instance, or even a model record. Super flexible, no contract. Well, > it just depends on "instance_eval" which all Objects support.
> Template instances are responsible for their own cache. If a template > is stale, needs to be reloaded or whatever, just drop the reference. > 'cache["foo"] ||= Tilt::ERBTemplate.new("foo.erb")' is a viable > caching strategy.
Nice write up. I couldn't have put it better myself.
> * Contexts have a stricter definition and can't be any object. So you > need to be sure you are rendering against some "complaint" context. > Tilt needs to provide "Tilt::Template" and "Tilt::Context". (twice as > many as one simple class)
> * Templates along with their locals need to be hashed into some sort > of unique key to be used as a method. Context method pollution is > another minor concern.
> * Contexts hold a set of cached templates. If difficult and not > completely clear how to expire single templates. This usually leads to > memory leaks if you are using one off templates and throwing them > away. Expiring template requires some sort of Tilt.expire(context, > template) api, you can't just forget about it. Template lifespan is > now a prominent concern people need to be more aware of.
> ----
> I have issues with the design philosophy of the context focused > approached. I mainly complaining about using "CompiledTemplates" as a > public API. It doesn't really matter to me how Tilt templates work at > the end of the day. If we could somehow implement the module caching > approach internally and keep the same simple Tilt API, I'm all for > that.
I think reorganizing Tilt to be context focused would be a mistake -- better to just create another project that went at it with that approach.
I remember having a conversation with Yehuda about this at RubyConf. I didn't fully understand what he was talking about at the time, but now that you've laid this out it makes a lot more sense. He suggested making the compiled templates module an option. There would be some way a module object could be passed to Template::new (or configured globally) to hold the pre-compiled template methods. Then, in Template#render, the provided context object would be extended with the configured CompiledTemplates module and the pre-compiled template method invoked.
Some more or less random thoughts on this:
1.) If you want the performance benefits of pre-compiling, you'd need to do a little extra work in providing the CompiledTemplates module. That shouldn't be an issue for frameworks like Rails or Sinatra where pretty much all template managing code is in once place.
2.) By providing a CompiledTemplates module explicitly, you're acknowledging that your context objects will be extended. I did not like the idea of extending context objects all willy nilly as the default behavior but I feel better about it in this scenario where you have to opt in.
3.) Extending the context object with the CompiledTemplates module on each call to render may be rather expensive. Maybe we check if the context object respond_to? the compiled template method, and if so, assume that the module was already included. This way, Rails can include its CompiledTemplates module in the base View class; Sinatra would include the CompiledTemplates module in Sinatra::Base.
4.) The problem of cleaning up / expiring old template methods is punted to whoever provides the CompiledTemplates module. A simple strategy may be to simply recreate the module every N requests, or once the number of methods exceeds some threshold.
With this approach, it seems like we could keep the template focused interface, but still get the performance benefits of precompiling, although maybe not as seemlessly as with a context focused approach.
On Tue, Mar 2, 2010 at 11:49 PM, Ryan Tomayko <rtoma...@gmail.com> wrote: > I remember having a conversation with Yehuda about this at RubyConf. I > didn't fully understand what he was talking about at the time, but now > that you've laid this out it makes a lot more sense. He suggested > making the compiled templates module an option. There would be some > way a module object could be passed to Template::new (or configured > globally) to hold the pre-compiled template methods. Then, in > Template#render, the provided context object would be extended with > the configured CompiledTemplates module and the pre-compiled template > method invoked.
> Some more or less random thoughts on this:
> 1.) If you want the performance benefits of pre-compiling, you'd need > to do a little extra work in providing the CompiledTemplates module. > That shouldn't be an issue for frameworks like Rails or Sinatra where > pretty much all template managing code is in once place.
> 2.) By providing a CompiledTemplates module explicitly, you're > acknowledging that your context objects will be extended. I did not > like the idea of extending context objects all willy nilly as the > default behavior but I feel better about it in this scenario where you > have to opt in.
> 3.) Extending the context object with the CompiledTemplates module on > each call to render may be rather expensive. Maybe we check if the > context object respond_to? the compiled template method, and if so, > assume that the module was already included. This way, Rails can > include its CompiledTemplates module in the base View class; Sinatra > would include the CompiledTemplates module in Sinatra::Base.
> 4.) The problem of cleaning up / expiring old template methods is > punted to whoever provides the CompiledTemplates module. A simple > strategy may be to simply recreate the module every N requests, or > once the number of methods exceeds some threshold.
Cleanup is the dirtiest part. Memory leaks are a serious concern.
> With this approach, it seems like we could keep the template focused > interface, but still get the performance benefits of precompiling, > although maybe not as seemlessly as with a context focused approach.
This discussion could easily go out of control when more people chime in. Lets make sure we are focusing on real implementations.
Option #1 is interesting. Tilt could (even optionally) include CompiledTemplates into Object making any object a valid context. The cons are "method pollution" on every object. It seems like a valid concern, but I'm more interested if there are any performance hits for shoving this into every object. Finalizers should automatically deal with unused complied methods. The real win is keeping the Tilt api the same. Maybe one day, Ruby will be fast enough not to need this compiled method hack and we can revert to instance eval or whatever.
On Tue, Mar 2, 2010 at 11:11 PM, Joshua Peek <j...@37signals.com> wrote: > On Tue, Mar 2, 2010 at 11:49 PM, Ryan Tomayko <rtoma...@gmail.com> wrote: >> I remember having a conversation with Yehuda about this at RubyConf. I >> didn't fully understand what he was talking about at the time, but now >> that you've laid this out it makes a lot more sense. He suggested >> making the compiled templates module an option. There would be some >> way a module object could be passed to Template::new (or configured >> globally) to hold the pre-compiled template methods. Then, in >> Template#render, the provided context object would be extended with >> the configured CompiledTemplates module and the pre-compiled template >> method invoked.
>> Some more or less random thoughts on this:
>> 1.) If you want the performance benefits of pre-compiling, you'd need >> to do a little extra work in providing the CompiledTemplates module. >> That shouldn't be an issue for frameworks like Rails or Sinatra where >> pretty much all template managing code is in once place.
> Ideally, you don't have to think about this.
>> 2.) By providing a CompiledTemplates module explicitly, you're >> acknowledging that your context objects will be extended. I did not >> like the idea of extending context objects all willy nilly as the >> default behavior but I feel better about it in this scenario where you >> have to opt in.
>> 3.) Extending the context object with the CompiledTemplates module on >> each call to render may be rather expensive. Maybe we check if the >> context object respond_to? the compiled template method, and if so, >> assume that the module was already included. This way, Rails can >> include its CompiledTemplates module in the base View class; Sinatra >> would include the CompiledTemplates module in Sinatra::Base.
>> 4.) The problem of cleaning up / expiring old template methods is >> punted to whoever provides the CompiledTemplates module. A simple >> strategy may be to simply recreate the module every N requests, or >> once the number of methods exceeds some threshold.
> Cleanup is the dirtiest part. Memory leaks are a serious concern.
I don't disagree. But if the code to perform cleanup can be simple outside of tilt while it must be complex inside of tilt, I'd prefer to punt it to the outside and let the calling code handle it. I'm assuming cleanup would require a lot of logic inside of tilt, though. There may be a clean and simple way to accomplish it.
>> With this approach, it seems like we could keep the template focused >> interface, but still get the performance benefits of precompiling, >> although maybe not as seemlessly as with a context focused approach.
> This discussion could easily go out of control when more people chime > in. Lets make sure we are focusing on real implementations.
I was thinking of something slightly different. I'll get a branch together tonight or tomorrow.
> Option #1 is interesting. Tilt could (even optionally) include > CompiledTemplates into Object making any object a valid context. The > cons are "method pollution" on every object. It seems like a valid > concern, but I'm more interested if there are any performance hits for > shoving this into every object.
Having to include these pre-compiled template methods in Object would make me cry. You're trying to send me back to Python, I think ;) There's got to be a better way. Even if there's no performance issues, I'd still hate to do that if only because it would be a mess listing methods on objects in IRB and ruby-debug.
Here's a slight modification to option #1 that extends the context object if the precompiled method doesn't exist:
Yehuda tells me extending objects breaks the method cache so this could be more trouble than it's worth.
I still like the idea of making the calling code own the module. That way it can be responsible for including it in context objects.
> Finalizers should automatically deal with unused complied methods.
Just so I'm clear, the idea with using finalizers would be to automatically remove the method from the module when the Template instance falls out of reference? That seems like a good idea regardless of who owns the module.
> The real win is keeping the Tilt api the > same. Maybe one day, Ruby will be fast enough not to need this > compiled method hack and we can revert to instance eval or whatever.
That reminds me. I'm not entirely sure using a proc + instance_eval/instance_exec won't work here. Using a method is supposed to be a little faster but I don't think it's a huge difference. The reason that approach was reverted was because it broke under MRI 1.9.2 due to the arity changes with lambda or something. I'm not convinced that approach is fundamentally flawed. Before we get ahead of ourselves going the module/method route, we should make sure there's not a cleaner way.
I'm going to create a few branches with different approaches and maybe run some benchmarks.
On Wed, Mar 3, 2010 at 12:14 AM, Ryan Tomayko <rtoma...@gmail.com> wrote: > That reminds me. I'm not entirely sure using a proc + > instance_eval/instance_exec won't work here. Using a method is > supposed to be a little faster but I don't think it's a huge > difference. The reason that approach was reverted was because it broke > under MRI 1.9.2 due to the arity changes with lambda or something.
My bad. It was yield from within a template that broke for some reason under 1.9:
1) Error: test_passing_a_block_for_yield(ERBTemplateTest): LocalJumpError: no block given (yield) (__TEMPLATE__):1:in `block in template_proc' /Users/rtomayko/git/tilt/lib/tilt.rb:235:in `instance_eval' /Users/rtomayko/git/tilt/lib/tilt.rb:235:in `evaluate' /Users/rtomayko/git/tilt/lib/tilt.rb:112:in `render' /Users/rtomayko/git/tilt/test/tilt_erbtemplate_test.rb:34:in `block in <class:ERBTemplateTest>'
It breaks under MRI >= 1.9.1. I'm pretty sure we can figure out a fix.
I'm looking at this in a more practical way: Imagine you're writing the most awesome web framework, and you're writing most of your templates with ERB. Would you choose Tilt which supports tons of template engines or would you just implement ERB support yourself and make template rendering 8 times faster? Unfortunately, I believe many would take the "performance-route" just to have some shiny numbers in their announcement.
And the "Custom template evaluation scopes" part of Tilt is nice, but in reality the context will almost always be some instance of a Controllers::Base-class and we therefore have a very sensible place to define the methods (not Object, please).
That said, it's probably possible to keep support for both approaches without too much work.
As far as I know, proc + instance_eval/instance_exec won't work because if we're caching it we can't capture any yields within it, and currently you can't invoke instance_eval/instance_exec with a block. I'd love to be proven wrong though :-)
On Mar 3, 9:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote:
> On Tue, Mar 2, 2010 at 11:11 PM, Joshua Peek <j...@37signals.com> wrote: > > On Tue, Mar 2, 2010 at 11:49 PM, Ryan Tomayko <rtoma...@gmail.com> wrote: > >> I remember having a conversation with Yehuda about this at RubyConf. I > >> didn't fully understand what he was talking about at the time, but now > >> that you've laid this out it makes a lot more sense. He suggested > >> making the compiled templates module an option. There would be some > >> way a module object could be passed to Template::new (or configured > >> globally) to hold the pre-compiled template methods. Then, in > >> Template#render, the provided context object would be extended with > >> the configured CompiledTemplates module and the pre-compiled template > >> method invoked.
> >> Some more or less random thoughts on this:
> >> 1.) If you want the performance benefits of pre-compiling, you'd need > >> to do a little extra work in providing the CompiledTemplates module. > >> That shouldn't be an issue for frameworks like Rails or Sinatra where > >> pretty much all template managing code is in once place.
> > Ideally, you don't have to think about this.
> >> 2.) By providing a CompiledTemplates module explicitly, you're > >> acknowledging that your context objects will be extended. I did not > >> like the idea of extending context objects all willy nilly as the > >> default behavior but I feel better about it in this scenario where you > >> have to opt in.
> >> 3.) Extending the context object with the CompiledTemplates module on > >> each call to render may be rather expensive. Maybe we check if the > >> context object respond_to? the compiled template method, and if so, > >> assume that the module was already included. This way, Rails can > >> include its CompiledTemplates module in the base View class; Sinatra > >> would include the CompiledTemplates module in Sinatra::Base.
> >> 4.) The problem of cleaning up / expiring old template methods is > >> punted to whoever provides the CompiledTemplates module. A simple > >> strategy may be to simply recreate the module every N requests, or > >> once the number of methods exceeds some threshold.
> > Cleanup is the dirtiest part. Memory leaks are a serious concern.
> I don't disagree. But if the code to perform cleanup can be simple > outside of tilt while it must be complex inside of tilt, I'd prefer to > punt it to the outside and let the calling code handle it. I'm > assuming cleanup would require a lot of logic inside of tilt, though. > There may be a clean and simple way to accomplish it.
> >> With this approach, it seems like we could keep the template focused > >> interface, but still get the performance benefits of precompiling, > >> although maybe not as seemlessly as with a context focused approach.
> > This discussion could easily go out of control when more people chime > > in. Lets make sure we are focusing on real implementations.
> I was thinking of something slightly different. I'll get a branch > together tonight or tomorrow.
> > Option #1 is interesting. Tilt could (even optionally) include > > CompiledTemplates into Object making any object a valid context. The > > cons are "method pollution" on every object. It seems like a valid > > concern, but I'm more interested if there are any performance hits for > > shoving this into every object.
> Having to include these pre-compiled template methods in Object would > make me cry. You're trying to send me back to Python, I think ;) > There's got to be a better way. Even if there's no performance issues, > I'd still hate to do that if only because it would be a mess listing > methods on objects in IRB and ruby-debug.
> Here's a slight modification to option #1 that extends the context > object if the precompiled method doesn't exist:
> Yehuda tells me extending objects breaks the method cache so this > could be more trouble than it's worth.
> I still like the idea of making the calling code own the module. That > way it can be responsible for including it in context objects.
> > Finalizers should automatically deal with unused complied methods.
> Just so I'm clear, the idea with using finalizers would be to > automatically remove the method from the module when the Template > instance falls out of reference? That seems like a good idea > regardless of who owns the module.
> > The real win is keeping the Tilt api the > > same. Maybe one day, Ruby will be fast enough not to need this > > compiled method hack and we can revert to instance eval or whatever.
> That reminds me. I'm not entirely sure using a proc + > instance_eval/instance_exec won't work here. Using a method is > supposed to be a little faster but I don't think it's a huge > difference. The reason that approach was reverted was because it broke > under MRI 1.9.2 due to the arity changes with lambda or something. I'm > not convinced that approach is fundamentally flawed. Before we get > ahead of ourselves going the module/method route, we should make sure > there's not a cleaner way.
> I'm going to create a few branches with different approaches and maybe > run some benchmarks.
Yeah. There's a pretty massive perf gain to be had not needing to parse the source into its nodes each time. Lots of needless object creation is avoided too. I think the question now is how much better methods perform vs. procs.
> I'm looking at this in a more practical way: Imagine you're writing > the most awesome web framework, and you're writing most of your > templates with ERB. Would you choose Tilt which supports tons of > template engines or would you just implement ERB support yourself and > make template rendering 8 times faster? Unfortunately, I believe many > would take the "performance-route" just to have some shiny numbers in > their announcement.
Well, Tilt is just an extraction from Sinatra. We were getting tons of requests to add support for different template languages and didn't want to take that on as a core piece of Sinatra so we split it out. The other big motivator for me was that it's always a pain in the ass trying to get backtraces w/ correct line numbers and sane scope rules with every template engine I've dealt with. I wanted to know that as long as I conform to an interface, the engine will do basically what I want.
Also, great performance is going to be one more reason to choose tilt. It's not there yet but I'm confident we'll get it nailed down and then that's just one more benefit you get simply by following a standard interface.
> And the "Custom template evaluation scopes" part of Tilt is nice, but > in reality the context will almost always be some instance of a > Controllers::Base-class
Another place I use tilt and plan to use it a lot more is generating static files. For instance, most projects I work on usually end up with a bunch of markdown files + an ERB template under a `doc/` directory. It's trivial to throw a few Rake tasks together that generate the content. In those cases, the scope is typically a simple Hash or even just Object. So I'd like tilt to stay useful in non-web framework cases as well. I definitely don't want to assume the existance of a common controller/view object.
> and we therefore have a very sensible place to define the methods (not Object, please).
I do agree that in cases where performance is critical, you're likely going to have a common base class for context objects and that we shouldn't ignore this if we have to go the method route.
> That said, it's probably possible to keep support for both approaches > without too much work.
> As far as I know, proc + instance_eval/instance_exec won't work > because if we're caching it we can't capture any yields within it, and > currently you can't invoke instance_eval/instance_exec with a block. > I'd love to be proven wrong though :-)
I'd love for that to be proven wrong too :) I'm playing with it now and it looks like it might be impossible under 1.9. More soon.
On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote:
> I don't disagree. But if the code to perform cleanup can be simple > outside of tilt while it must be complex inside of tilt, I'd prefer to > punt it to the outside and let the calling code handle it. I'm > assuming cleanup would require a lot of logic inside of tilt, though. > There may be a clean and simple way to accomplish it.
"Object.include Tilt::CompiledTemplate" was just a crazy idea that could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
> Yehuda tells me extending objects breaks the method cache so this > could be more trouble than it's worth.
Right, extend on the fly is no good.
I put together a #3 sample that combines my first two.
We keep the instance_eval path around so we can eval on any object. If you know ahead of time some class is going be reused over and over as a context object, you can opt-in for more speed by including CompiledTemplates. The caching contract stays the same. The template is responsible for cleaning up its compiled method cache when its GC'd.
The cleanup and finalize approach is very cool (if i do say so myself). I hope there is no technically implementation issues with doing that.
On Wed, Mar 3, 2010 at 15:46, Joshua Peek <j...@37signals.com> wrote: > On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote: > > I don't disagree. But if the code to perform cleanup can be simple > > outside of tilt while it must be complex inside of tilt, I'd prefer to > > punt it to the outside and let the calling code handle it. I'm > > assuming cleanup would require a lot of logic inside of tilt, though. > > There may be a clean and simple way to accomplish it.
> "Object.include Tilt::CompiledTemplate" was just a crazy idea that > could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
> > Yehuda tells me extending objects breaks the method cache so this > > could be more trouble than it's worth.
> Right, extend on the fly is no good.
> I put together a #3 sample that combines my first two.
> We keep the instance_eval path around so we can eval on any object. If > you know ahead of time some class is going be reused over and over as > a context object, you can opt-in for more speed by including > CompiledTemplates. The caching contract stays the same. The template > is responsible for cleaning up its compiled method cache when its > GC'd.
> The cleanup and finalize approach is very cool (if i do say so > myself). I hope there is no technically implementation issues with > doing that.
On Wed, Mar 3, 2010 at 6:46 AM, Joshua Peek <j...@37signals.com> wrote: > On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote: >> I don't disagree. But if the code to perform cleanup can be simple >> outside of tilt while it must be complex inside of tilt, I'd prefer to >> punt it to the outside and let the calling code handle it. I'm >> assuming cleanup would require a lot of logic inside of tilt, though. >> There may be a clean and simple way to accomplish it.
> "Object.include Tilt::CompiledTemplate" was just a crazy idea that > could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
>> Yehuda tells me extending objects breaks the method cache so this >> could be more trouble than it's worth.
> Right, extend on the fly is no good.
> I put together a #3 sample that combines my first two.
> We keep the instance_eval path around so we can eval on any object. If > you know ahead of time some class is going be reused over and over as > a context object, you can opt-in for more speed by including > CompiledTemplates. The caching contract stays the same. The template > is responsible for cleaning up its compiled method cache when its > GC'd.
> The cleanup and finalize approach is very cool (if i do say so > myself). I hope there is no technically implementation issues with > doing that.
I have branches up for both the proc and method approaches:
The procs approach falls down under 1.9 due to the yield from proc problem. I wasn't able to figure out a way around it. Otherwise, I love it and it would make life a whole lot easier. It works great on 1.8.7.
I plan on merging the precomp-methods branch after adding finalizers. It looks a lot like #3, allowing the compile site to be configured on a template-by-template basis but also has a one-line hack to enable template compilation on Object:
Tilt.enable_global_compile_site!
The way I plan on using it from Sinatra would be something like:
module Sinatra module CompiledTemplates end
class Base include CompiledTemplates
def render(type, name, options, locals, &block) Tilt.new(type, options, CompiledTemplates) { # load template data } end end end
I assume Rails would be very similar, assuming this meets all your needs.
I'm pretty happy with how this turned out, to be honest. It's a lot more complex than procs but it seems to be the best we can do and forced me to clean up some other crap that's been laying around forever.
Review and feedback appreciated. Huge thanks to everyone who chipped in on this.
On Wed, Mar 3, 2010 at 8:18 AM, Ryan Tomayko <rtoma...@gmail.com> wrote: > On Wed, Mar 3, 2010 at 6:46 AM, Joshua Peek <j...@37signals.com> wrote: >> On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote: >>> I don't disagree. But if the code to perform cleanup can be simple >>> outside of tilt while it must be complex inside of tilt, I'd prefer to >>> punt it to the outside and let the calling code handle it. I'm >>> assuming cleanup would require a lot of logic inside of tilt, though. >>> There may be a clean and simple way to accomplish it.
>> "Object.include Tilt::CompiledTemplate" was just a crazy idea that >> could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
>>> Yehuda tells me extending objects breaks the method cache so this >>> could be more trouble than it's worth.
>> Right, extend on the fly is no good.
>> I put together a #3 sample that combines my first two.
>> We keep the instance_eval path around so we can eval on any object. If >> you know ahead of time some class is going be reused over and over as >> a context object, you can opt-in for more speed by including >> CompiledTemplates. The caching contract stays the same. The template >> is responsible for cleaning up its compiled method cache when its >> GC'd.
>> The cleanup and finalize approach is very cool (if i do say so >> myself). I hope there is no technically implementation issues with >> doing that.
> I have branches up for both the proc and method approaches:
> The procs approach falls down under 1.9 due to the yield from proc > problem. I wasn't able to figure out a way around it. Otherwise, I > love it and it would make life a whole lot easier. It works great on > 1.8.7.
> I plan on merging the precomp-methods branch after adding finalizers. > It looks a lot like #3, allowing the compile site to be configured on > a template-by-template basis but also has a one-line hack to enable > template compilation on Object:
> Tilt.enable_global_compile_site!
> The way I plan on using it from Sinatra would be something like:
> module Sinatra > module CompiledTemplates > end
> class Base > include CompiledTemplates
> def render(type, name, options, locals, &block) > Tilt.new(type, options, CompiledTemplates) { # load template data } > end > end > end
> I assume Rails would be very similar, assuming this meets all your needs.
> I'm pretty happy with how this turned out, to be honest. It's a lot > more complex than procs but it seems to be the best we can do and > forced me to clean up some other crap that's been laying around > forever.
> Review and feedback appreciated. Huge thanks to everyone who chipped in on this.
You know, I do like how the #3 example just uses the mixin to determine if the scope supports compilation. That'll make things a ton cleaner.
On Wed, Mar 3, 2010 at 17:18, Ryan Tomayko <rtoma...@gmail.com> wrote: > On Wed, Mar 3, 2010 at 6:46 AM, Joshua Peek <j...@37signals.com> wrote: > > On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote: > >> I don't disagree. But if the code to perform cleanup can be simple > >> outside of tilt while it must be complex inside of tilt, I'd prefer to > >> punt it to the outside and let the calling code handle it. I'm > >> assuming cleanup would require a lot of logic inside of tilt, though. > >> There may be a clean and simple way to accomplish it.
> > "Object.include Tilt::CompiledTemplate" was just a crazy idea that > > could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
> >> Yehuda tells me extending objects breaks the method cache so this > >> could be more trouble than it's worth.
> > Right, extend on the fly is no good.
> > I put together a #3 sample that combines my first two.
> > We keep the instance_eval path around so we can eval on any object. If > > you know ahead of time some class is going be reused over and over as > > a context object, you can opt-in for more speed by including > > CompiledTemplates. The caching contract stays the same. The template > > is responsible for cleaning up its compiled method cache when its > > GC'd.
> > The cleanup and finalize approach is very cool (if i do say so > > myself). I hope there is no technically implementation issues with > > doing that.
> I have branches up for both the proc and method approaches:
> The procs approach falls down under 1.9 due to the yield from proc > problem. I wasn't able to figure out a way around it. Otherwise, I > love it and it would make life a whole lot easier. It works great on > 1.8.7.
> I plan on merging the precomp-methods branch after adding finalizers. > It looks a lot like #3, allowing the compile site to be configured on > a template-by-template basis but also has a one-line hack to enable > template compilation on Object:
> Tilt.enable_global_compile_site!
> The way I plan on using it from Sinatra would be something like:
> module Sinatra > module CompiledTemplates > end
> class Base > include CompiledTemplates
> def render(type, name, options, locals, &block) > Tilt.new(type, options, CompiledTemplates) { # load template data > } > end > end > end
> I assume Rails would be very similar, assuming this meets all your needs.
> I'm pretty happy with how this turned out, to be honest. It's a lot > more complex than procs but it seems to be the best we can do and > forced me to clean up some other crap that's been laying around > forever.
> Review and feedback appreciated. Huge thanks to everyone who chipped in on > this.
On Wed, Mar 3, 2010 at 8:38 AM, Magnus Holm <judo...@gmail.com> wrote: > After this patch, the tests breaks on 1.8.7 too: > diff --git a/test/tilt_erbtemplate_test.rb b/test/tilt_erbtemplate_test.rb > index b3e1da2..01f47ee 100644 > --- a/test/tilt_erbtemplate_test.rb > +++ b/test/tilt_erbtemplate_test.rb > @@ -39,6 +39,7 @@ class ERBTemplateTest < Test::Unit::TestCase > test "passing a block for yield" do > template = Tilt::ERBTemplate.new { 'Hey <%= yield %>!' } > assert_equal "Hey Joe!", template.render { 'Joe' } > + assert_equal "Hey Moe!", template.render { 'Moe' } > end
> test "backtrace file and line reporting without locals" do > Result: > 1) Failure: > test_passing_a_block_for_yield(ERBTemplateTest) > [./test/tilt_erbtemplate_test.rb:42]: > <"Hey Moe!"> expected but was > <"Hey Joe!">. > // Magnus Holm
Which branch is that off of? The precomp-methods branch handles it fine. The precomp-procs branch may use the first block passed forever. Actually, yeah, I'm sure that's it.
Also, I've updated the precomp-methods branch to look a bit more like josh's example #3. There's a simple Tilt::CompileSite mixin. Anything that includes it gets compiled methods when used as a scope.
> On Wed, Mar 3, 2010 at 17:18, Ryan Tomayko <rtoma...@gmail.com> wrote:
>> On Wed, Mar 3, 2010 at 6:46 AM, Joshua Peek <j...@37signals.com> wrote: >> > On Mar 3, 2:14 am, Ryan Tomayko <rtoma...@gmail.com> wrote: >> >> I don't disagree. But if the code to perform cleanup can be simple >> >> outside of tilt while it must be complex inside of tilt, I'd prefer to >> >> punt it to the outside and let the calling code handle it. I'm >> >> assuming cleanup would require a lot of logic inside of tilt, though. >> >> There may be a clean and simple way to accomplish it.
>> > "Object.include Tilt::CompiledTemplate" was just a crazy idea that >> > could work. 'AnyObject.methods.grep(/tilt/) #=> lots' is nasty
>> >> Yehuda tells me extending objects breaks the method cache so this >> >> could be more trouble than it's worth.
>> > Right, extend on the fly is no good.
>> > I put together a #3 sample that combines my first two.
>> > We keep the instance_eval path around so we can eval on any object. If >> > you know ahead of time some class is going be reused over and over as >> > a context object, you can opt-in for more speed by including >> > CompiledTemplates. The caching contract stays the same. The template >> > is responsible for cleaning up its compiled method cache when its >> > GC'd.
>> > The cleanup and finalize approach is very cool (if i do say so >> > myself). I hope there is no technically implementation issues with >> > doing that.
>> I have branches up for both the proc and method approaches:
>> The procs approach falls down under 1.9 due to the yield from proc >> problem. I wasn't able to figure out a way around it. Otherwise, I >> love it and it would make life a whole lot easier. It works great on >> 1.8.7.
>> I plan on merging the precomp-methods branch after adding finalizers. >> It looks a lot like #3, allowing the compile site to be configured on >> a template-by-template basis but also has a one-line hack to enable >> template compilation on Object:
>> Tilt.enable_global_compile_site!
>> The way I plan on using it from Sinatra would be something like:
>> module Sinatra >> module CompiledTemplates >> end
>> class Base >> include CompiledTemplates
>> def render(type, name, options, locals, &block) >> Tilt.new(type, options, CompiledTemplates) { # load template data >> } >> end >> end >> end
>> I assume Rails would be very similar, assuming this meets all your needs.
>> I'm pretty happy with how this turned out, to be honest. It's a lot >> more complex than procs but it seems to be the best we can do and >> forced me to clean up some other crap that's been laying around >> forever.
>> Review and feedback appreciated. Huge thanks to everyone who chipped in on >> this.
On Wed, Mar 3, 2010 at 10:18 AM, Ryan Tomayko <rtoma...@gmail.com> wrote: > I plan on merging the precomp-methods branch after adding finalizers. > It looks a lot like #3, allowing the compile site to be configured on > a template-by-template basis but also has a one-line hack to enable > template compilation on Object:
> Tilt.enable_global_compile_site!
> The way I plan on using it from Sinatra would be something like:
> module Sinatra > module CompiledTemplates > end
> class Base > include CompiledTemplates
> def render(type, name, options, locals, &block) > Tilt.new(type, options, CompiledTemplates) { # load template data } > end > end > end
> I assume Rails would be very similar, assuming this meets all your needs.
I don't think we need multiple compile sites. One "Tilt::CompileSite" should work fine. It shouldn't be harmful if some of Rails' compiled templates get in Sinatra's module. Its no huge deal, just think compile site should be as internal as possible.
+ "__tilt_#{object_id}_#{locals.keys.hash}"
Probably need a better hashing algorithm for locals.
On Wed, Mar 3, 2010 at 9:47 AM, Joshua Peek <j...@37signals.com> wrote: > On Wed, Mar 3, 2010 at 10:18 AM, Ryan Tomayko <rtoma...@gmail.com> wrote: >> I plan on merging the precomp-methods branch after adding finalizers. >> It looks a lot like #3, allowing the compile site to be configured on >> a template-by-template basis but also has a one-line hack to enable >> template compilation on Object:
>> Tilt.enable_global_compile_site!
>> The way I plan on using it from Sinatra would be something like:
>> module Sinatra >> module CompiledTemplates >> end
>> class Base >> include CompiledTemplates
>> def render(type, name, options, locals, &block) >> Tilt.new(type, options, CompiledTemplates) { # load template data } >> end >> end >> end
>> I assume Rails would be very similar, assuming this meets all your needs.
> I don't think we need multiple compile sites. One "Tilt::CompileSite" > should work fine. It shouldn't be harmful if some of Rails' compiled > templates get in Sinatra's module. Its no huge deal, just think > compile site should be as internal as possible.
> + "__tilt_#{object_id}_#{locals.keys.hash}"
> Probably need a better hashing algorithm for locals.
We only need uniqueness over key names, not values, so that should be pretty close. If not, we can crc32 the concatenated keys or something and that should give us a bit more insurance against collisions.
Somewhat related, I ran into all kinds of weirdness with finalizers and object_id uniqueness. It appears object_ids can be reused long before the previous object's finalizers are run, causing all kinds of strangeness. Or something. I had to add a timestamp to the method name to avoid collisions.
1) Haml generates code too, so it should definitely be possible to use method generation here too. See engine.rb#def_method and precompiled.rb#precompiled_with_ambles in nex3/haml.
2) Why do we use @_out_buf instead of a local variable? Shouldn't the user be able to specify the buffer name instead? Instead of all the funky instance_variable_get-stuff, we could simply wrap the source in:
begin __tilt_prev_buf = #{bufname} #{source} ensure #{bufname} = __tilt_prev_buf end
And you can specify the buffer name with something like Tilt.new(:buffer_name => "$something")? Then it would work with both lvars, ivars and gvars (even though we might only want to enable it for ivars and gvars).
3) Are you sure the finalizer works properly and doesn't capture the instance? I always mess those up...
On Wed, Mar 3, 2010 at 19:35, Ryan Tomayko <rtoma...@gmail.com> wrote: > On Wed, Mar 3, 2010 at 9:47 AM, Joshua Peek <j...@37signals.com> wrote: > > On Wed, Mar 3, 2010 at 10:18 AM, Ryan Tomayko <rtoma...@gmail.com> > wrote: > >> I plan on merging the precomp-methods branch after adding finalizers. > >> It looks a lot like #3, allowing the compile site to be configured on > >> a template-by-template basis but also has a one-line hack to enable > >> template compilation on Object:
> >> Tilt.enable_global_compile_site!
> >> The way I plan on using it from Sinatra would be something like:
> >> def render(type, name, options, locals, &block) > >> Tilt.new(type, options, CompiledTemplates) { # load template > data } > >> end > >> end > >> end
> >> I assume Rails would be very similar, assuming this meets all your > needs.
> > I don't think we need multiple compile sites. One "Tilt::CompileSite" > > should work fine. It shouldn't be harmful if some of Rails' compiled > > templates get in Sinatra's module. Its no huge deal, just think > > compile site should be as internal as possible.
> > + "__tilt_#{object_id}_#{locals.keys.hash}"
> > Probably need a better hashing algorithm for locals.
> We only need uniqueness over key names, not values, so that should be > pretty close. If not, we can crc32 the concatenated keys or something > and that should give us a bit more insurance against collisions.
> Somewhat related, I ran into all kinds of weirdness with finalizers > and object_id uniqueness. It appears object_ids can be reused long > before the previous object's finalizers are run, causing all kinds of > strangeness. Or something. I had to add a timestamp to the method name > to avoid collisions.
On Wed, Mar 3, 2010 at 3:13 PM, Magnus Holm <judo...@gmail.com> wrote: > Yeah, I was talking about the proc-branch. > The method-branch looks great. A few comments: > 1) Haml generates code too, so it should definitely be possible to use > method generation here too. See engine.rb#def_method and > precompiled.rb#precompiled_with_ambles in nex3/haml.
Nice.
> 2) Why do we use @_out_buf instead of a local variable? Shouldn't the user > be able to specify the buffer name instead? Instead of all the funky > instance_variable_get-stuff, we could simply wrap the source in: > begin > __tilt_prev_buf = #{bufname} > #{source} > ensure > #{bufname} = __tilt_prev_buf > end > And you can specify the buffer name with something like > Tilt.new(:buffer_name => "$something")? Then it would work with both lvars, > ivars and gvars (even though we might only want to enable it for ivars and > gvars).
Yeah. Good catch. This might be a regression Sinatra 1.0.a now that you mention it.
> 3) Are you sure the finalizer works properly and doesn't capture the > instance? I always mess those up...
I'll see about adding a test that verifies the finalizers aren't holding onto to anything. I verified they were running with puts debugging but it'd be good to having some a bit more robust in there to catch any issues in the future.
