Skip to content
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

Speed up collection rendering and add support for multifetch collection handling #501

Merged
merged 1 commit into from
Nov 14, 2021

Conversation

yuki24
Copy link
Contributor

@yuki24 yuki24 commented Feb 22, 2021

This PR mainly improves two things:

  • Speed up collection rendering by taking advantage of the existing collection renderer in ActionView
  • Add support for multifetch collection handling to make it more efficient

Basically, the idea is to create a class that extends the ActionView::CollectionRenderer (or ActionView::PartialRenderer in Rails 6.0) and use the ScopedIterator on each iteration of the collection, so that we will be able to render JSON for each element without having to find_template, which is known to be slow. Aside from that, we will be able to use other features of the render method as well because it's the same render we use in a typical Rails view.

The biggest winner here is the ability to use cached: true that 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:

Variant multifetch Time per request [ms]
Version 2.10.1 (without caching) - 882.299
Version 2.10.1 (with the existing json.cache!) 103.909
This PR (without cached: true or cold) - 662.832
This PR (with cached: true, warmed up) 96.911

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:

$ rails c
> City.first.touch
=> true

And when curl http://localhost:3000/states is run, the following log would be displayed:

Started GET "/states" for ::1 at 2021-02-22 10:04:08 -0500
Processing by StatesController#index as */*
  Rendering states/index.json.jbuilder
  State Load (0.4ms)  SELECT "states".* FROM "states"
  ↳ app/views/states/index.json.jbuilder:1
  City Load (5.2ms)  SELECT "cities".* FROM "cities" WHERE "cities"."state_id" = ?  [["state_id", 129]]
  ↳ app/views/states/index.json.jbuilder:1
  Rendered collection of cities/_city.json.jbuilder [19 / 20 cache hits] (Duration: 2.9ms | Allocations: 1816)
  Rendered collection of states/_state.json.jbuilder [50 / 51 cache hits] (Duration: 107.3ms | Allocations: 59640)
  Rendered states/index.json.jbuilder (Duration: 160.0ms | Allocations: 125006)
Completed 200 OK in 161ms (Views: 155.0ms | ActiveRecord: 5.7ms | Allocations: 125215)

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

  • Keep the compatibility with Rails <= 5.2
  • Add support for Rails 6.0
  • Add a bit more test coverage around the cached: true usage
  • Add documentation to Jbuilder::CollectionRenderer
  • Update README

Example 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.json endpoint returns a list of all the 50 states and the first 20 cities for each state.

# app/models/state.rb
#
# t.string "name"
# t.string "abbreviation"
# t.datetime "created_at", precision: 6, null: false
# t.datetime "updated_at", precision: 6, null: false
class State < ApplicationRecord
  has_many :cities
end
# app/models/city.rb
#
# t.integer "state_id"
# t.string "name"
# t.string "county"
# t.datetime "created_at", precision: 6, null: false
# t.datetime "updated_at", precision: 6, null: false
class City < ApplicationRecord
  belongs_to :state, touch: true
end
# app/controllers/states_controller.rb
class StatesController < ApplicationController
  def index
    @states = State.all.includes(:cities)
  end
end
# app/views/states/index.json.jbuilder
json.array! @states, partial: "states/state", as: :state, cached: true
# app/views/states/_state.json.jbuilder
json.extract! state, :id, :name, :abbreviation, :created_at, :updated_at
json.url state_url(state, format: :json)

json.cities state.cities.first(20), partial: "cities/city", as: :city, cached: true
# app/views/cities/_city.json.jbuilder
json.extract! city, :id, :name, :county, :created_at, :updated_at

Raw results of $ ab -n100 http://localhost:3000/states.json

Version 2.10.1
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:
Server Hostname:        localhost
Server Port:            3000

Document Path:          /states
Document Length:        140948 bytes

Concurrency Level:      1
Time taken for tests:   88.230 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      14141100 bytes
HTML transferred:       14094800 bytes
Requests per second:    1.13 [#/sec] (mean)
Time per request:       882.299 [ms] (mean)
Time per request:       882.299 [ms] (mean, across all concurrent requests)
Transfer rate:          156.52 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   586  882 182.6    831    1568
Waiting:      586  882 182.6    831    1568
Total:        586  882 182.6    831    1568

Percentage of the requests served within a certain time (ms)
  50%    831
  66%    899
  75%    964
  80%    982
  90%   1130
  95%   1268
  98%   1527
  99%   1568
 100%   1568 (longest request)
Version 2.10.1 (with the existing json.cache!)
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:
Server Hostname:        localhost
Server Port:            3000

Document Path:          /states
Document Length:        140948 bytes

Concurrency Level:      1
Time taken for tests:   10.391 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      14141100 bytes
HTML transferred:       14094800 bytes
Requests per second:    9.62 [#/sec] (mean)
Time per request:       103.909 [ms] (mean)
Time per request:       103.909 [ms] (mean, across all concurrent requests)
Transfer rate:          1329.01 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       1
Processing:    92  104   9.8    100     144
Waiting:       92  103   9.8     99     144
Total:         92  104   9.8    100     144

Percentage of the requests served within a certain time (ms)
  50%    100
  66%    102
  75%    107
  80%    111
  90%    122
  95%    125
  98%    129
  99%    144
 100%    144 (longest request)
With this PR (without cached: true)
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:
Server Hostname:        localhost
Server Port:            3000

Document Path:          /states
Document Length:        140948 bytes

Concurrency Level:      1
Time taken for tests:   66.283 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      14141100 bytes
HTML transferred:       14094800 bytes
Requests per second:    1.51 [#/sec] (mean)
Time per request:       662.832 [ms] (mean)
Time per request:       662.832 [ms] (mean, across all concurrent requests)
Transfer rate:          208.34 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   427  663 229.6    570    1225
Waiting:      426  662 229.6    570    1225
Total:        427  663 229.6    570    1225

Percentage of the requests served within a certain time (ms)
  50%    570
  66%    663
  75%    688
  80%    715
  90%   1134
  95%   1170
  98%   1212
  99%   1225
 100%   1225 (longest request)
With this PR (with cached: true)
$ ab -n100 http://localhost:3000/states
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:
Server Hostname:        localhost
Server Port:            3000

Document Path:          /states
Document Length:        140948 bytes

Concurrency Level:      1
Time taken for tests:   9.691 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      14141100 bytes
HTML transferred:       14094800 bytes
Requests per second:    10.32 [#/sec] (mean)
Time per request:       96.911 [ms] (mean)
Time per request:       96.911 [ms] (mean, across all concurrent requests)
Transfer rate:          1424.99 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    73   97  20.3     94     202
Waiting:       73   96  20.3     94     202
Total:         74   97  20.3     94     202

Percentage of the requests served within a certain time (ms)
  50%     94
  66%    102
  75%    108
  80%    110
  90%    122
  95%    138
  98%    146
  99%    202
 100%    202 (longest request)

@yuki24 yuki24 force-pushed the faster-collection-rendering branch 3 times, most recently from 22cee2e to 3a5b475 Compare February 22, 2021 03:08
@yuki24
Copy link
Contributor Author

yuki24 commented Feb 22, 2021

I believe we should look at #502 and merge it first since the CI builds are a little out of date.

@kaspth
Copy link
Contributor

kaspth commented Feb 22, 2021

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
Copy link
Contributor Author

yuki24 commented Feb 23, 2021

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?

@kaspth
Copy link
Contributor

kaspth commented Feb 23, 2021

Yeah, we can go for that version too and then ditch 5.2 support separately 👍

@yuki24 yuki24 force-pushed the faster-collection-rendering branch 3 times, most recently from 1d34ad8 to 4a0a64a Compare February 24, 2021 02:00
@yuki24
Copy link
Contributor Author

yuki24 commented Feb 24, 2021

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 yuki24 force-pushed the faster-collection-rendering branch 4 times, most recently from c4177aa to 1f67fbe Compare February 26, 2021 19:52
@@ -253,9 +255,17 @@ json.cache_if! !admin?, ['v1', @person], expires_in: 10.minutes do
end
```

If you are rendering fragments for a collection of objects, have a look at
`jbuilder_cache_multi` gem. It uses fetch_multi (>= Rails 4.1) to fetch
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@yuki24
Copy link
Contributor Author

yuki24 commented Feb 26, 2021

All the todos are done now and this should be good for review. Thanks!

lib/jbuilder/collection_renderer.rb Outdated Show resolved Hide resolved
lib/jbuilder/collection_renderer.rb Outdated Show resolved Hide resolved
lib/jbuilder/collection_renderer.rb Outdated Show resolved Hide resolved
lib/jbuilder/collection_renderer.rb Outdated Show resolved Hide resolved
lib/jbuilder/collection_renderer.rb Show resolved Hide resolved
lib/jbuilder/collection_renderer.rb Outdated Show resolved Hide resolved
@kaspth
Copy link
Contributor

kaspth commented Mar 7, 2021

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 yuki24 force-pushed the faster-collection-rendering branch 2 times, most recently from 4e988ff to 8a4ae2d Compare March 9, 2021 02:38
@yuki24
Copy link
Contributor Author

yuki24 commented Mar 9, 2021

Updated the code as suggested. let me know if there is anything I can do.

@tomazzlender
Copy link

Is this ready to be merged or is there something missing?

Amazing work @yuki24, I'm looking forward to using this!

@yuki24 yuki24 force-pushed the faster-collection-rendering branch from 8a4ae2d to 00c92ef Compare June 26, 2021 19:02
@yuki24
Copy link
Contributor Author

yuki24 commented Jun 26, 2021

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:

test "returns an empty array for an empty collection" do
result = render('json.array! @posts, partial: "post", as: :post, cached: true', posts: [])
# Do not use #assert_empty as it is important to ensure that the type of the JSON result is an array.
assert_equal [], result
end

@ghost
Copy link

ghost commented Nov 12, 2021

+1

@dhh
Copy link
Member

dhh commented Nov 13, 2021

Excellent work, @yuki24! There's some issues with rails master. Could you have a look? I'll get it merged!

@yuki24
Copy link
Contributor Author

yuki24 commented Nov 14, 2021

@dhh The test seems to be failing even on the master branch, so I sent a separate PR to fix that #513. Once that PR is merged everything including this PR should be green.

@dhh dhh merged commit d84c37d into rails:master Nov 14, 2021
@yuki24 yuki24 deleted the faster-collection-rendering branch November 14, 2021 11:59
tf added a commit to tf/pageflow that referenced this pull request Nov 24, 2021
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
tf added a commit to tf/pageflow that referenced this pull request Nov 24, 2021
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
rafaelfranca added a commit that referenced this pull request Dec 21, 2021
Jbuilder since #501 depends on Action View so we need to make it a
dependency.

Also we should be requiring both action_view and active_support before
using it.

Closes #518.
yuki24 added a commit to yuki24/jbuilder that referenced this pull request Feb 17, 2022
UsmanMuhammad pushed a commit to UsmanMuhammad/pageflow that referenced this pull request Aug 12, 2022
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
skatkov added a commit to cheddar-me/pbbuilder that referenced this pull request Oct 23, 2023
# 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
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.

4 participants