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

Feat/persistent term settings #44

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

leonardb
Copy link
Contributor

Move the config/settings into persistent terms rather than use application env.

Purely for performance as application:get_env requires an ETS lookup where persistent_term is highly optimized for reads

@leonardb leonardb marked this pull request as draft August 17, 2023 13:39
@leonardb
Copy link
Contributor Author

@choptastic Would you be ok with my moving the ets/process dictionary settings to persistent terms?

Persistent term performance should be close to process dictionary and would allow for quite a bit of simplification of how settings are handled within qdate.

@choptastic
Copy link
Owner

This is actually very interesting. I totally missed the introduction of the persistent_term module. Very nice!

@choptastic
Copy link
Owner

BTW, I see you marked this PR as a draft. I just want to check to make sure: is there anything else you'd like to add or change before I merge it in?

@leonardb
Copy link
Contributor Author

@choptastic I marked as a draft since if you were ok with me converting the existing qdate_srv ets/p-dict to persistent terms I'd rework things to move all these changes into that module

@choptastic
Copy link
Owner

Ah yes, that makes sense. Yes, I like the plan of replacing all the ets calls with the persistent_terms. But the stuff that is actually using the process dictionary, that needs to remain as-is. The purpose for using the pdict for those functions is not performance, but for those specific settings to persist throughout that process.

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

Successfully merging this pull request may close these issues.

2 participants