Improving caching

[oliverbarnes]

@ahukkanen just posted some useful findings on an issue from last year for defining a caching strategy that was posted by @armandfardeau, and @alecslupu has also done some analysis of caching that he published on this google doc.

Thought it'd be a good idea to have a go-to place with all the findings, and where the discussion can be continued as well, here.

(@mrcasals and @andreslucena were also involved on that issue discussion)

[ahukkanen]

In general, I think this also has a lot to do with how the interface is designed and how much user specific functionality do we really need to have e.g. in the listing views. Or could we possibly render the cards without any user specific parts and then add those dynamically once the view is loaded?

If we would cache the non-user specific cards separately and the user specific stuff in its own block, it would greatly decrease the memory/space needed to store all of these.

I don't know but something seemed weird to me when comparing the view render time to the GraphQL API running the same query for the proposals listing (which completes successfully in few seconds for 20 items):

Show the GraphQL query

{
  participatoryProcess(slug: "aliquam-esse") {
    components(filter: {type: "proposals"}) {
      ... on Proposals {
        proposals(after: "", first: 20, order: { publishedAt: "DESC" }) {
          pageInfo {
            hasNextPage
            endCursor
          }
          edges {
            node {
              id
              title {
                translation(locale: "en")
              }
              body {
                translation(locale: "en")
              }
              scope {
                id
                name {
                  translation(locale: "en")
                }
              }
              category {
                id
                name {
                  translation(locale: "en")
                }
              }
              authors {
                name
                nickname
                avatarUrl
                deleted
                profilePath
                badge
              }
              state
              createdAt
              endorsementsCount
              totalCommentsCount
              voteCount
            }
          }
        }
      }
    }
  }
}

An important thing to note here is that right now it is not possible to fetch the followers of the proposal or its authors through the API and also it's not possible to fetch the following state of the current user for these records.

So, what about removing the following actions in the views from these two places (I removed these lines):

decidim/decidim-core/app/cells/decidim/card_m/show.erb

Line 20 in b0d9d24

<%= render :status %>

decidim/decidim-core/app/cells/decidim/author/profile_minicard.erb

Lines 10 to 24 in b0d9d24

	<div class="ml-s">
	<%= cell "decidim/follow_button", raw_model %>
	<div>
	<%= link_to decidim.profile_followers_path(raw_model.nickname), class: "card__link" do %>
	<strong><%= model.followers_count %></strong> <%= t("decidim.profiles.show.followers") %>
	<% end %>
	</div>
	<% if model.can_follow? %>
	<div>
	<%= link_to decidim.profile_following_path(raw_model.nickname), class: "card__link" do %>
	<strong><%= model.following_count %></strong> <%= t("decidim.profiles.show.following") %>
	<% end %>
	</div>
	<% end %>
	</div>

I first ran 10 requests with these removed using curl, averaging at 8.53 seconds / request.

Then, I reverted the change and ran the same 10 requests using curl, averaging at 21,34 seconds / request.

The requests were run in the development mode and without caching, both cases running on the exact same configuration.

[ahukkanen]

Yes, it modifies the author but I think the original intention of the test would be that when an author updates their profile, the cache hash will change (if you read the description of the test).

It just happens "by accident" right now because the authors are likely to be also followers in the proposal.

Other than that, I don't see any reason why we should include any other information about the followers in the cache hash than the amount of followers.

[ahukkanen]

I actually think the poor performance mostly caused by model.followers.to_json because apparently the JSON transform is slow.

I changed it in the PR so that it calculates the digest from the authors and it doesn't seem to have any significant performance impact compared to getting the cache keys from the coauthorships instead. It does require more DB queries but after running some test requests both ways, they seem to be performing quite similarly with cache enabled and disabled.

So, I'd say let's just fix it for now like this. It's not optimal but it's still faster as it is right now with the model.followers.to_json in place. According to my quick tests, it's without cache about 30% faster and with cache (when all records on the page are cached) about 40% faster than when the followers are being serialized to JSON.

I think there is also added performance benefit of the followers counter cache in general but I don't have anything to test it with right now, except for those numbers that I presented in the beginning of this thread.

[armandfardeau]

Hello, I am not satisfied with what I have done in the hash cache generation. For me it is a bad answer to a real problem. I really like the idea of retrieving the data asynchronously. This would decrease the time of the first view.

Wouldn't it make things easier to generate a hash from the model concerned? This way, we would only calculate the hash when updating an object

[alecslupu]

I do not think the issue is with the json generation, but the way the data is structured. Every celll you are trying to cache has way too many moving parts. I do think that the caching on the cells would be better if we decouple the resource and model. The key may actually be something like:
d/cells/props/m/id/upodated_at.
Then make sure every interaction (follow, comment) etc is touching the target model.

[ahukkanen]

@alecslupu Yes, the whole caching idea in Decidim is currently very difficult.

I didn't mean the only problem is with the JSON hashing. I'm just saying replacing that one line in #7581 improves the performance and reduces the problem at hand.

The whole caching as a topic is much wider than this. I'm just trying to fix this one problem at hand.

Feel free to benchmark the performance against current develop and then against #7581. I'm quite sure you'll find it performing better.

[oliverbarnes]

In the Trailblazer book apotonick (Trailblazer's creator) mentions using a CacheVersion pattern for cell caching:

The point of CacheVersion is to minimize persistence access in order to compute a cache key. Instead of trying to generate an MD5 hash from all Thing instances visible in the grid, as it is often done, we only need exactly one database lookup to find, or compute, the cache key.

Example implementation:

class Grid < Cell::Concept cache 
  :show do
    CacheVersion.for("thing/cell/grid")
  end

class CacheVersion < ActiveRecord::Base
  def self.for(name)
    where(name: name).first or create(name: name)
  end

  def cache_key # called in AS::Cache#retrieve_cache_key. 
    updated_at
  end

  def expire!
     update_attribute(:updated_at, (updated_at || Time.now) + 1.second)
  end

create_table "cache_versions" do |t|
  t.string "name"
  t.datetime "updated_at"
end

He then describes expiring it in an Operation, Trailblazer's equivalent of Rectify's Command:

class Thing::Create < Trailblazer::Operation
  callback :default, Callback::Default

module Thing::Callback
  class Default < Disposable::Callback::Group
    on_change :expire_cache!

  def expire_cache!(thing, *)
    CacheVersion.for("thing/cell/grid").expire!
  end

Does anyone think this could improve things at all?

(I have to admit I don't fully understand how this alternative approach would map to Decidim's specific case, but thought it might be worth discussing)

[oliverbarnes]

That makes sense, I think it jives with what he says next:

A few things I have to criticize about this approach.

First, didn’t we say logic shouldn’t be in models? Why am I putting expiry and caching logic into the CacheVersion class? This is surely breaking Trailblazer’s idea of logic-less models. At a later point, we will move it to an operation, so we can trigger expires from the console, too.

Does the operation really need to know about view details? Why does the expire_cache! method pass the cell’s view token to CacheVersion? That is tightly coupling business and view. The answer: It’s a quick and easy way to achieve what we need now.

And:

When caching gets more complex, the operation could dispatch to a cache expire class that knows what actions affects which cached parts of the app.

[ahukkanen]

To me this seems like a fancier way to do what I think @mrcasals suggested before, i.e. calling proposal.touch every time one of the proposal card related parameters are changed (follows, endorsements, comments, authors, author profile updates, etc.).

The bigger problem to me at least is the complexity related to the cards. I would rather think of ways to decouple the proposals loading from loading the view itself. And then optimize the dynamic action that loads the proposals. Finally, add caching if the dynamic loading cannot be made more performant.

And if it's possible, remove as much complexity from the cards as possible (e.g. how many people are interested in how many follows the proposal has, or how many people want to see this information on the card, has anyone asked any real users?).

[mrcasals]

To me this seems like a fancier way to do what I think @mrcasals suggested before, i.e. calling proposal.touch every time one of the proposal card related parameters are changed (follows, endorsements, comments, authors, author profile updates, etc.).

With the difference that with Trailblazer'a approach we could have different keys, being able to control when to expire them without all of them being affected by proposal.touch!

And if it's possible, remove as much complexity from the cards as possible (e.g. how many people are interested in how many follows the proposal has, or how many people want to see this information on the card, has anyone asked any real users?).

That's possibly the biggest issue here, I don't know if there were user tests when the design was planned.

[ahukkanen]

With the difference that with Trailblazer'a approach we could have different keys, being able to control when to expire them without all of them being affected by proposal.touch!

If we take e.g. the proposals case, I'm not sure if it's necessary to control separately when to expire specific cache keys per proposal. E.g. if the authors of the proposal change, we have to expire all keys for that proposal. If the amount of follows changes, we have to expire all keys for that proposal. If the amount of comments change, we have to expire all keys for that proposal. (etc.)

It could be also I'm not understanding specifically what you refer to but I think before adding a new technical layer, the use cases should be defined first where it would actually benefit compared to the simpler alternative.

[andreslucena]

And if it's possible, remove as much complexity from the cards as possible (e.g. how many people are interested in how many follows the proposal has, or how many people want to see this information on the card, has anyone asked any real users?).

pinging @decidim/product team
I think one time we've discussed how relevant it was to have the followers' button/counter in the proposal listing page

[alecslupu]

Because we are using a lot of cells, we have situations where a cell like CommentCell, is actually rendering other cells like : report button (Where we aim the relation between current user and the resource in question), also the author of the comment (a cell where we display author specific data and current_user's relation ), and this multiple rendering is causing the cache to be slow as we need to build very complex cache keys like :

 hash = []
        hash.push(I18n.locale)
        hash.push(model.must_render_translation?(current_organization) ? 1 : 0)
        hash.push(model.authored_by?(current_user) ? 1 : 0) # query 
        hash.push(model.reported_by?(current_user) ? 1 : 0) # query 
        hash.push(model.up_votes_count)
        hash.push(model.down_votes_count)
        hash.push(model.cache_key_with_version)
        hash.push(model.author.cache_key_with_version) # query 

Apart of those queries, we also have the queries related to upvotes or downvotes which is being used in an unoptimal way and also other queries.
Considering applications with a large number of active users,and much activity on resources the cache is inefficient.

In order to adopt a more efficient cache, we need to split somehow the caching of the resource and the cache of other things that could be impacted by the current user that is being logged in.
There are 2 ways that we have at hand:

1. Move some of the data on AJAX, but this does not entirely fixes the performance, as we cand generate a lot of ajax requests, flooding the servers.
2. Add some kind of cache registry to help us split the nesting of the resources composing the data back using JS. This does not floods the server, but it may have some negative impact on the load speed on user end.