Repository navigation
Optimize _blank? - #622
Optimize _blank?#622moberegger wants to merge 3 commits into
_blank?#622Conversation
| def ==(other) | ||
| super || Blank === other | ||
| end | ||
|
|
There was a problem hiding this comment.
Not necessary for the optimization, but figured I'd remove it since it is no longer running in the gem.
I suppose it's possible that a user is doing something with Jbuilder::Blank directly that may require this, but I believe this class was intended to be an implementation detail.
There was a problem hiding this comment.
Since this is technically a change to the public API (Jbuilder::Blank is not marked with # :nodoc:), I think it'd be best (and increase the likelihood of this PR being reviewed and merged) to maintain the method and open a separate PR to deprecate it.
There was a problem hiding this comment.
Yup, I can put it back no problem!
To clarify:
but I believe this class was intended to be an implementation detail.
I failed to mention that my only rationale there was that the Readme doesn't mention Blank.
| def ==(other) | ||
| super || Blank === other | ||
| end | ||
|
|
There was a problem hiding this comment.
Since this is technically a change to the public API (Jbuilder::Blank is not marked with # :nodoc:), I think it'd be best (and increase the likelihood of this PR being reviewed and merged) to maintain the method and open a separate PR to deprecate it.
|
Could you share the source for the IPS benchmark mentioned in the pull request description? |
|
@seanpdoyle Updated the PR description to add the bench source. Also updated the results for Ruby 4.0.7 (originally they were run on 4.0.6). Also included a run with YJIT enabled (which was missing before). YJIT is a tad faster, but still falls within the margin of error according to the |
This reverts commit 165714a.
|
Interestingly, when initializing a new Benchmark.ips do |x|
x.report('old') do |n|
i = 0
while i < n
i += 1
json = Jbuilder.new
json.set! :foo do
json.set! :bar, 123
end
end
end
x.report('new') do |n|
i = 0
while i < n
i += 1
json = Jbuilder.new
json.set! :foo do
json.set! :bar, 123
end
end
end
x.hold! 'ips_results'
x.compare! order: :baseline
end |
Switches
_blank?fromBLANK == valuetoBlank === value.Originally
_blank?did a Ruby-level dispatch toBlank#==, which itself callssuper(for an identity check) and thenBlank === other. We can short circuit this to simply beBlank === other. This gives us the same result for less work becauseBlank#==would always returntruefor anyBlankinstance. We don't actually need to an identity check.Below is an IPS benchmark against a simple builder that exercises
_blanka few times.Without YJIT
With YJIT