Speed up collection rendering and add support for multifetch collection handling#501
Conversation
yuki24
force-pushed
the
faster-collection-rendering
branch
3 times, most recently
from
22cee2e to
3a5b475
Compare
|
I believe we should look at #502 and merge it first since the CI builds are a little out of date. |
yuki24
force-pushed
the
faster-collection-rendering
branch
from
3a5b475 to
d35bae7
Compare
|
Really cool stuff! We could consider shipping this as a major version bump and drop support for < 6.1 and Ruby < 2.7? It looks like we'd need to trim 5.2 and < 2.5 support anyway. |
yuki24
force-pushed
the
faster-collection-rendering
branch
from
d35bae7 to
92fd6c0
Compare
|
We can also add some sort of feature detection and fall back to the old way on Rails <= 5.2. That way users will easily be able to upgrade to the newer versions of Jbuilder and get a performance boost for free after upgrading Rails. Any thoughts? |
|
Yeah, we can go for that version too and then ditch 5.2 support separately 👍 |
yuki24
force-pushed
the
faster-collection-rendering
branch
3 times, most recently
from
1d34ad8 to
4a0a64a
Compare
|
Alright, Rails <= 5.2 compatibility is back and additional tests are in place. Now I have add a bit more documentation and this should be good for another round of review. |
yuki24
force-pushed
the
faster-collection-rendering
branch
4 times, most recently
from
c4177aa to
1f67fbe
Compare
README.md
Outdated
There was a problem hiding this comment.
I do not believe we have to mention it any more as Jbuilder will have it built-in once this PR is merged. So I just replaced it with how cached: true could be used on collection rendering.
|
All the todos are done now and this should be good for review. Thanks! |
|
Super cool! A separate PR would be to set up CI running on Rails head and automatic once-a-day builds so we don't cause regressions with other refactorings on the Rails side. …would also be good to switch to Buildkite, but @matthewd may remember how to do that. |
yuki24
force-pushed
the
faster-collection-rendering
branch
2 times, most recently
from
4e988ff to
8a4ae2d
Compare
|
Updated the code as suggested. let me know if there is anything I can do. |
|
Is this ready to be merged or is there something missing? Amazing work @yuki24, I'm looking forward to using this! |
yuki24
force-pushed
the
faster-collection-rendering
branch
from
8a4ae2d to
00c92ef
Compare
|
There was a bug where the type of the JSON changes when the collection is empty. Fixed and added a test that covers this case: |
|
+1 |
|
Excellent work, @yuki24! There's some issues with rails master. Could you have a look? I'll get it merged! |
yuki24
force-pushed
the
faster-collection-rendering
branch
from
3ae8a4c to
4df3418
Compare
Turn page_types into array since JBuilder 2.11.3 (rails/jbuilder#501) requires collection to respond to `empty?`, which `Enumerable` does not provide. See also rails/jbuilder#514 REDMINE-19357
Turn enumerables into arrays since JBuilder 2.11.3 (rails/jbuilder#501) requires collection to respond to `empty?`, which `Enumerable` does not provide. See also rails/jbuilder#514 REDMINE-19357
Turn enumerables into arrays since JBuilder 2.11.3 (rails/jbuilder#501) requires collection to respond to `empty?`, which `Enumerable` does not provide. See also rails/jbuilder#514 REDMINE-19357
# Goal We should be able to effectively render collection of data and be able to cache it. ## Problem statement Previously, we have used following code to render collection: ```ruby pb.cache!(@account.offers_cache_key, expires_in: 12.hours) do pb.featured_offers @featured_offers, partial: "offer", as: :offer pb.normal_offers @normal_offers, partial: "offer", as: :offer end ``` But we need to introduce fragment caching to offers, since list cache has high miss rate. This is how implementing fragment caching in pbbuilder should look like: ```ruby pb.cache!(@account.offers_cache_key, expires_in: 12.hours) do pb.featured_offers partial: "offer", as: offer, collection: @featured_offers, cached: true pb.normal_offers partial: "offer", as: :offer, collection: @normal_offers, cached: true end ``` This syntax is heavily inspired by jbuilder. Here is list of examples from that gem that uses cached: and collection: attributes. ```ruby json.partial! partial: 'posts/post', collection: @posts, as: :post ``` ```ruby json.partial! 'posts/post', collection: @posts, as: :post ``` ```ruby json.partial! "post", collection: @posts, cached: true, as: :post ``` ```ruby json.array! @posts, partial: "post", as: :post, cached: true ``` ```ruby json.array! @posts, partial: "posts/post", as: :post, cached: true ``` ```ruby json.comments @post.comments, partial: "comments/comment", as: :comment, cached:true ``` ## Proposed solution In this PR, we're implementing more effective rendering of collection with a help of `ActiveView::CollectionRenderer`. Support for caching of partial is currently not in the scope, but this will help implement caching and caching digest later. Following syntax should be supported: ```ruby pb.friends partial: "racers/racer", as: :racer, collection: @Racers ``` ```ruby pb.friends "racers/racer", as: :racer, collection: @Racers ``` Previous syntax works as it used to before. These will remain unchanged and will not use collection renderer (at least for now) ```ruby pb.partial! @racer, racer: Racer.new(123, "Chris Harris", friends) ``` ```ruby pb.friends @friends, partial: "racers/racer", as: :racer ``` ## TODO - [x] Add documentation for this change - [ ] Check performance difference between Collection Renderer and our approach. (without cache for now) - [x] It's using PartialRenderer with rails 7 now - this seems like a bug, it should use CollectionRenderer. ## Prior work Some inspiration for this PR could be found here: * rails/jbuilder#501 * https://github.com/rails/rails/blob/main/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb#L20 * https://github.com/rails/rails/tree/main/actionview collection_renderer * https://github.com/rails/rails/blob/main/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb#L20
4 participants
This PR mainly improves two things:
Basically, the idea is to create a class that extends the
ActionView::CollectionRenderer(orActionView::PartialRendererin Rails 6.0) and use theScopedIteratoron each iteration of the collection, so that we will be able to render JSON for each element without having tofind_template, which is known to be slow. Aside from that, we will be able to use other features of therendermethod as well because it's the samerenderwe use in a typical Rails view.The biggest winner here is the ability to use
cached: truethat multi-fetches fragments in one shot from the cache store. This was originally proposed by @dhh here: #399 (comment), but we haven't been able to implement it because of the tricky rendering logic in Jbuilder. However since Rails 6.0, the code for collection caching has been improved and more structured. As a result, I was able to extend and re-use the renderer in Jbuilder.Here are the performance comparison:
json.cache!)cached: trueor cold)cached: true, warmed up)As you can see, collection rendering is now faster by 200ms in the example below, with the ability fo multi-fetch if needed!
That means if there is a record that was updated, it is going to be the only record that will get re-generated:
And when
curl http://localhost:3000/statesis run, the following log would be displayed:Now the existing cache has been re-used and there is only one fragment that got re-generated! This is a huge performance boost, and I have already confirmed that this works pretty well with one of my clients.
The major downside of this is that we will have to drop support for Rails 5.2 and earlier. Because the collection caching logic isn't as extendable as it is in Rails 6.0 and later, it was a lot more complicated to do the same in Rails 5.2. I thought that was a stopping point and decided to send this PR.
Let me know what you all think about this. Thanks!
Remaining Todos
cached: trueusageJbuilder::CollectionRendererExample code
The data is based on the 50 U.S. states and cities for each state. The import script could be found here. The
/states.jsonendpoint returns a list of all the 50 states and the first 20 cities for each state.Raw results of
$ ab -n100 http://localhost:3000/states.jsonVersion 2.10.1
Version 2.10.1 (with
the existing json.cache!)With this PR (without
cached: true)With this PR (with
cached: true)