Skip to content

Conversation

@moberegger
Copy link

@moberegger moberegger commented Jun 17, 2025

Follow up to #15, which made some changes to how blocks were passed around. Did some due diligence and actually found that doing it in _set didn't help. I put the results of some IPS benchmarks below... while they all show "difference falls within error", this new revision does run a bit faster. I had some previous runs showing that this new revision is faster without the error margin, but can't consistently reproduce that. Only included IPS because no differences showed up in memory allocation.

I think it's best to go with this new revision for a few reasons:

  • It might be faster. At worst, no difference.
  • It's closer to what was there before, so fewer deltas with upstream.
  • It keeps things consistent with the rest of the source code, so fewer refactors will be necessary
  • I have seen calls to ::Kernel.block_given? show up as hotspots in our Datadog profiles, so maybe best to save on that if we can.

Oddly enough, having &block in JbuilderTemplate#set! is consistently slower. I have no idea why it matters in one case but not the other.


json.set! :foo, :bar
ruby 3.4.4 (2025-05-14 revision a38531fd3f) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
              before   750.032k i/100ms
               after   721.349k i/100ms
Calculating -------------------------------------
              before      9.085M (± 2.2%) i/s  (110.07 ns/i) -     45.752M in   5.038314s
               after      9.163M (± 1.7%) i/s  (109.14 ns/i) -     46.166M in   5.039896s

Comparison:
               after:  9162918.1 i/s
              before:  9085243.6 i/s - same-ish: difference falls within error

# Where object = { bar: 123 }
json.set! :foo do
  json.extract! object, :bar
end
Warming up --------------------------------------
              before   164.804k i/100ms
               after   184.442k i/100ms
Calculating -------------------------------------
              before      1.755M (± 3.9%) i/s  (569.71 ns/i) -      8.899M in   5.080089s
               after      1.894M (± 7.1%) i/s  (527.90 ns/i) -      9.407M in   4.999829s

Comparison:
               after:  1894297.3 i/s
              before:  1755275.3 i/s - same-ish: difference falls within error

# Where array = [1, 2, 3]
json.set! :foo, array do |item|
  json.set! :bar, item
end
ruby 3.4.4 (2025-05-14 revision a38531fd3f) +YJIT +PRISM [arm64-darwin24]
Warming up --------------------------------------
              before   104.803k i/100ms
               after   111.370k i/100ms
Calculating -------------------------------------
              before    967.022k (±18.6%) i/s    (1.03 μs/i) -      4.611M in   5.036618s
               after      1.115M (± 4.2%) i/s  (897.07 ns/i) -      5.568M in   5.005477s

Comparison:
               after:  1114745.4 i/s
              before:   967022.0 i/s - same-ish: difference falls within error

@moberegger moberegger marked this pull request as ready for review June 18, 2025 15:35
lib/jbuilder.rb Outdated
# json.comments @post.comments { |comment| ... }
# { "comments": [ { ... }, { ... } ] }
_scope { _array(value) { |element| yield element } }
_scope { _array(value, &block) }
Copy link

@mscrivo mscrivo Jun 18, 2025

Choose a reason for hiding this comment

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

I'm having trouble reasoning about this change. Is it actually equivalent to what it was before? since the block yielding an element before?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it's the same. This is what was originally there. The block provided to set! just accepts on param, so this just passes the block instead of yielding to it inside another block.

@moberegger moberegger merged commit 3b9bf0a into main Jun 18, 2025
30 checks passed
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.

3 participants