Rearrange conditions to short-circuit before database queries #11197
Conversation
changelog: Internal, Performance, Optimize code conditions to avoid unnecessary database queries
There was a problem hiding this comment.
👏 Nice. I think it's easy to get bogged down in changing logic and not consider the impact of database queries.
A couple thoughts...
- Have we looked into using bullet to try to find N+1 queries and such? Not directly related, just on the topic of "I didn't realize we were doing that bad database thing..."
- What about building out a little style guide / best practices doc as we go? This might be a neat thing to note. We had this when I worked on Caseflow, for example.
Both are just ideas for potential followups.
Not sure what you mean by "disabled by default", but it's caught issues for me in the past (recent example). |
I like this idea, though I could also see a counter-argument that these sorts of optimizations aren't really specific to our project, and more just best practices for any programming language that has boolean short-circuiting. We could maybe have a "Optimizations" section in the "Back-end Architecture" doc, or maybe a separate performance-specific document? |
D'oh! I took a look at the Gemfile but didn't make it down to the
It is definitely not unique to the project. My thinking was to just document best practices as a reminder. That said...
I consistently forget this document exists. 😬 So I'm realizing that my "we should document this as a best practice!" thought might really mean "we should put this in a document that I rarely look at," and maybe that's not worthwhile. |
True, though I think it can still be a nice thing to point to as a reference when reviewing a pull request if the reviewer is aware of the document. For example, I frequently point people to documentation they may not know exists, which hopefully indirectly increases awareness of said documentation. I think discoverability of this document is a problem in its own right, but I'm not really sure how much more we can highlight beyond what we've done to make it one of the few top links in the top-level README 🤷 |
🛠 Summary of changes
Rearranges a handful of
&&conditions to move trivial-cost checks to earlier in the condition, leveraging boolean short-circuiting to avoid more costly checks.These were discovered by searching
&&inapp/**/*.rbfiles and manually reviewing each instance.📜 Testing Plan
Verify build passes.
It's not expected this should have a meaningful impact, but there may be scenarios where a condition was already written with short-circuiting in mind, where the latter part of a condition relies on a dependency established in an earlier part. Those have been considered as part of these changes, but a secondary review is appreciated.