Skip to content

Conversation

@fatelei
Copy link
Contributor

@fatelei fatelei commented Dec 9, 2025

Important

  1. Make sure you have read our contribution guidelines
  2. Ensure there is an associated issue and you have been assigned to it
  3. Use the correct syntax to link this PR: Fixes #<issue number>.

Summary

fix #29304
fix #27299

Before After Improvement
6 separate sessions 1 shared session 84% reduction
6x connection overhead 1x connection overhead 84% less overhead
6 transaction setups 1 transaction setup 84% faster startup

Screenshots

Before After
... ...

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

@fatelei fatelei requested a review from QuantumGhost as a code owner December 9, 2025 02:20
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 9, 2025
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @fatelei, 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 the get_configurations function by refactoring its database interaction logic. Instead of initiating multiple separate database sessions for various data fetches, it now leverages a single, shared session. This change drastically reduces the overhead associated with database connections and transaction management, leading to faster execution and more efficient resource usage.

Highlights

  • Performance Optimization: The get_configurations function has been optimized to reduce database session overhead, resulting in a 66% reduction in sessions, connection overhead, and transaction setups.
  • Shared Database Session: The previous implementation used three separate database sessions for different data retrievals; now, a single shared session is utilized across all related database operations within get_configurations.
  • Refactored Helper Methods: The internal helper methods _get_all_providers, _get_all_provider_models, and _get_all_preferred_model_providers now accept a database session as an argument, removing their responsibility for session creation and management.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to optimize the get_configurations method by sharing a single database session across multiple helper methods, reducing connection overhead. The changes correctly refactor _get_all_providers, _get_all_provider_models, and _get_all_preferred_model_providers to use a shared session. However, the optimization is incomplete as other helper methods within get_configurations still create their own sessions. I've added a comment to suggest completing the refactoring for a more significant performance gain.

@fatelei fatelei force-pushed the perf_get_configurations branch from efe0956 to 01147bd Compare December 9, 2025 02:26
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Dec 9, 2025
@fatelei fatelei force-pushed the perf_get_configurations branch 2 times, most recently from 7f6a4f0 to c854e22 Compare December 9, 2025 02:39
@crazywoola crazywoola requested a review from hjlarry December 15, 2025 03:27
@hjlarry
Copy link
Contributor

hjlarry commented Dec 16, 2025

/gemini Here's another AI's suggestion. What's your suggestion?

Using a single db.session inside get_configurations and passing it into _get_all_provider_models / _get_all_preferred_model_providers would not be an improvement here.

  • In api/core/provider_manager.py (lines 406-517) every helper deliberately opens its own Session(db.engine, expire_on_commit=False) inside a with block, so each query runs in an isolated, short‑lived transaction that closes immediately. Sharing one long‑lived session from get_configurations would keep that transaction open across the dozens of queries that follow, increasing the chance of holding onto stale state or locking rows longer than necessary.
  • db.session in this project is Flask‑SQLAlchemy’s scoped session, which is only guaranteed to exist inside an application/request context. ProviderManager.get_configurations is used by background jobs and CLI entry points (see the class being instantiated directly in management code), so switching to db.session risks runtime errors whenever no Flask context is present.
  • The current pattern also makes the helpers trivially re‑usable; requiring a caller‑supplied session would force every caller to manage session lifecycle correctly—which is error‑prone given the number of call sites.

If connection overhead ever becomes an actual bottleneck, a safer refactor would be to keep using explicit Session objects but open one locally with a with Session(...) as session: block inside get_configurations and pass that session down, ensuring you still control the lifecycle explicitly. However, swapping to the global db.session is not advisable under the current architecture.

@gemini-code-assist
Copy link
Contributor

Thank you, @hjlarry, for this incredibly insightful and detailed feedback. Your points are very well-taken and highlight critical architectural considerations that my initial summary overlooked.

I agree with your assessment on all three points:

  1. Transaction Management: You're absolutely right that using a single, long-lived db.session across multiple queries could indeed lead to issues with stale state or prolonged row locks, especially in a complex application. The original design with isolated, short-lived transactions in each helper method is a robust pattern for ensuring data consistency and minimizing contention.
  2. Flask Context: The dependency on Flask-SQLAlchemy's scoped session (db.session) is a significant concern for functions like get_configurations that are utilized by background jobs and CLI entry points. Introducing a Flask context dependency where none existed before would definitely lead to runtime errors in those environments.
  3. Reusability: Forcing every caller of the helper methods to manage the session lifecycle would indeed make them less reusable and more prone to errors, undermining the modularity of the codebase.

My initial focus was purely on reducing the overhead of session creation, but your explanation clearly demonstrates that the current implementation's benefits regarding transaction isolation, environment compatibility, and helper reusability are paramount. The potential performance gain from a shared db.session is not worth the architectural compromises and risks you've outlined.

Your suggestion of explicitly opening a Session(db.engine, expire_on_commit=False) within get_configurations and passing that down to the helper methods is a much safer and more appropriate refactor. This approach would allow for the desired reduction in session overhead while maintaining explicit control over the session's lifecycle and avoiding the pitfalls of db.session in non-Flask contexts.

I appreciate you catching these crucial details. This feedback is invaluable for ensuring the stability and correctness of the system. I will adjust my review and recommendations accordingly.

@fatelei fatelei force-pushed the perf_get_configurations branch from 763283c to 103ee19 Compare December 18, 2025 08:31
@fatelei fatelei force-pushed the perf_get_configurations branch from 797b18e to 2bc796c Compare December 18, 2025 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Chore/Refactor] Workflow database performance optimization perf: optimize ProviderManager.get_configurations

3 participants