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

Initial tag load performance improvement #87

Merged
merged 22 commits into from
May 4, 2021

Conversation

luceos
Copy link
Member

@luceos luceos commented Jun 29, 2020

Links flarum/framework#2177

Current situation

On hitting the Forum index, the ShowForumController gets its payload enriched from flarum/tags:

$event->data['tags'] = Tag::whereVisibleTo($event->actor)
->withStateFor($event->actor)
->with(['parent', 'lastPostedDiscussion'])
->get();

This code loads all tags and some of its relations. For small forums this isn't really an issue. But forums that have a ton of subtags this affects performance.

New situation

I've decided to exclude subtags, but include helper tags. The helper tags are limited to three, unless you click "more tags" which redirects you to the tags page. We could potentially improve this logic further by only including the 3 tags with the most discussions (withCount('discussions')), but that would need some optimalization as subqueries can't use limit in MySQL.

I am currently deploying this branch to the Bokt staging environment. Query count reduction in Bokt staging: 411 to 52. 163 tags.

  • Permission Scope js

@tankerkiller125
Copy link
Contributor

I'm assuming that on clicking a parent tag the child tags will get loaded in? (I haven't had time to test locally)

@dsevillamartin
Copy link
Member

This is definitely not ready for merge. When clicking on a tag, only those with existing discussions that load appear (because they are included in the /api/discussions response - we could improve performance times by not including already-loaded tags in API response, not sure if possible).

Sometimes a second child tag appears under another tag - not sure why that one is loaded 🤔.

@askvortsov1
Copy link
Member

Iirc, there isn't currently a call to the tags endpoint in the tags frontend (at least not on the tags page I think)? How are additional tags being loaded?

Also, I'm still confused why the tag states can't be loaded alongside the tags themselves in a single query. I'm in favor of not loading everything at once for reasons of scalability and reducing payload size, but I think the multiple queries thing is the bigger issue when it comes to performance. Does it still occur when no extensions other than core and tags are enabled?

@dsevillamartin
Copy link
Member

@askvortsov1 Additional tags are only loaded when a discussion has the tag.

@luceos
Copy link
Member Author

luceos commented Jun 29, 2020

Hm, right I didn't look far enough beyond the first page. Will look into that.

@luceos luceos marked this pull request as draft June 29, 2020 14:51
@askvortsov1
Copy link
Member

Also, if the page we're on IS the page for a parent tag (which is possible via custom homepages), perhaps we should load those too?

@dsevillamartin
Copy link
Member

@askvortsov1 Well it'd also be a parent tag if you directly visit /t/. I think this should also be extensible.

@luceos
Copy link
Member Author

luceos commented Jul 20, 2020

@datitisev what exactly do you mean with extensible? What should be extensible?

@dsevillamartin
Copy link
Member

@luceos 🤔 i don't know.

I assume deciding which tags get loaded, because of this comment I was replying to:

Also, if the page we're on IS the page for a parent tag (which is possible via custom homepages), perhaps we should load those too?

@luceos
Copy link
Member Author

luceos commented Jul 20, 2020

My pr adds a ShowTagController to load one tag through the api. The parent include (relation) already existed, I've added children in addition. There are a ton of other changes and I still need to tackle some minor things (like the permission grid tag selection). Other than that, it seems to already work much better than before.

luceos and others added 8 commits April 21, 2021 23:09
This code drops the sub tags so that forums with many tags wont load
the state unecessarily for each sub tag.

I've attempted to load only the 3 tags with most discusssions withCount(),
but the issue is that the subquery doesn't allow using a limit in MySQL.

We can further improve this logic, but that requires discussion.

Please note that specific tag pages aren't affected because they have
their own logic!
- admin page listing
- ajaxified sidenav
- tags page
- non-js tags page
@askvortsov1
Copy link
Member

TODO: Add tag loading logic for the composer and selection modal.

I feel like this could also use pagination, but I think that might be better in a separate PR after we get @datitisev's PaginatedList utils class merged.

@askvortsov1 askvortsov1 marked this pull request as ready for review April 22, 2021 22:06
@askvortsov1 askvortsov1 requested a review from SychO9 April 22, 2021 22:06
- Keep json_decoded api doc as object
- Load in siblings
@askvortsov1
Copy link
Member

Current setup:

~50 top level primary tags
~25 primary subtags for first 20 top level primary tags
~3000 secondary tags

Load differences:

  • On a regular discussion page: ~5.1s => 2.2s
  • On the discussion list: ~5s => 1.7s
  • On a top-level primary tag page (with 25 children): ~5s => ~2.2s
  • On a child tag page (with 25 siblings): ~5s => ~2.2s
  • On the "all tags" page: ~4.9s => 5.1s

This still feels way too slow, especially the regular discussion / discussion list pages (with no tags, they load in ~200ms).

Also, some logic issues are still present with load. Taking this back into draft mode.

@askvortsov1 askvortsov1 marked this pull request as draft April 23, 2021 04:24
@askvortsov1 askvortsov1 self-requested a review April 23, 2021 04:24
@askvortsov1 askvortsov1 marked this pull request as ready for review April 24, 2021 05:31
@askvortsov1
Copy link
Member

And back out of draft

src/Api/Controller/ListTagsController.php Show resolved Hide resolved
src/LoadForumTagsRelationship.php Outdated Show resolved Hide resolved
js/src/forum/states/TagListState.js Show resolved Hide resolved
@askvortsov1 askvortsov1 requested a review from SychO9 May 4, 2021 16:23
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice work!

@askvortsov1 askvortsov1 merged commit 39e4649 into master May 4, 2021
@luceos luceos deleted the dk/fix-initial-tag-load branch May 4, 2021 18:04
askvortsov1 added a commit that referenced this pull request Mar 11, 2022
- Only load lastPostedDiscussion on TagsPage
- For forum payload, only load top-level primary tags and top 3 secondary tags.
- In other cases, load tags in dynamically when needed.


Co-authored-by: Alexander Skvortsov <[email protected]>
Co-authored-by: Sami Mazouz <[email protected]>
askvortsov1 added a commit that referenced this pull request May 10, 2022
- Only load lastPostedDiscussion on TagsPage
- For forum payload, only load top-level primary tags and top 3 secondary tags.
- In other cases, load tags in dynamically when needed.


Co-authored-by: Alexander Skvortsov <[email protected]>
Co-authored-by: Sami Mazouz <[email protected]>
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.

5 participants