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

perf: speed up post creation time #3808

Merged
merged 6 commits into from
Apr 30, 2023
Merged

perf: speed up post creation time #3808

merged 6 commits into from
Apr 30, 2023

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Apr 27, 2023

Changes proposed in this pull request:
This PR brings performance improvements for the post creation endpoint.

  • Visibility checking on the notification syncer (which is expensive) is dropped as it didn't actually fix what it was introduced for and was replaced in the past with visibility checking on the specific notification jobs. (ce6cb58).
  • When post has a large amount of mentions, it retrieves each mention individually, this PR eager loads mentions once and uses the loaded collection instead (df4d3ca).
  • For visibility checking on mentions notifications, this PR also adds some needed eager loads (8aaa330).
  • Right now, with group mentions, user mentions, and post mentions (without taking into account extensions), the endpoint has to do a lot of visibility checking, which is an expensive operation when applied to many users. This PR also moves the logic that triggers mention notification to a queueable job (6e1c22a).

Screenshot
Screenshot from 2023-04-27 14-43-45

Screenshot from 2023-04-27 14-43-50

Screenshot from 2023-04-27 16-11-58

Screenshot from 2023-04-27 16-44-53

Screenshot from 2023-04-27 17-47-14

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

SychO9 and others added 6 commits April 27, 2023 14:35
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 requested a review from a team as a code owner April 27, 2023 17:22
@imorland imorland added this to the 1.8 milestone Apr 27, 2023
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Haven't tested locally, but code changes look good.

protected $post;

/**
* @var array
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can we annotate array<what>?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants