Homepage content blocks cache by armandfardeau · Pull Request #6235 · decidim/decidim

armandfardeau · 2020-06-23T16:21:24Z

🎩 What? Why?

Add cache to homepage content blocks
This P.R. is an implementation proposal for a caching and hash testing strategy.

📌 Related Issues

Related to Defining a caching strategy #6229
Fixes Last activities home page block causes performance trouble #6142

📋 Subtasks

Add tests

mrcasals · 2020-06-23T18:24:57Z

decidim-core/lib/decidim/view_model.rb

    private

+    def perform_caching?
+      false


You can define cache_hash in the ViewModel, make it return nil by default, and change perform_caching? body to cache_hash.present? so that you skip required method 😄

Yes thank you!

andreslucena · 2020-06-25T06:18:09Z

Hi @armandfardeau can you please fix the title and link the github issue 🙏 ? Thanks

tramuntanal

Hi @armandfardeau good start! 👏
I've just left you a couple of questions so that we are aligned when you retake this PR

tramuntanal · 2020-07-07T09:30:27Z

decidim-core/app/cells/decidim/content_blocks/hero_cell.rb

+      private
+
+      def cache_hash
+        "decidim/content_blocks/hero/#{Digest::MD5.hexdigest(model.attributes.to_s)}/#{current_organization.cache_version}"


why don't use model.updated_at instead of hashing all the attributes?
shouldn't we take into account also the current locale?

Definitely the locale is needed so we have a cached version for each one.

why don't use model.updated_at instead of hashing all the attributes?
shouldn't we take into account also the current locale?

The model doesn't have an updated_at attribute in this case, nor a cache_version therefore it is needed to hash all attributes.

definitely, I just added them

why don't use model.updated_at instead of hashing all the attributes?

I thought exactly the same when reviewing this... @armandfardeau could you add your answer as a comment in the code 🙏 ?

why don't use model.updated_at instead of hashing all the attributes?

I thought exactly the same when reviewing this... @armandfardeau could you add your answer as a comment in the code 🙏 ?

For sure.

tramuntanal · 2020-07-07T09:30:46Z

decidim-core/app/cells/decidim/content_blocks/last_activity_cell.rb

      private

+      def cache_hash
+        "decidim/content_blocks/last_activity/#{Digest::MD5.hexdigest(valid_activities.map(&:updated_at).to_s)}"


same as with the preivous cell

same as with the preivous cell

In this case, suppose that the number of valid activities increases to a large number, which means a long identification key for the cache system to find. With an MD5 hash, we make sure that the key will remain the same length.

armandfardeau · 2020-07-17T07:01:37Z

I will be able to work on it on Monday, thanks for your feedbacks

stale · 2020-09-20T03:55:10Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. @carolromero & @andreslucena feel free to chime in.

armandfardeau · 2020-09-21T18:26:06Z

@andreslucena It's been a long time and I should have moved on this but I think we need to open the door to a standardised way of testing hash of cached items. This implementation is a modest proposal.

mrcasals · 2020-09-21T19:05:55Z

@armandfardeau I love French, but do you mind translating your previous comment into English? I'm guessing you had a rough day and it just slipped 🙈

armandfardeau · 2020-09-22T07:55:03Z

@armandfardeau I love French, but do you mind translating your previous comment into English? I'm guessing you had a rough day and it just slipped see_no_evil

Damn what a messy day! thanks

ivan-mr

Everything seems ok. Thanks @armandfardeau !

ivan-mr · 2020-09-23T20:05:10Z

There's an internal error in the repository. As soon as we solve it I will merge this PR.
Thanks @armandfardeau !

armandfardeau · 2020-09-24T10:21:52Z

There's an internal error in the repository. As soon as we solve it I will merge this PR.
Thanks @armandfardeau !

That Segfault looks like a nightmare, good luck

ivan-mr · 2020-09-25T14:21:21Z

The segfault is now fixed in develop branch. Can you merge it with your branch to solve it @armandfardeau , please? Thanks!

mrcasals · 2020-09-28T18:03:44Z

@armandfardeau there's something funky with the cache and the tests!

armandfardeau · 2020-10-01T09:22:27Z

@armandfardeau there's something funky with the cache and the tests!

Thanks, I forgot to get back to English after switching locale, silly me.

However, there are some issues on Meetings and Admin (I suspect a drop in performance)

armandfardeau · 2020-10-13T11:50:27Z

@ivan-mr Is the error I encounter a known flaky?

ivan-mr · 2020-10-13T22:35:20Z

@ivan-mr Is the error I encounter a known flaky?

The meeting test failure seems to be a flaky but the error on Admin you say, I don't see any error on it.
Anyway, seems to be solved right now?

ivan-mr

Looks great. Thanks @armandfardeau !

mrcasals reviewed Jun 23, 2020

View reviewed changes

armandfardeau changed the title ~~Performance/homepage content blocks cache~~ Homepage content blocks cache Jun 25, 2020

tramuntanal reviewed Jul 7, 2020

View reviewed changes

stale bot added the wontfix label Sep 20, 2020

armandfardeau added 7 commits September 21, 2020 19:22

Add cache to hero cell

f7ff649

Add cache to last activity

6a4630a

Fix rubocop offenses

6b31fec

Don't herit perform caching method

560a1be

add current locale to cache hash

76b5a19

add current locale to cache hash

31a8995

Fix offenses

8614dde

stale bot removed the wontfix label Sep 21, 2020

armandfardeau marked this pull request as ready for review September 21, 2020 18:23

ivan-mr added in-review module: core labels Sep 23, 2020

ivan-mr self-assigned this Sep 23, 2020

ivan-mr self-requested a review September 23, 2020 18:28

ivan-mr previously approved these changes Sep 23, 2020

View reviewed changes

armandfardeau added 2 commits September 25, 2020 17:14

Merge branch 'develop' into performance/homepage-content-blocks-cache

0b673da

Add comment on method cache_hash

f383445

armandfardeau dismissed ivan-mr’s stale review via f383445 September 28, 2020 13:47

armandfardeau requested review from andreslucena and ivan-mr September 28, 2020 13:47

Revert back to original locale after test

2f22ebb

armandfardeau added 2 commits October 12, 2020 16:56

Use a regex instead of changing locale

935abdb

Fix offense

74cfe50

ivan-mr approved these changes Oct 13, 2020

View reviewed changes

ivan-mr merged commit e29c9e3 into decidim:develop Oct 13, 2020

armandfardeau deleted the performance/homepage-content-blocks-cache branch October 14, 2020 13:11

mrcasals added the target: developer-experience label Feb 25, 2021

Uh oh!

Conversation

armandfardeau commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreslucena commented Jun 25, 2020

Uh oh!

tramuntanal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armandfardeau commented Jul 17, 2020

Uh oh!

stale bot commented Sep 20, 2020

Uh oh!

armandfardeau commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrcasals commented Sep 21, 2020

Uh oh!

armandfardeau commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivan-mr left a comment

Choose a reason for hiding this comment

Uh oh!

ivan-mr commented Sep 23, 2020

Uh oh!

armandfardeau commented Sep 24, 2020

Uh oh!

ivan-mr commented Sep 25, 2020

Uh oh!

mrcasals commented Sep 28, 2020

Uh oh!

armandfardeau commented Oct 1, 2020

Uh oh!

armandfardeau commented Oct 13, 2020

Uh oh!

ivan-mr commented Oct 13, 2020

Uh oh!

ivan-mr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

armandfardeau commented Jun 23, 2020 •

edited

Loading

armandfardeau commented Sep 21, 2020 •

edited

Loading

armandfardeau commented Sep 22, 2020 •

edited

Loading