Action View: Remove internal calls to tag with positional arguments

[seanpdoyle]

Motivation / Background

Follow-up to #49371

In the wake of the deprecation of tag(:div) in favor of tag.div, this commit replaces all calls made internally with the more modern syntax.

This change is separate from the deprecation cycle, since there are slight behavioral changes, since the HTML output changes from self-closing elements like <input /> to open void elements like <input>.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[rafaelfranca]

How much slower are the new calls?

[rafaelfranca]

Building a tag, just to remove the closing tag, and then calling html_safe doesn't seem worthy doing. You are making the framework do more work than it is necessary. This has performance impact.

[seanpdoyle]

This change would be unnecessary if #44275 were merged.

[rafaelfranca]

Simple benchmark shows me it is almost 2 slower

# frozen_string_literal: true

require "bundler/setup"

require "action_view"
require "minitest/autorun"
require "benchmark/ips"

class Foo
  include ActionView::Helpers
end

helpers = Foo.new

Benchmark.ips do |x|
  x.report("tag") { helpers.tag("a", href: "foo") }
  x.report("tag_builder") { helpers.tag.a(href: "foo") }
  x.compare!
end

Warming up --------------------------------------
                 tag    48.738k i/100ms
         tag_builder    29.990k i/100ms
Calculating -------------------------------------
                 tag    441.345k (±21.9%) i/s -      2.096M in   5.067122s
         tag_builder    265.445k (±23.2%) i/s -      1.230M in   5.085887s

Comparison:
                 tag:   441344.9 i/s
         tag_builder:   265445.4 i/s - 1.66x  (± 0.00) slower

I don't think it is worthy changing internal usage.

[seanpdoyle]

@rafaelfranca thank you so much for your detailed review, especially the benchmark.

I would love to address these performance issues enough to make my recent batch of TagBuilder pull requests more viable.

I've opened #49390 to explore what it'd be like to dynamically define methods to avoid calls to #method_missing. I'd really appreciate feedback on the approach. #49390 only explores void and self-closing elements, but if it's deemed to be worth exploring, I'm happy to work on elements with content too.

[seanpdoyle]

In response to #49377 (comment), I've opened #49390. With that being merged, I consider the following to be the remaining barriers to this work progressing:

1. The ActiveModel tag helper: Action View: Remove internal calls to tag with positional arguments #49377 (comment)
2. Rendering open <form> elements requiring String manipulation (comment)

While 1. is a common enough issue to make exploring alternatives a good idea, I wonder if 2. might be addressed by a change like #44275.

[seanpdoyle]

Simple benchmark shows me it is almost 2 slower

The changes introduced by #49390 have changed the difference in performance to 1.34x slower:

# frozen_string_literal: true

require "bundler/setup"
require "action_view"
require "benchmark/ips"

class Foo
  include ActionView::Helpers
end

helpers = Foo.new

Benchmark.ips do |x|
  x.report("tag") { helpers.tag("a", href: "foo") }
  x.report("tag_builder") { helpers.tag.a(href: "foo") }
  x.compare!
end

Warming up --------------------------------------
                 tag    80.406k i/100ms
         tag_builder    59.307k i/100ms
Calculating -------------------------------------
                 tag    808.012k (± 0.7%) i/s -      4.101M in   5.075322s
         tag_builder    602.288k (± 0.7%) i/s -      3.025M in   5.022178s

Comparison:
                 tag:   808012.0 i/s
         tag_builder:   602288.3 i/s - 1.34x  slower

While that's still a degradation in performance, a benchmark one layer up the stack based on ActionDispatch::IntegrationTest yields comparable performance:

<%# app/views/documents/index.html.erb %>
<%= @documents.each do |document| %>
  <% if @use_builder %>
    <%= tag.a document.title, href: "/" %>
  <% else %>
    <%= tag "a", document.title, href: "/" %>
  <% end %>
  <p><%= document.content %></p>
<% end %>

# app/controllers/documents_controller.rb
class DocumentsController < ApplicationController
  class_attribute :use_builder, default: false

  def index
    @documents = Document.all
    @use_builder = use_builder
  end
end

# test/integration/documents_benchmark_test.rb

require "test_helper"
require "benchmark/ips"

class DocumentsIntegrationTest < ActionDispatch::IntegrationTest
  test "index" do
    get "/documents"
    assert_equal 200, response.status
  end
end

Benchmark.ips do |bm|
  bm.report "tag" do
    DocumentsController.use_builder = false
    Minitest.run_one_method(DocumentsIntegrationTest, "test_index")
  end

  bm.report "tag_builder" do
    DocumentsController.use_builder = true
    Minitest.run_one_method(DocumentsIntegrationTest, "test_index")
  end

  bm.compare!
end

The results:

Calculating -------------------------------------
                 tag   153.000  i/100ms
         tag_builder   163.000  i/100ms
-------------------------------------------------
                 tag      1.658k (± 1.6%) i/s -      8.415k
         tag_builder      1.659k (± 1.4%) i/s -      8.313k

Comparison:
         tag_builder:     1659.0 i/s
                 tag:     1657.5 i/s - 1.00x slower

[skipkayhil]

The changes introduced by #49390 have changed the difference in performance to 1.34x slower:

Here's a PR to make it only 12% slower: #50737