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

Reduce ForumPost requests on category pages #4523

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

mkbeefcake
Copy link
Contributor

@mkbeefcake mkbeefcake commented Sep 4, 2023

integrated persist cache with local storage.
need to discuss more with @thesan about the point when can fetch 500 posts from server.

@vercel
Copy link

vercel bot commented Sep 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Oct 10, 2023 3:45pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Oct 10, 2023 3:45pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Oct 10, 2023 3:45pm

@mkbeefcake
Copy link
Contributor Author

Hello @thesan I have tried to use local storage as persist cache for Apollo client. and then updated the code to use fetchPolicy as 'cache-only' for posts on Forum page. But i hope to know when and where will be the best to fetch 500 posts from the server and hope to get your opinion.
Regards

@mkbeefcake
Copy link
Contributor Author

@thesan I have tried to reduce Graph queries by using memory cache by updating some calls. Also I integrated to fetch 500 latest posts per one day, but it looks like not good effect for this. Please review and give me feedback.

@chrlschwb
Copy link
Contributor

chrlschwb commented Sep 13, 2023

fixes #4484

@chrlschwb chrlschwb added community-dev issue suitable for community-dev pipeline jsg-code-review labels Sep 13, 2023
Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Hey @mkbeefcake, sorry I wasn't checking my GH notifications lately so I completely missed your 3 messages.

Again the idea of relying on Apollo fetch policies is great but sadly I haven't had a very good experience with the Apollo cache either.

I detailed a solution closer to what I had in mind in the comments. It's not as good in theory because it assumes that enough of the currently showing threads and sub-categories have a post which is one of the 500 last updated ones else it shows a "-".

Finally I just thought of this but it would probably be safer to fetch the 500 last created post rather than updated.

packages/ui/src/forum/hooks/useForumLatestPosts.ts Outdated Show resolved Hide resolved
packages/ui/src/forum/hooks/useCategoryLatestPost.ts Outdated Show resolved Hide resolved
@@ -11,6 +11,7 @@ export const useForumCategoryThreadCount = (category_eq: string, isArchive?: boo
}
const { data } = useGetForumThreadsCountQuery({
variables: { where: { category: { id_eq: category_eq }, status_json } },
fetchPolicy: 'cache-first',
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Hey @mkbeefcake, thanks to you I finally got to test this idea. Here's the result on the "Joystream General" category:

  1. It works fine for the sub categories 4 out 4 here.
  2. It doesn't works well on the threads 5 out of 30.

This result is consistent on most categories. Sometime a few sub categories are missing their latest post but overall most sub categories have a latest post. However most thread latest posts are missing - except for the very active categories like "Pre-Proposals" which has a 30 out of 30 on the first page !

The solution could probably be tweaked a bit e.g:

  • Increasing the amount cached posts from 500 to something a bit bigger.
  • Decreasing the amount cached posts but filtering by category instead (for the threads last activity).
  • Relying on the Apollo cache instead (like the way you started this PR) so that the missing still posts still get queried.
  • Or a combination of these ☝️

However when I proposed this caching solution the query node were in a really bad shape which resulted in Pioneer being unusable for most people, so I felt that we needed to act fast even if it meant sacrificing some of the functionalities in the short term. Back then maybe this solution would have been acceptable but IMO now it's not. As a result, I don't think we should be spending time trying to tweak this short term solution now that the problem is not urgent anymore.

Instead I think we should focus on the long term solution which is Joystream/joystream#2599

What do you think @mkbeefcake ?

@chrlschwb
Copy link
Contributor

@thesan can we complete this PR and merge it as a solution to the original issue?
#2599 requires a query node modification which might cost a lot of dev time and most likely this can only be attempted much later due to a large workload

@thesan
Copy link
Member

thesan commented Oct 10, 2023

@thesan can we complete this PR and merge it as a solution to the original issue? #2599 requires a query node modification which might cost a lot of dev time and most likely this can only be attempted much later due to a large workload

@chrlschwb even if this solution was viable this PR would need a bit more work, also #3868 would need to be finalized first (but I think this part should be done anyway). But even once its implementation is complete it turns out that the original idea I proposed isn't very good (see the screenshot of the "Joystream General" category ☝️). And since the QN are now working I don't think it's worth spending a lot of effort improving it because I think it will never be great, it will always be a short term trade-off until the QN issue can be addressed.

However this change could be merged and maybe it could be applied to useCategoryLatestPost, useForumLatestPosts, useThreadLatestPost, and useForumPopularThreads (after all other changes on these files are reversed).

cc @mkbeefcake

@thesan thesan merged commit 1b8a4b7 into Joystream:dev Oct 11, 2023
3 checks passed
@ivanturlakov
Copy link

ivanturlakov commented Oct 12, 2023

Tested on https://pioneer-2.vercel.app/#/forum/category/2
Mainnet

The number of requests when the page first loaded remained the same, but with continued interaction with the forum, the number of requests decreased significantly. Data display is OK 👍

pioneerapp.xyz pioneer-2.vercel.app
First load ~50 requests ~50 requests
2 +50 +10
3 +50 +10
4 +50 +10
Total 200 80

akiowebstar added a commit to akiowebstar/pioneer-goldwolf that referenced this pull request Oct 12, 2023
Reduce `ForumPost` requests on category pages (Joystream#4523)
@chrlschwb chrlschwb removed the qa-task label Oct 13, 2023
akiowebstar pushed a commit to akiowebstar/pioneer-goldwolf that referenced this pull request Oct 16, 2023
Fix 4484 : only using FetchPolicy at this moment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline qa-tested-ready-for-prod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants