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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Super small queries optimizations #1586

Merged
merged 2 commits into from Jan 19, 2019
Merged

Super small queries optimizations #1586

merged 2 commits into from Jan 19, 2019

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Jan 18, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

I applied two micro optimizations, more in the spirit of following good practices than actual overall performance improvements.

  1. replaced find_by().nil? with exists? because in those situations the object loaded by find_by is never used, thus is more efficient in general to just ask the DB for what you actually want (in this case to reply to the question asked by the nil? predicate)

  2. replaced any? with present? to populate the list of users of an organization. The reasoning is the following: most organizations will most likely exist with users than without, so we use present? which preloads the list of users used by the subsequent iteration. Hence 1 query instead of 2.

Related Tickets & Documents

This is most definitely the result of me digging through the code after reading 3 ActiveRecord Mistakes That Slow Down Rails Apps: Count, Where and Present.

I'm well aware that these two optimizations don't amount to much at the end of the day, it's more an attempt to hopefully cement good practices when they make sense.

At a first glance I didn't find anything else (though I didn't feel comfortable enough to delve in possible where() optimizations TBH, especially without hard numbers in front of me), so kudos to everyone!

Well, I already knew dev.to was fast 馃敟

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

find_by asks the DB and the ORM to load the entire object in memory, then it is discarded just know if it exists or not. Exists will just return true or false
A request for any? followed by each in a most likely full collection results in two SQL queries. present? preloads the objects, so it results in one single connection because the objects are already in memory.
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 18, 2019
@Link2Twenty
Copy link
Contributor

Can present? not be replaced by exists? too?

@rhymes
Copy link
Contributor Author

rhymes commented Jan 18, 2019

Can present? not be replaced by exists? too?

It depends on the context but I didn't find any suspicious present? attached to ignored relations in the code. I might have missed something of course :)

@benhalpern benhalpern merged commit c989cbf into forem:master Jan 19, 2019
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Jan 19, 2019
@rhymes rhymes deleted the rhymes/micro-queries-optimizations branch January 20, 2019 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged PR: reviewed-approved bot applied label for PR's where reviewer approves changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants