Skip to content

Optimize array! and set! #604

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+69 −58
Open

Optimize array! and set! #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fddb7ca
Optimize array! DSL
moberegger Sep 25, 2025
b63e7b1
Optimize set! DSL
moberegger Sep 25, 2025
44b42f9
Remove call to one?
moberegger Sep 25, 2025
b0c88ad
Remove call to Kernel.block_given?
moberegger Sep 25, 2025
d3740a3
Default attributes to nil
moberegger Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 58 additions & 47 deletions lib/jbuilder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require 'jbuilder/errors'
require 'json'
require 'active_support/core_ext/hash/deep_merge'
require 'active_support/core_ext/object/blank'

class Jbuilder
@@key_formatter = nil
Expand All @@ -32,41 +33,12 @@ def self.encode(...)
new(...).target!
end

BLANK = Blank.new
BLANK = Blank.new.freeze
EMPTY_ARRAY = [].freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were quite a few spots allocating a new empty array when dealing with an empty collection. Figured it was worthwhile to re-use the same Array instance. This does assume that no call sites attempt to mutate the output from Jbuilder#target!.

private_constant :BLANK, :EMPTY_ARRAY

def set!(key, value = BLANK, *args, &block)
result = if ::Kernel.block_given?
if !_blank?(value)
# json.comments @post.comments { |comment| ... }
# { "comments": [ { ... }, { ... } ] }
_scope{ array! value, &block }
else
# json.comments { ... }
# { "comments": ... }
_merge_block(key){ yield self }
end
elsif args.empty?
if ::Jbuilder === value
# json.age 32
# json.person another_jbuilder
# { "age": 32, "person": { ... }
_format_keys(value.attributes!)
else
# json.age 32
# { "age": 32 }
_format_keys(value)
end
elsif _is_collection?(value)
# json.comments @post.comments, :content, :created_at
# { "comments": [ { "content": "hello", "created_at": "..." }, { "content": "world", "created_at": "..." } ] }
_scope{ array! value, *args }
else
# json.author @post.creator, :name, :email_address
# { "author": { "name": "David", "email_address": "david@loudthinking.com" } }
_merge_block(key){ _extract value, args }
end

_set_value key, result
_set(key, value, args, &block)
end

# Specifies formatting to be applied to the key. Passing in a name of a function
Expand Down Expand Up @@ -206,18 +178,8 @@ def child!
# json.array! [1, 2, 3]
#
# [1,2,3]
def array!(collection = [], *attributes, &block)
array = if collection.nil?
[]
elsif ::Kernel.block_given?
_map_collection(collection, &block)
elsif attributes.any?
_map_collection(collection) { |element| _extract element, attributes }
else
_format_keys(collection.to_a)
end

@attributes = _merge_values(@attributes, array)
def array!(collection = EMPTY_ARRAY, *attributes, &block)
_array collection, attributes, &block
end

# Extracts the mentioned attributes or hash elements from the passed object and turns them into attributes of the JSON.
Expand All @@ -242,8 +204,8 @@ def extract!(object, *attributes)
end

def call(object, *attributes, &block)
if ::Kernel.block_given?
array! object, &block
if block
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick micro benchmark to show the difference between these

module Foo
  def self.if_block_given?
    block_given? ? true : false
  end

  def self.if_kernel_block_given?
    ::Kernel.block_given? ? true : false
  end

  def self.if_block?(&block)
    block ? true : false
  end
end

Benchmark.ips do |x|
  x.report('block_given?') { Foo.if_block_given? }
  x.report('Kernel.block_given?') { Foo.if_kernel_block_given? }
  x.report('block?') { Foo.if_block? }

  x.compare!
end
ruby 3.4.5 (2025-07-16 revision 20cda200d3) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
        block_given?     2.450M i/100ms
 Kernel.block_given?     2.270M i/100ms
              block?     2.516M i/100ms
Calculating -------------------------------------
        block_given?     44.851M (± 1.9%) i/s   (22.30 ns/i) -    225.368M in   5.026585s
 Kernel.block_given?     39.434M (± 1.5%) i/s   (25.36 ns/i) -    197.469M in   5.008676s
              block?     45.037M (± 2.8%) i/s   (22.20 ns/i) -    226.436M in   5.032194s

Comparison:
              block?: 45036992.5 i/s
        block_given?: 44851312.9 i/s - same-ish: difference falls within error
 Kernel.block_given?: 39434092.2 i/s - 1.14x  slower

_array object, &block
else
_extract object, attributes
end
Expand Down Expand Up @@ -276,6 +238,55 @@ def target!

alias_method :method_missing, :set!

def _set(key, value = BLANK, attributes = nil, &block)
result = if block
if _blank?(value)
# json.comments { ... }
# { "comments": ... }
_merge_block key, &block
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used to be

_merge_block(key){ yield self }

else
# json.comments @post.comments { |comment| ... }
# { "comments": [ { ... }, { ... } ] }
_scope { _array value, &block }
end
Comment on lines +243 to +251
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition used to be the inverse, with a if !_blank?. I think this improves control flow, and it saves a tiny bit in processing.

elsif attributes.empty?
if ::Jbuilder === value
# json.age 32
# json.person another_jbuilder
# { "age": 32, "person": { ... }
_format_keys(value.attributes!)
else
# json.age 32
# { "age": 32 }
_format_keys(value)
end
elsif _is_collection?(value)
# json.comments @post.comments, :content, :created_at
# { "comments": [ { "content": "hello", "created_at": "..." }, { "content": "world", "created_at": "..." } ] }
_scope { _array value, attributes }
else
# json.author @post.creator, :name, :email_address
# { "author": { "name": "David", "email_address": "david@loudthinking.com" } }
_merge_block(key) { _extract value, attributes }
end

_set_value key, result
end

def _array(collection = EMPTY_ARRAY, attributes = nil, &block)
array = if collection.nil?
EMPTY_ARRAY
elsif block
_map_collection(collection, &block)
elsif attributes.present?
_map_collection(collection) { |element| _extract element, attributes }
else
_format_keys(collection.to_a)
end

@attributes = _merge_values(@attributes, array)
end

def _extract(object, attributes)
if ::Hash === object
_extract_hash_values(object, attributes)
Expand Down
22 changes: 11 additions & 11 deletions lib/jbuilder/jbuilder_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def initialize(context, options = nil)
# json.comments @post.comments, partial: "comments/comment", as: :comment, cached: true
#
def partial!(*args)
if args.one? && _is_active_model?(args.first)
if _is_active_model?(args.first)
_render_active_model_partial args.first
else
options = args.extract_options!.dup
Expand Down Expand Up @@ -119,25 +119,25 @@ def target!
@cached_root || super
end

def array!(collection = [], *args)
def array!(collection = EMPTY_ARRAY, *args, &block)
options = args.first

if args.one? && _partial_options?(options)
if _partial_options?(options)
options = options.dup
options[:collection] = collection
_render_partial_with_options options
else
super
_array collection, args, &block
end
end

def set!(name, object = BLANK, *args)
def set!(name, object = BLANK, *args, &block)
options = args.first

if args.one? && _partial_options?(options)
if _partial_options?(options)
_set_inline_partial name, object, options.dup
else
super
_set name, object, args, &block
end
end

Expand All @@ -151,7 +151,7 @@ def _render_partial_with_options(options)
as = options[:as]

if as && options.key?(:collection)
collection = options.delete(:collection) || []
collection = options.delete(:collection) || EMPTY_ARRAY
partial = options.delete(:partial)
options[:locals][:json] = self
collection = EnumerableCompat.new(collection) if collection.respond_to?(:count) && !collection.respond_to?(:size)
Expand All @@ -169,9 +169,9 @@ def _render_partial_with_options(options)
.new(@context.lookup_context, options) { |&block| _scope(&block) }
.render_collection_with_partial(collection, partial, @context, nil)

array! if results.respond_to?(:body) && results.body.nil?
_array if results.respond_to?(:body) && results.body.nil?
else
array!
_array
end
else
_render_partial options
Expand Down Expand Up @@ -233,7 +233,7 @@ def _is_active_model?(object)

def _set_inline_partial(name, object, options)
value = if object.nil?
[]
EMPTY_ARRAY
elsif _is_collection?(object)
_scope do
options[:collection] = object
Expand Down