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

perf: cache getConfig #9377

Merged
merged 2 commits into from
May 10, 2024
Merged

perf: cache getConfig #9377

merged 2 commits into from
May 10, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented May 10, 2024

Description

When not using a config file, we currently fetch the config on each call. This adds up to quite a few unnecessary queries since this method is called so frequently. Even when using a config file, we apply the same overrides and re-validate the same resulting object each time. This PR caches the config object, only updating it when a config update event is triggered.

How Has This Been Tested

Tested that jobs process as normal with and without a config file, that updating the config through the web UI updates the config object, and that multiple concurrent calls to getConfig result in only one query.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Did you test changing config at runtime and verifying the microservices uses the new config? Specifically for stuff like storage template migration and concurrency?

@mertalev
Copy link
Contributor Author

Yep. I ran thumbnail generation and changed the preview format midway. The success logs immediately started using the new format.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM :)

server/src/interfaces/database.interface.ts Outdated Show resolved Hide resolved
Co-authored-by: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com>
Copy link

cloudflare-workers-and-pages bot commented May 10, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: f68d5bc
Status: ✅  Deploy successful!
Preview URL: https://0671f7ec.immich.pages.dev
Branch Preview URL: https://perf-cache-get-config.immich.pages.dev

View logs

@mertalev mertalev enabled auto-merge (squash) May 10, 2024 18:11
@mertalev mertalev merged commit f3fbb9b into main May 10, 2024
23 checks passed
@mertalev mertalev deleted the perf/cache-get-config branch May 10, 2024 18:15
@jrasm91
Copy link
Contributor

jrasm91 commented May 10, 2024

Yep. I ran thumbnail generation and changed the preview format midway. The success logs immediately started using the new format.

Those don't use config$ though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants