Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Freeze strings to prevent re-allocation of newline/empty strings #961

Closed
wants to merge 6 commits into from
Closed

Freeze strings to prevent re-allocation of newline/empty strings #961

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f70a7e5
Freeze strings to prevent re-allocation of newline/empty strings
dillonwelch Oct 17, 2017
6858136
CHANGELOG.md note
dillonwelch Oct 20, 2017
b939015
Freeze even more strings
dillonwelch Oct 20, 2017
42f4eaa
Replace interpolated interpolation with cleaner method
dillonwelch Oct 23, 2017
de05e61
Roll back changes to use array joins because it's slower
dillonwelch Oct 24, 2017
2130e84
Rewrite old attribute mapping to save many object allocations
dillonwelch Oct 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 20 additions & 11 deletions lib/haml/compiler.rb
View file
Open in desktop
Expand Up @@ -110,28 +110,28 @@ def compile_tag
push_temple(@attribute_compiler.compile(t[:attributes], object_ref, dynamic_attributes))
push_text(
if t[:self_closing] && @options.xhtml?
" />#{"\n" unless t[:nuke_outer_whitespace]}"
" />#{"\n".freeze unless t[:nuke_outer_whitespace]}"
else
">#{"\n" unless (t[:self_closing] && @options.html?) ? t[:nuke_outer_whitespace] : (!block_given? || t[:preserve_tag] || t[:nuke_inner_whitespace])}"
">#{"\n".freeze unless (t[:self_closing] && @options.html?) ? t[:nuke_outer_whitespace] : (!block_given? || t[:preserve_tag] || t[:nuke_inner_whitespace])}"
end
)

if value && !parse
push_text("#{value}</#{t[:name]}>#{"\n" unless t[:nuke_outer_whitespace]}")
push_text("#{value}</#{t[:name]}>#{"\n".freeze unless t[:nuke_outer_whitespace]}")
end

return if t[:self_closing]

if value.nil?
yield if block_given?
rstrip_buffer! if t[:nuke_inner_whitespace]
push_text("</#{t[:name]}>#{"\n" unless t[:nuke_outer_whitespace]}")
push_text("</#{t[:name]}>#{"\n".freeze unless t[:nuke_outer_whitespace]}")
return
end

if parse
push_script(value, t.merge(:in_tag => true))
push_text("</#{t[:name]}>#{"\n" unless t[:nuke_outer_whitespace]}")
push_text("</#{t[:name]}>#{"\n".freeze unless t[:nuke_outer_whitespace]}")
end
end

Expand Down Expand Up @@ -219,9 +219,9 @@ def text_for_doctype
def push_silent(text, can_suppress = false)
flush_merged_text
return if can_suppress && @options.suppress_eval?
newline = (text == "end") ? ";" : "\n"
newline = (text == "end") ? ";" : "\n".freeze
@temple << [:code, "#{resolve_newlines}#{text}#{newline}"]
@output_line = @output_line + text.count("\n") + newline.count("\n")
@output_line = @output_line + text.count("\n".freeze) + newline.count("\n".freeze)
end

# Adds `text` to `@buffer`.
Expand All @@ -231,7 +231,7 @@ def push_text(text)

def push_temple(temple)
flush_merged_text
@temple.concat([[:newline]] * resolve_newlines.count("\n"))
@temple.concat([[:newline]] * resolve_newlines.count("\n".freeze))
@temple << temple
@output_line += TempleLineCounter.count_lines(temple)
end
Expand Down Expand Up @@ -265,6 +265,14 @@ def push_script(text, opts = {})

unless block_given?
push_generated_script(no_format ? "(#{text}\n).to_s" : build_script_formatter("(#{text}\n)", opts))
# TODO this \n should be able to be frozen but the following tests fails
#
# bin/rails test test/helper_test.rb:596
# bin/rails test test/engine_test.rb:534
#
# Both appear to be testing outer whitespace nuking but at this point
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If anyone has insight into this issue and is willing to help me fix the code/test to be able to freeze this string that would be much appreciated! 😍

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I have no time to assist you, but I would write many binding.prys to see how string that shouldn't be freezed is freezed.

# of the code there is no nuke_outer_whitespace key and the
# nuke_inner_whitespace key is false.
push_text("\n") unless opts[:in_tag] || opts[:nuke_inner_whitespace]
return
end
Expand Down Expand Up @@ -294,14 +302,15 @@ def build_script_formatter(text, opts)

def push_generated_script(text)
@to_merge << [:script, resolve_newlines + text]
@output_line += text.count("\n")
@output_line += text.count("\n".freeze)
end

def resolve_newlines
diff = @node.line - @output_line
return "" if diff <= 0
return "".freeze if diff <= 0
@output_line = @node.line
"\n" * diff
return "\n".freeze if diff == 1
"\n".freeze * diff
end

# Get rid of and whitespace at the end of the buffer
Expand Down
2 changes: 1 addition & 1 deletion lib/haml/generator.rb
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ def on_code(exp)
end

def on_newline
"\n"
"\n".freeze
end

private
Expand Down
5 changes: 2 additions & 3 deletions lib/haml/helpers.rb
View file
Open in desktop
Expand Up @@ -383,7 +383,7 @@ def capture_haml(*args, &block)

captured = haml_buffer.buffer.slice!(position..-1)

if captured == '' and value != haml_buffer.buffer
if captured == ''.freeze and value != haml_buffer.buffer
captured = (value.is_a?(String) ? value : nil)
end

Expand Down Expand Up @@ -641,7 +641,7 @@ def is_haml?
# @param block [Proc] A Ruby block
# @return [Boolean] Whether or not `block` is defined directly in a Haml template
def block_is_haml?(block)
eval('!!defined?(_hamlout)', block.binding)
eval('!!defined?(_hamlout)'.freeze, block.binding)
end

private
Expand Down Expand Up @@ -704,4 +704,3 @@ def is_haml?
false
end
end

4 changes: 3 additions & 1 deletion lib/haml/helpers/action_view_mods.rb
View file
Open in desktop
Expand Up @@ -55,7 +55,9 @@ module TagHelper
def content_tag_with_haml(name, *args, &block)
return content_tag_without_haml(name, *args, &block) unless is_haml?

preserve = haml_buffer.options.fetch(:preserve, %w[textarea pre code]).include?(name.to_s)
@_content_tag_name_cache ||= {}
name_string = @_content_tag_name_cache[name] ||= name.to_s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idea here being that we only needed to run to_s on each content tag once. In particular, I was getting a in here a lot from links. Any better ideas are welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performance is within margin of error but saves a string allocation for each time this method is called.

Warming up --------------------------------------
      master_version   188.121k i/100ms
        fast_version   201.722k i/100ms
Calculating -------------------------------------
      master_version      5.176M (±10.1%) i/s -     25.584M in   5.007528s
        fast_version      5.595M (± 7.3%) i/s -     27.838M in   5.004395s

Comparison:
        fast_version:  5595000.8 i/s
      master_version:  5176423.8 i/s - same-ish: difference falls within error

Copy link
Member

Choose a reason for hiding this comment

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

Having %w[textarea pre code] as constant would be enough. I guess main performance gain comes from that. I'm reluctant to introduce ivar in template evaluation environment, and in general cache is likely to introduce bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@k0kubun I went ahead and added the constant for %w[textarea pre code] in #966. Additionally, what do you think about adding a constant for common content_tag options to cache the name.to_s call? Mainly the things that show up in this search of the Rails repo and any other tag suggestions you have?

Copy link
Member

@k0kubun k0kubun Oct 26, 2017 •
edited

Choose a reason for hiding this comment

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

Your above benchmark is now expired after you add constant, so you should put benchmark again.

As I said,

I'm reluctant to introduce ivar in template evaluation environment, and in general cache is likely to introduce bug.

So at least I wouldn't merge it unless there's SIGNIFICANT performance benefit in benchmark. Basically to_s is not slow as the constant includes only string.

preserve = haml_buffer.options.fetch(:preserve, %w[textarea pre code]).include?(name_string)

if block_given? && block_is_haml?(block) && preserve
return content_tag_without_haml(name, *args) do
Expand Down
6 changes: 3 additions & 3 deletions lib/haml/parser.rb
View file
Open in desktop
Expand Up @@ -559,9 +559,9 @@ def self.parse_class_and_id(list)
case type
when '.'
if attributes[CLASS_KEY]
attributes[CLASS_KEY] += " "
attributes[CLASS_KEY] += " ".freeze
else
attributes[CLASS_KEY] = ""
attributes[CLASS_KEY] = "".freeze
end
attributes[CLASS_KEY] += property
when '#'; attributes[ID_KEY] = property
Expand Down Expand Up @@ -637,7 +637,7 @@ def parse_tag(text)
end

if value.nil?
value = ''
value = ''.freeze
else
value.strip!
end
Expand Down
4 changes: 2 additions & 2 deletions lib/haml/temple_engine.rb
View file
Open in desktop
Expand Up @@ -64,14 +64,14 @@ def precompiled_with_return_value
#
# @return [String]
def precompiled_with_ambles(local_names, after_preamble: '')
preamble = <<END.tr!("\n", ';')
preamble = <<END.tr!("\n".freeze, ';'.freeze)
begin
extend Haml::Helpers
_hamlout = @haml_buffer = Haml::Buffer.new(haml_buffer, #{Options.new(options).for_buffer.inspect})
_erbout = _hamlout.buffer
#{after_preamble}
END
postamble = <<END.tr!("\n", ';')
postamble = <<END.tr!("\n".freeze, ';'.freeze)
#{precompiled_method_return_value}
ensure
@haml_buffer = @haml_buffer.upper if @haml_buffer
Expand Down
4 changes: 2 additions & 2 deletions lib/haml/util.rb
View file
Open in desktop
Expand Up @@ -64,9 +64,9 @@ def check_encoding(str)
# Get rid of the Unicode BOM if possible
# Shortcut for UTF-8 which might be the majority case
if str.encoding == Encoding::UTF_8
return str.gsub(/\A\uFEFF/, '')
return str.gsub(/\A\uFEFF/, ''.freeze)
elsif str.encoding.name =~ /^UTF-(16|32)(BE|LE)?$/
return str.gsub(Regexp.new("\\A\uFEFF".encode(str.encoding)), '')
return str.gsub(Regexp.new("\\A\uFEFF".encode(str.encoding)), ''.freeze)
else
return str
end
Expand Down