Skip to content
Browse files

revert erb cached proc templates; breaks yield on 1.9

  • Loading branch information...
1 parent 15c7d3f commit 0675f8035e7231426d599c3f0e3ac0d4c6cce0cb @rtomayko committed Dec 13, 2009
Showing with 4 additions and 13 deletions.
  1. +4 −13 lib/tilt.rb
View
17 lib/tilt.rb
@@ -209,11 +209,6 @@ def template_source
# ERB template implementation. See:
# http://www.ruby-doc.org/stdlib/libdoc/erb/rdoc/classes/ERB.html
class ERBTemplate < Template
- def initialize(*args)
- super
- @template_procs = {}
- end
-
def initialize_engine
require_template_library 'erb' unless defined? ::ERB
end
@@ -227,11 +222,14 @@ def template_source
end
def evaluate(scope, locals, &block)
+ source, offset = local_assignment_code(locals)
+ source = [source, template_source].join("\n")
+
original_out_buf =
scope.instance_variables.any? { |var| var.to_sym == :@_out_buf } &&
scope.instance_variable_get(:@_out_buf)
- scope.instance_eval(&template_proc(locals))
+ scope.instance_eval source, eval_file, line - offset
output = scope.instance_variable_get(:@_out_buf)
scope.instance_variable_set(:@_out_buf, original_out_buf)
@@ -240,13 +238,6 @@ def evaluate(scope, locals, &block)
end
private
- def template_proc(locals)
- @template_procs[locals] ||= begin
- source, offset = local_assignment_code(locals)
- source = [source, template_source].join("\n")
- instance_eval("proc { #{source} }", eval_file, line - offset)
- end
- end
# ERB generates a line to specify the character coding of the generated
# source in 1.9. Account for this in the line offset.

7 comments on commit 0675f80

@josh
josh commented on 0675f80 Dec 30, 2009

Do you have a failing test for this?

On 1.9, it may require us to use instance_exec instead of instance_eval.

Definitely want to see this optimization get back in.

@judofyr
Collaborator
judofyr commented on 0675f80 Mar 2, 2010

Wouldn't this anyway use a lot of memory? Every time you pass a different locals-hash (:foo => 1 vs :foo => 2) it builds a new Proc?

@rtomayko
Owner

@josh: Sorry, missed your comment from 2009 somehow. I think using instance_exec or maybe lambda instead proc would fix us. I haven't had a chance to look into it.

@judofyr: Heavy variance in locals will definitely eat a good chunk of memory quickly with this kind of setup. Anyone know off hand how Rails manages this today? If I remember correctly, it caches templates as methods on a module and does it in a way that allows for locals to be passed in.

@judofyr
Collaborator
judofyr commented on 0675f80 Mar 2, 2010

Everything referenced in the locals will also never be garbage collected, well, since it's referenced.

Rails generates a method named based on the template name and the locals used (http://github.com/rails/rails/blob/master/actionpack/lib/action_view/template.rb#L117) and then generates the method in ActionView::CompiledTemplates. This module is then included where they need to render templates.

If Tilt want to do this properly, we'll have to do something like this:

module CompiledTemplates
  extend Tilt::Compilable # Only exposes the method .tilt_compile
end

class Base
  include CompiledTemplates

  def render(file, locals = {}, &blk)
    # Defines a method under the CompiledTemplates-module.
    # Takes the same parameters as Tilt.new, but we'll also need to pass in the locals hash. 
    meth_name = CompiledTemplates.tilt_compile(file, locals)
    # Since it's included, we can easily call it:
    send(meth_name, locals, &blk)
  end
end

Base.new.render("foo.erb")
@rtomayko
Owner

Interesting. So, that would create a new method for each distinct invocation but wouldn't have the reference problems. Works for me. Josh, does this sound like an okay solution to you?

@josh
josh commented on 0675f80 Mar 3, 2010

The nasty thing is that contexts need to be "tilt compatible". You can't eval onto any object. I'd be willing to make some trade offs here.

Can we turn this into a mailing list post? It sort of implies a complete API overhaul.

Please sign in to comment.
Something went wrong with that request. Please try again.