Change ActionView ERB Handler from Erubis to Erubi #27757

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@jeremyevans
Contributor

Erubi offers the following advantages for Rails:

  • Works with ruby's --enable-frozen-string-literal option
  • Has 88% smaller memory footprint
  • Does no freedom patching (Erubis adds a method to Kernel)
  • Has simpler internals (1 file, <150 lines of code)
  • Has an open development model (Erubis doesn't have a
    public source control repository or bug tracker)
  • Is not dead (Erubis hasn't been updated since 2011)

Erubi is a simplified fork of Erubis that contains just the
parts that are generally needed (which includes the parts
that Rails uses). The only intentional difference in
behavior is that it does not include support for <%=== tags
for debug output. That could be added to the ActionView ERB
handler if it is desired.

@maclover7
Member

Just curious, are there any benchmarks available comparing Erubis and Erubi?

@jeremyevans
Contributor

In terms of runtime performance, Erubi and Erubis are about the same. Erubi uses the same algorithms and approach as Erubis, it just doesn't split the implementation into 10+ files and include a lot of features that nobody uses. Erubi is significantly faster to load as it is only 1 file.

@kaspth

Mostly stylistic comments. Looks great! ๐Ÿ‘

Is there any way Erubi could take on the @newline_pending code and friends we added in #9555? Looks like it could emit fewer method calls out the gate ๐Ÿ˜Š

actionview/actionview.gemspec
@@ -22,7 +22,7 @@ Gem::Specification.new do |s|
s.add_dependency "activesupport", version
s.add_dependency "builder", "~> 3.1"
- s.add_dependency "erubis", "~> 2.7.0"
+ s.add_dependency "erubi", "~> 1.4"
@kaspth
kaspth Jan 22, 2017 Member

Let's nudge this one more space so it keeps being aligned with the above line.

@newline_pending = 0
- src << "@output_buffer = output_buffer || ActionView::OutputBuffer.new;"
+
+ properties = Hash[properties]
@kaspth
kaspth Jan 22, 2017 Member

Why do we need to do Hash[] on properties?

@jeremyevans
jeremyevans Jan 22, 2017 Contributor

Because otherwise you mutate an input hash that is passed in. Mutation is faster, but it's generally considered a bad idea to mutate something you don't own, and it could lead to undesired behavior if the constructor is called directly.

@kaspth
kaspth Jan 23, 2017 Member

Wasn't aware that Hash[] was intended to mean .dup. All good ๐Ÿ˜Š

+ properties[:preamble] = "@output_buffer = output_buffer || ActionView::OutputBuffer.new;"
+ properties[:postamble] = "@output_buffer.to_s"
+ properties[:bufvar] = "@output_buffer"
+ properties[:escapefunc] = ""
@kaspth
kaspth Jan 22, 2017 Member

Let's align the assignments except the Hash[] line.

end
- def add_text(src, text)
+ def evaluate(_context)
@kaspth
kaspth Jan 22, 2017 Member

Why are we prefixing this variable?

@jeremyevans
jeremyevans Jan 22, 2017 Contributor

That's what the argument name is called in Erubis. I can unprefix it.

@matthewd
matthewd Jan 23, 2017 Member

The prefix is to avoid a conflict with a context local in the template. We should keep it.

@nateberkopec
nateberkopec Jan 23, 2017 Contributor

Is it Rails' usual style to prefix shadowed variables with _? In other codebases, prefixing args with _ indicates the argument is unused.

@jeremyevans
jeremyevans Jan 23, 2017 Contributor

Maybe we should pick a variable name less likely to be used? Something like action_view_erb_handler_context?

@matthewd
matthewd Jan 23, 2017 Member

Shadowing generally shows up in method names rather than local names, because the latter are only relevant when eval/binding gets involved. For methods, yes, we do use an underscore. I don't recall whether we have other places that care about locals.

+ end
+
+ private
+
@kaspth
kaspth Jan 22, 2017 Member

โœ‚๏ธ this line

@jeremyevans
jeremyevans Jan 22, 2017 Contributor

I'm not sure what is being requested. Do you just want to separate line 19 into two lines, or should one of the 19-23 lines be deleted, and if so which one?

@kaspth
kaspth Jan 23, 2017 Member

I mean delete line 23 :)

return if text.empty?
if text == "\n"
@newline_pending += 1
else
src << "@output_buffer.safe_append='"
src << "\n" * @newline_pending if @newline_pending > 0
- src << escape_text(text)
+ src << text.gsub(/['\\]/, '\\\\\&')
@kaspth
kaspth Jan 22, 2017 Member

I think the old version was clearer. Let's extract this to an escape_text of our own.

I'm curious why Erubi doesn't have an escape_text method though? I see the same gsub call in its add_text method.

@jeremyevans
jeremyevans Jan 22, 2017 Contributor

Because having a method that is only caused in one place is slower, and it seems unlikely someone would like to override the behavior. I'll go ahead and add an escape_text method in the Rails ERB handler.

@matthewd
matthewd Jan 23, 2017 Member

I think this is fine here. This is where the surrounding ' appear, so separating that from the compensatory escaping seems less than ideal.

@kaspth
kaspth Jan 23, 2017 edited Member

@jeremyevans thanks for the explanation ๐Ÿ‘

Sounds good to me, @matthewd. Let's keep it like it is ๐Ÿ‘

@@ -73,6 +70,9 @@ def flush_newline_if_pending(src)
end
end
+ # For backwards compatibility
+ Erubis = Erubi
@kaspth
kaspth Jan 22, 2017 Member

Looks like both add_preamble and add_text were public. But I don't if we need to deprecate those two or other methods ::Erubis::Eruby might have had. cc @rafaelfranca

@jeremyevans
jeremyevans Jan 23, 2017 Contributor

Here are the public instance methods that Erubis::Eruby has that Erubi::Engine lacks (omitting the ones added by Rails ERB Handler):

  • add_expr
  • add_expr_debug
  • add_expr_escaped
  • add_expr_literal
  • add_preamble
  • add_stmt
  • add_text
  • convert
  • convert!
  • convert_input
  • def_method
  • escape
  • escape=
  • escaped_expr
  • escapefunc
  • escapefunc=
  • filename=
  • init_converter
  • init_evaluator
  • init_generator
  • pattern
  • pattern=
  • postamble
  • postamble=
  • preamble
  • preamble=
  • process
  • process_proc
  • result
  • src=
  • trim
  • trim=

Most of these have no equivalent in Erubi, and are not used. Certainly the setter methods couldn't have an equivalent, as Erubi::Engine.new returns a frozen object.

If you really wanted to deprecate, you'd probably want to ship with both Erubis and Erubi, and deprecate Erubis.new. I can prepare a commit that does that if you want. AFAICT, nothing accesses Erubis directly outside of the Rails ERB Handler except for a test helper (actionview/test/template/erb/helper.rb)

@kaspth
kaspth Jan 23, 2017 Member

I think we have to do that because we advertise the Erubis class here http://api.rubyonrails.org/classes/ActionView/Template/Handlers/Erubis.html

Then we're in an even better position to default to Erubi as the ERB implementation because people have an out.

I think Active Support Deprecation has a method to deprecate a constant which could help here. ๐Ÿ‘

@matthewd
matthewd Jan 23, 2017 Member

I think the body of the Erubis class (and now the Erubi class) should have been / should be fully :nodoc: -- its existence should be documented, but not any of its methods.

As it was published, though, it's probably safer to keep the old one as is (but deprecate it).

Let's throw a :nodoc: all inside Erubi, though.

@jeremyevans
Contributor

I'll try to make the changes requested tomorrow. I don't want to add the @newline_pending code to Erubi itself, though. Do you prefer additional patches, or should I amend the existing commit?

@jeremyevans
Contributor

The new Travis failures look unrelated (EventMachine, and Resolving Dependencies missing in railties output).

@kaspth
Member
kaspth commented Jan 23, 2017

@jeremyevans let's rebase on master to be sure it's not this code ๐Ÿ˜

@jeremyevans jeremyevans Change ActionView ERB Handler from Erubis to Erubi
Erubi offers the following advantages for Rails:

* Works with ruby's --enable-frozen-string-literal option
* Has 88% smaller memory footprint
* Does no freedom patching (Erubis adds a method to Kernel)
* Has simpler internals (1 file, <150 lines of code)
* Has an open development model (Erubis doesn't have a
  public source control repository or bug tracker)
* Is not dead (Erubis hasn't been updated since 2011)

Erubi is a simplified fork of Erubis that contains just the
parts that are generally needed (which includes the parts
that Rails uses).  The only intentional difference in
behavior is that it does not include support for <%=== tags
for debug output.  That could be added to the ActionView ERB
handler if it is desired.

The Erubis template handler remains in a deprecated state
so that code that accesses it directly does not break.  It
can be removed after Rails 5.1.
7bfb6fb
@jeremyevans
Contributor

I've pushed a new commit rebased against master that keeps Erubis and adds Erubi. I've also added nodoc around both Erubis and Erubi, as well as removing the extra line after private.

In terms of deprecating Erubis, I used ActiveSupport::Deprecation::DeprecatedObjectProxy. There is ActiveSupport::Deprecation::DeprecatedConstantProxy for deprecating constants, but it appears to only work for renaming constants, and can't be used when the API differs between the new and old constants. The deprecation message only appears if you call a method on Erubis, just referencing the constant itself will not print a warning. Ruby itself offers a method for deprecating constants (Module#deprecate_constant), so that any reference to the constant results in a deprecation warning, but I'm not sure if it is allowed as the deprecation message style is different.

@jeremyevans
Contributor

Looks like Module#deprecate_constant wasn't added until ruby 2.3, so I don't think it can be used. It's possible to use const_missing to emulate it, if the current deprecation code is not considered good enough.

@@ -72,6 +74,77 @@ def flush_newline_if_pending(src)
end
end
end
+ Erubis = ActiveSupport::Deprecation::DeprecatedObjectProxy.new(erubis, "ActionView::Template::Handlers::Erubis is deprecated and will be removed from Rails 5.2.")
@jeremy
jeremy Jan 23, 2017 Member

๐Ÿ‘

I did some searching to see whether anyone's actually using the Erubis directly or extending it, and there are a handful.

The deprecation message should suggest what to do about it. How about something like

ActionView::Template::Handlers::Erubis is deprecated and will be removed from Rails 5.2. Switch to ActionView::Template::Handlers::Erubi instead.

+ properties[:bufvar] = "@output_buffer"
+ properties[:escapefunc] = ""
+
+ super
@jeremy
jeremy Jan 23, 2017 Member

Kinda surprising that this passes the overwritten properties local var along! I would've expected it to pass the original input and properties.

+ src << "@output_buffer.#{expr}append= " << code
+ else
+ src << "@output_buffer.#{expr}append=(" << code << ");"
+ end
@jeremy
jeremy Jan 23, 2017 Member

Noting that the other impl uses string literals, avoiding the additional interpolation. Not exactly a hot path here for template compilation, but we could do something like

src << "@output_buffer."
src << "safe_expr_" if @escape || indicator == "=="
src << "append="

if BLOCK_EXPR.match?(code)
  src << " " << code
else
  src << "(" << code << ");"
end
@jeremyevans
jeremyevans Jan 23, 2017 Contributor

Frozen literal string support is not turned on yet, but this is a good idea for when it is enabled. I'll use a slightly different approach that will save a 2-3 << calls

actionview/actionview.gemspec
@@ -22,6 +22,7 @@ Gem::Specification.new do |s|
s.add_dependency "activesupport", version
s.add_dependency "builder", "~> 3.1"
+ s.add_dependency "erubi", "~> 1.4"
s.add_dependency "erubis", "~> 2.7.0"
@jeremy
jeremy Jan 23, 2017 Member

Can we drop the Erubis dep? Perhaps by moving the handler to a separate lazy-loaded file with an explicit erubis require or by wrapping the existing impl with a begin require 'erubis'; rescue LoadError; else <ERubis handler> end.

@jeremyevans
jeremyevans Jan 23, 2017 Contributor

I'll include this change in my next commit, but won't it break things for people who access ActionView::Template::Handlers::Erubis and use a Gemfile that doesn't explicitly specify erubis? Then instead of getting a deprecation message, they get a NameError when they reference the constant.

@jeremy
jeremy Jan 25, 2017 Member

Agree that's a poor experience.

Workaround: Have the proxy reference the new constant which is autoloaded from a separate file with an explicit require. Then folks using ActionView::Template::Handlers::Erubis will get a straightforward LoadError.

I'll give this a shot and merge.

@@ -22,6 +23,7 @@ class Base
include ActiveSupport::Configurable
extend ActiveSupport::DescendantsTracker
+ # Remove after ActionView::Template::Handlers::Erubis is removed
@jeremyevans
jeremyevans Jan 23, 2017 Contributor

Erubis adds a private Kernel#not_implemented method. This undef_method was just to remove it. However, if we don't require erubis, we don't need to undef the method, so this can be removed.

I'm not sure what the effects are if erubis is loaded later, though, as the method would end up being defined at that point.

jeremyevans added some commits Jan 23, 2017
@jeremyevans jeremyevans Avoid interpolation to work better with frozen string literals ec7ddad
@jeremyevans jeremyevans Remove dependency on Erubis
Instead, require it, and if a LoadError is raised, do nothing. If
requiring doesn't raise an error, then define the Erubis handler
for deprecation purposes.

Recommend using Erubi template handler in the deprecation warning.
961ab4e
@jeremyevans
Contributor

I pushed up new commits that should address the points raised by @jeremy

@jeremyevans jeremyevans Remove trailing whitespace
a5b52c8
@jeremy
Member
jeremy commented Jan 25, 2017

Merged at 7da8d76. Thank you @jeremyevans!

@jeremy jeremy closed this Jan 25, 2017
@jeremy jeremy removed the needs feedback label Jan 26, 2017
@jeremy jeremy added this to the 5.1.0 milestone Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment