Skip to content

Optimize _is_collection? method #590

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moberegger
Copy link

@moberegger moberegger commented May 8, 2025

Saw the _is_collection? helper method showing up as a hotspot in profiles for some of our larger templates.

Running the .all? is 3.75x slower than simply doing a couple of respond_to? comparisons. Comparing the two _is_collection? method implementations directly:

ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
                 new     2.287M i/100ms
                 old   875.646k i/100ms
Calculating -------------------------------------
                 new     39.004M (± 1.3%) i/s   (25.64 ns/i) -    196.665M in   5.043030s
                 old     10.398M (± 3.9%) i/s   (96.18 ns/i) -     52.539M in   5.062959s

Comparison:
                 new: 39003805.0 i/s
                 old: 10397648.9 i/s - 3.75x  slower

As a side bonus, this also prevents an additional array allocation from when _object_respond_to? splats the provided method symbols, which adds up for larger responses that use multiple partials.

Calculating -------------------------------------
                 new     0.000  memsize (     0.000  retained)
                         0.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
                 old    40.000  memsize (     0.000  retained)
                         1.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
                 new:          0 allocated
                 old:         40 allocated - Infx more

A simple benchmark to compare results through the jbuilder API itself. This is the most minimalistic way I found to exercise the condition.

some_array = Array.new(10) { |i| { id: i, name: "name_#{i}" } }
json = Jbuilder.new

Benchmark.ips do |x|
  x.report('old') do
    json.set! :foo, some_array, :id, :name
  end

  x.report('new') do
    json.set! :foo, some_array, :id, :name
  end

  x.hold! 'temp_results'
  x.compare!
end
ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
                 old    18.514k i/100ms
                 new    19.520k i/100ms
Calculating -------------------------------------
                 old    189.515k (± 1.8%) i/s    (5.28 μs/i) -    962.728k in   5.081742s
                 new    198.868k (± 2.5%) i/s    (5.03 μs/i) -    995.520k in   5.009296s

Comparison:
                 new:   198867.8 i/s
                 old:   189515.1 i/s - 1.05x  slower

So ~5% faster, although performance characteristics will depend on how often your template exercises the condition.


As an aside... I'm not sure if this is the expected behaviour or not, but jbuilder does consider a Hash to be a collection, which was surprising to me.

# _foo.json.jbuilder
json.extract! foo, :bar, :baz

# show.json.jbuilder
# @foo = { bar: 1, baz: 2 }

# This will try to render @foo as a collection, resulting in the partial receiving key-value pairs rather than the hash
json.set! :data, @foo, partial: 'foo', as: :foo 

# Would expect the above to be sugar for
json.set! do
  json.partial! 'foo', foo: @foo
end

If this behaviour is not deliberate, I can add an additional guard to have _is_collection? return false when provided Hash. It's more intuitive to have Hashes render as objects, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant