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

Search performance regression #1764

Open
tobyzerner opened this issue Mar 24, 2019 · 5 comments

Comments

@tobyzerner
Copy link
Member

commented Mar 24, 2019

#1741 introduced a regression in search performance which needs to be looked at.

@luceos

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

For my information, how did you discover this?

@tobyzerner

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

I had a feeling there was a reason I didn't use left join when I wrote the search code, and I am aware of performance differences between left and normal join. So that tipped me off that I should performance-test this change.

To do so, I just ran the search SQL for the keyword "beta" with and without left against the discuss.flarum.org database. The effect is big enough that you can see it directly on discuss.flarum.org (this page takes way too long to load)

@luceos

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Would it make sense to add a test that:

  • creates a predefined set of discussions with a specific keyword
  • measures response time of a search onto that keyword
  • reports in when the response time is out of bounds

/cc @franzliedke

The reason I ask is that we want to prevent to have detrimental performance consequences of PR's that look okayish at first.

@luceos luceos added the critical label May 8, 2019

@luceos luceos added this to the 0.1.0-beta.10 milestone May 8, 2019

@luceos

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Prioritised this, because searching anything on discuss is a huge dealbreaker 😂

@vingrad

This comment has been minimized.

Copy link

commented Aug 18, 2019

I think, that it would be make sense to do elasticsearch integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.