Improve performance of queries on roms table#2811
Conversation
Summary of ChangesHello @MaienM, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the performance of database queries on the 'roms' table, especially for paginated results with large offsets. The core issue was identified as inefficient subqueries related to platform details being loaded for each ROM. By removing a specific 'joinedload' operation that proved to be redundant and detrimental to performance, the PR aims to resolve these bottlenecks, making API requests much faster without apparent loss of functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Excellent work on diagnosing this tricky performance issue! Your analysis is spot on. The subqueries on the Platform model's column_property fields combined with joinedload were indeed causing significant slowdowns on large datasets.
My review focuses on making the fix more robust. While removing the joinedload works in your tests, it relies on the rom.platform attribute not being accessed, which could be brittle. I've suggested changing joinedload to selectinload instead. This is a standard SQLAlchemy pattern for this exact problem. It will load the platform data efficiently in a separate query, fixing the performance issue while ensuring the data is available if needed by any part of the application.
You mentioned that when you tried selectinload, you didn't see the platform table in your query logs. This might be because selectinload issues a second, separate SELECT statement after the main query, which your logging setup might not be configured to display.
Overall, this is a great performance improvement.
I am having trouble creating individual review comments. Click here to see my feedback.
backend/handler/database/roms_handler.py (108-109)
You've correctly identified that joinedload(Rom.platform) is causing a performance issue due to the subqueries on the Platform model.
While removing it works in your tests, a more robust solution is to change the loading strategy to selectinload. This is the idiomatic SQLAlchemy fix for this type of N+1 subquery problem. It will load the platform data in a separate, efficient query, resolving the performance bottleneck while ensuring the rom.platform data is available if needed elsewhere.
You mentioned trying selectinload and it working well, which is great! I'd recommend committing that change instead of removing the load option entirely for better long-term maintainability.
selectinload(Rom.platform),
backend/handler/database/roms_handler.py (140-141)
For the same reasons as in the with_details decorator, switching to selectinload here is the recommended approach. It fixes the performance issue while safely pre-loading the platform data.
selectinload(Rom.platform),
The old variant added subqueries that query the entire rom table which were evaluated for each rom, absolutely tanking the performance on larger collections.
MaienM
force-pushed
the
feature/improve-roms-query-performance
branch
from
477ced7 to
dfd8840
Compare
|
I did not realize Still, I agree that including this is probably better for long-term maintainability, so I've re-added this join with |
|
@MaienM great analysis and solution dude, thank you so much for the contribution! |
3 participants
Description
The queries on the roms become painfully slow when attempting to query later pages in large databases. As an example my database contains ~25k roms, and getting the first 500 roms (
api/roms?limit=500&offset=0) is pretty quick, but getting a similar chunk towards the end (api/roms?limit=500&offset=20000) takes several minutes.I use a PostgreSQL database, so that is what the queries below will be for, but the same problem may also occur on other database engines.
Digging into the generated queries the query that's causing the issues looks something like this:
Running two query plans with offset 0 & 10000 shows the drastic increase in cost as the offset increases:
The plans didn't really make it clear to me where this difference was coming from though. I spent some time removing parts of the query and running plans on it to track this down, and found that the portion of the query that causes all the pain are the subqueries for
anon_1andanon_2, which correspond to therom_countandfs_size_bytesfields ofPlatform. Changing these properties to instead use a query that always returns 0 verified that these subqueries were indeed the cause, making the API request that previously took minutes take less than a second (as it should).It makes sense that these subqueries are an expensive part of the query as they require reading the entire roms table, meaning that this has a complexity of
O(n^2). I'm somewhat surprised this is worse for larger offsets though since evaluating these queries doesn't seem necessary to determine the set of roms to operate on. Either way it doesn't really make sense to calculate this for each rom since it's a property of the platform, not the rom. A much more efficient way to do this would be to join on a subquery in which these fields are calculated for each platform, like so:Running two query plans for this altered query (using the same offsets as before) confirms that this is much more efficient & scales much better:
Of course, we're not going to be changing the queries directly, but this does confirm that the thing we need to change to resolve this issue is how the
platformstable is joined on in this query.My first attempt at this was to change this join from
joinedloadtoselectinload. This didn't result in the exact same changes to the query as in the manually-improved version — in fact, the platform table no longer even appeared in any of the queries I logged — but it did result in the desired performance improvement. Perplexed by this I ran the tests and found they all passed. I also did some manual checking with the frontend, and did not encounter any issues there either.At this point I assumed this join was only added when needed & I just hadn't logged any of those queries. To confirm this I removed it altogether and… got the same results.
That leads to this PR, which
removes the offending joins. Everything I've tested suggest they are not needed, but I'm curious to hear from someone more knowledgeable about this project whether this is actually true or whether I've just missed a case where they are required.EDIT: Switched to using
selectinloadinstead of removing, see later comments.Checklist
Please check all that apply.