Performance Improvements

[matthewmcgarvey]

This does a few things to help improve performance but there is one main improvement.

1. Lazily initialize the String::Builder so that if you are rendering subcomponents and passing it the buffer it doesn't create an unnecessary object
2. Add generated tag methods for when no attributes are passed so we avoid all the attribute-related code
3. Instead of always calling gsub, check if the character to replace is present first. (Weird, I know, but it does improve performance)
4. Cache attribute strings. This is a class-level cache which never gets emptied. This could potentially bloat memory in long-running code or huge views (though it's that very code it would help the most) This is the biggest improvement

(minor change: added diff to print an understandable message when benchmark strings vary between blueprint and ecr)

Results: This almost doubles the speed of the library making it a little over 3x slower than ECR where it was 6x slower before. This would be more if sub-components were also used in the benchmark, but not too different.

[stephannv]

Thanks @matthewmcgarvey. I have some thoughts about this PR:

Lazy initialize

Nice catch, the change saves some memory and the code keeps simple. Suggestion: You can use getter to make it lazy loaded/memoized instead using nilable property:

getter buffer : String::Builder do
  String::Builder.new
end

About 2 & 3

About 2, I'm not sure if thas brings some perfomance improvement, so it would be nice some benchmark isolating this to check it out. In my tests it didn't effect the benchmark results.

About 3, in the past I tried this approach on parse_name method but it didn't improve the results. I think that passing invalid/dangerous strings to attributes is an edge case and #gsub already checks if the attribute is present before trying to replace it. So I isolated your changes and I didn't see any sensible improvements (results below).

Benchmark

class WithConditional
  getter buffer : String::Builder do
    String::Builder.new
  end

  def run(value)
    if value.includes?('"')
      buffer << value.gsub('"', "&quot;")
    else
      buffer << value
    end
  end
end

class WithoutConditional
  getter buffer : String::Builder do
    String::Builder.new
  end

  def run(value)
    buffer << value.gsub('"', "&quot;")
  end
end


Benchmark.ips do |x|
  x.report("With") do
    with_conditional = WithConditional.new
    with_conditional.run("test")
    with_conditional.run("some long string here to test the method")
    with_conditional.run("some long \" string here to test the method")
    with_conditional.run("btn btn-red")
    with_conditional.run("card shadow")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    with_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
  end
  x.report("Without") do
    without_conditional = WithoutConditional.new
    without_conditional.run("test")
    without_conditional.run("some long string here to test the method")
    without_conditional.run("some long \" string here to test the method")
    without_conditional.run("btn btn-red")
    without_conditional.run("card shadow")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
    without_conditional.run("hidden peer-checked:flex w-full h-full mask items-center justify-center border-[2px] ring-[2px] ring-offset-1 ring-base-content rounded-full")
  end
end

Results:

   With   1.29M (777.05ns) (± 0.74%)  4.47kB/op        fastest
Without   1.28M (778.69ns) (± 0.38%)  4.47kB/op   1.00× slower

Attributes caching

About attributes caching, I gave a try in this project in the past but I reverted the chances since I need to ensure it to be thread safe using a kind of lock/sync. Something like this:

@[Experimental]
module Blueprint::HTML::AttributeCaching
  CACHE_MUTEX = Mutex.new
  private CACHE = {} of UInt64 => String

  private def append_attributes(attributes : NamedTuple) : Nil
    return if attributes.empty?

    attributes_hash : UInt64 = attributes.hash

    CACHE_MUTEX.synchronize do
      if cached_attributes = CACHE[attributes_hash]?
        @buffer << cached_attributes
      else
        # ... processes attributes here  
        CACHE[attributes_hash] = processed_attributes
        @buffer << processed_attributes
      end
    end
  end
end

So I don't want this complexity in the project right now because it needs some maintenance and advanced tests.

Since the users can implement their own module, they can copy & paste my or yours attribute cache implementation in their projects.

About crystal-diff

About the addded dependency, since benchmark has the purpose to check any perf regression/improvement, its primary goal is not debug possible wrong html generation, so I would avoiding adding it to the project.

[stephannv]

Thanks. I detailed my review in previous comments. So, if you agree, we need to keep the lazy load/memoization on @buffer.

[stephannv]

Improvement

Suggested change

	@buffer : String::Builder?
	private def buffer : String::Builder
	@buffer ||= String::Builder.new
	end
	getter buffer : String::Builder do
	String::Builder.new
	end

[stephannv]

thoughts
I think we don't need to sofisticate this.

[stephannv]

thoughts
Since we don't want to sofisticate benchmark tests, we would not need this anymore.

[stephannv]

Problem
Cache attributes brings some complexity that we don't want in the project right now because we need some thread-safe code and advanced tests.

And I think changes in parsing attribute name/value don't bring sensible improvements in performance.

[stephannv]

Thoughts
I don't think it brings sensible improvements to benchmark resulst. But if we have some benchmark showing the benefits we can merge this.

[matthewmcgarvey]

Thanks for looking it over. I should have just made a draft PR because I figured this would not be desired as-is. I'll isolate the code without the caching and verify there are improvements.

[stephannv]

@matthewmcgarvey about attributes caching, you motivated to work on a new version of my previous work with thread-safe code. This is the PR: #120

[matthewmcgarvey]

You were right. None of the other stuff mattered as much as the caching. I know I ran performance tests on gsub for checking first, but it might have been just straight up benchmarking the string directly instead of this library and I can't remember the results. I found no change when implementing it in the library and running the benchmark.