-
Notifications
You must be signed in to change notification settings - Fork 54
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
Use to_json when rendering json response #181
Conversation
I was trying to debug why an inertia response was 5x slower than the initial page response. After some investigation, it turns out that render json: foo does not actually do the same thing as foo.to_json does. It does a whole lot of rails magic instead, which is both a lot slower and inconsistent. Changing this to use page.to_json ensures consistency with the props output in inertia.html.erb, avoiding weird inconsistencies between props renders and full page renders, and is also a lot faster (especially if you are using a library like Oj to optimise json generation in the first place).
This is awesome, thanks @alexspeller ! Out of curiosity did you do any profiling or metrics for this? It would be cool to see the difference. No need to create something if you didn't. |
I made a controller that renders this data structure (from memory, so not including the time to generate it):
With
|
Wow, that's a big difference! I'm source diving a bit in Rails to understand this better and it looks like this mimics the logic here Am I looking in the wrong place or is there something happening elsewhere first? |
Love this and love the helpful writeup! If I know @skryukov I suspect he did a backflip of joy when he saw "5x faster"! Any reason not to merge this @BrandonShar ? |
@bknoles Only because I'm genuinely not clear on why it's working based on that link I posted above. It looks like the same logic is happening just in different places so I'm hoping to understand a bit more about what's happening here (or be told I'm looking in the wrong place in rails source, it would not be the first time 😄) |
ah ok, i didn't realize that was why you were source diving |
Ok, so the problem lies in Oj's implementation and Rails internally calling Here's a quick benchmark:
|
On the other hand, the patch isn't obscure or anything, and patching our |
Hi, just coming back to this and that's a really good find, after looking at your link to the rails source I couldn't figure it out either and planned to spend some time digging today - thanks for figuring it out first 😄 Note that this still offers a good speedup regardless of Without Ojrequire 'active_support/all'
require 'benchmark/ips'
DATA = (1..100_000).map {
{
a: rand,
b: rand,
c: rand,
d: rand,
h: {
a: rand,
b: rand,
h: {
a: rand,
b: rand,
},
},
}
}
if defined?(Oj)
puts "Oj is loaded"
else
puts "Oj is not loaded"
end
Benchmark.ips do |x|
x.report('to_json') { DATA.to_json }
x.report('to_json({})') { DATA.to_json({}) }
x.report('to_json({prefixes: 123})') { DATA.to_json({prefixes: 123}) }
x.compare!
end
With Oj but without calling Oj.optimize_rails(exact same benchmark script but with (almost exact same results as above) With Oj and calling Oj.optimize_rails(exact same benchmark script but with
It's going to strongly depend on the amount of data and shape of the data, but I think that this should speed up the json render for all users not just Oj users, although the speedup with Oj is more dramatic |
Hey, @alexspeller thanks for digging into this!
Not quite. The difference in your benchmark comes from the fact that Benchmark.ips do |x|
x.report('to_json') { {}; DATA.to_json }
x.report('to_json({})') { DATA.to_json({}) }
x.compare!
end which ends up with similar results:
I still think there are a lot of Oj users out there, so the PR is good to go. Thanks again @alexspeller! |
The empty hash case is the same as calling without a hash - try this one: Benchmark.ips do |x|
x.report("to_json") { { prefixes: 123 }; DATA.to_json }
x.report("to_json({prefixes: 123})") { DATA.to_json({ prefixes: 123 }) }
x.compare!
end
|
Inertia only sends |
Love all this back and forth. I'm mentally bookmarking this for the future when I want to explain why "LOC committed" is not a good measurement of engineering effort/productivity. I like to summarize discussions like this mostly because re-explaining something ensures I actually grok it. Explicitly calling I agree with @skryukov that this is a nice change worth merging even if Oj/Rails quirks could be considered out of our scope. There's no real downside. So, I'm smashing merge! Thanks all for the detective work! |
I was trying to debug why an inertia json response was 5x slower than the initial html response.
After some investigation, it turns out that
render json: foo
does not actually do the same thing asfoo.to_json
does. It does a whole lot of rails magic instead, which is both a lot slower and inconsistent.Changing this to use
page.to_json
ensures consistency with the props output in inertia.html.erb, avoiding weird inconsistencies between props renders and full page renders, and is also a lot faster (especially if you are using a library like Oj to optimise json generation).