Skip to content

Commit

Permalink
drastically reduce object allocations
Browse files Browse the repository at this point in the history
before this change, we were allocating AS::SafeBuffer objects that were
being interpolated in to a string, so the safe buffer object was being
thrown away.  This change only allocates a string (vs a string *and* a
safebuffer) and interpolates the string.

On my test application, this reduced the AS::SafeBuffer objects from
1527k per request to about 500 per request.
  • Loading branch information
tenderlove committed Jun 2, 2014
1 parent 29a1b77 commit 8899503
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
4 changes: 2 additions & 2 deletions actionview/lib/action_view/helpers/tag_helper.rb
Expand Up @@ -139,7 +139,7 @@ def escape_once(html)


def content_tag_string(name, content, options, escape = true) def content_tag_string(name, content, options, escape = true)
tag_options = tag_options(options, escape) if options tag_options = tag_options(options, escape) if options
content = ERB::Util.h(content) if escape content = ERB::Util.unwrapped_html_escape(content) if escape
"<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name.to_sym]}#{content}</#{name}>".html_safe "<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name.to_sym]}#{content}</#{name}>".html_safe
end end


Expand Down Expand Up @@ -174,7 +174,7 @@ def boolean_tag_option(key)


def tag_option(key, value, escape) def tag_option(key, value, escape)
value = value.join(" ") if value.is_a?(Array) value = value.join(" ") if value.is_a?(Array)
value = ERB::Util.h(value) if escape value = ERB::Util.unwrapped_html_escape(value) if escape
%(#{key}="#{value}") %(#{key}="#{value}")
end end
end end
Expand Down
19 changes: 13 additions & 6 deletions activesupport/lib/active_support/core_ext/string/output_safety.rb
Expand Up @@ -19,12 +19,7 @@ module Util
# puts html_escape('is a > 0 & a < 10?') # puts html_escape('is a > 0 & a < 10?')
# # => is a &gt; 0 &amp; a &lt; 10? # # => is a &gt; 0 &amp; a &lt; 10?
def html_escape(s) def html_escape(s)
s = s.to_s unwrapped_html_escape(s).html_safe
if s.html_safe?
s
else
s.gsub(HTML_ESCAPE_REGEXP, HTML_ESCAPE).html_safe
end
end end


# Aliasing twice issues a warning "discarding old...". Remove first to avoid it. # Aliasing twice issues a warning "discarding old...". Remove first to avoid it.
Expand All @@ -36,6 +31,18 @@ def html_escape(s)
singleton_class.send(:remove_method, :html_escape) singleton_class.send(:remove_method, :html_escape)
module_function :html_escape module_function :html_escape


# HTML escapes strings but doesn't wrap them with an ActiveSupport::SafeBuffer.
# This method is not for public consumption! Seriously!
def unwrapped_html_escape(s) # :nodoc:
s = s.to_s
if s.html_safe?
s
else
s.gsub(HTML_ESCAPE_REGEXP, HTML_ESCAPE)
end
end
module_function :unwrapped_html_escape

# A utility method for escaping HTML without affecting existing escaped entities. # A utility method for escaping HTML without affecting existing escaped entities.
# #
# html_escape_once('1 < 2 &amp; 3') # html_escape_once('1 < 2 &amp; 3')
Expand Down

12 comments on commit 8899503

@TALlama
Copy link

@TALlama TALlama commented on 8899503 Jun 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@hlegius
Copy link

@hlegius hlegius commented on 8899503 Jun 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@celeduc
Copy link

@celeduc celeduc commented on 8899503 Jun 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐎 ⌚ 🐇

@tenderlove
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, meant to say 1527 / req to 500 / req. orz.

@ernie
Copy link
Contributor

@ernie ernie commented on 8899503 Jun 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!! ❤️❤️❤️❤️❤️❤️❤️❤️

@BrianTheCoder
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎬

@rizwanreza
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@sgrif
Copy link
Contributor

@sgrif sgrif commented on 8899503 Jun 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, sir!

@shireeshj
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@jewilmeer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@Davidslv
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@ytrezq
Copy link

@ytrezq ytrezq commented on 8899503 Aug 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tenderlove : Sorry but typically, what string could have tags not escaped if passed tounwrapped_html_escapedirectly (“# This method is not for public consumption”) ?

Please sign in to comment.