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

Fix cache warnings #338

Merged
merged 2 commits into from
Dec 24, 2021
Merged

Conversation

adrien-berchet
Copy link
Member

No description provided.

@adrien-berchet
Copy link
Member Author

Hi @zzzeek
Sorry I need you again :)
I am fixing the new warnings for the cache issues but I am not sure in which case we should use the cache or not and I don't have much time to investigate this in details... In this PR I assumed that only the objects whose internal state does not change the SQL string can use the cache (which is unfortunately not always the case for some classes provided by GeoAlchemy2), but I am not sure.
I disabled the cache for the types and enabled it for the functions (enabling it for the types break a test for an unknown reason so I disabled it).
Could you take a quick look on this please?

Copy link
Contributor

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

heya

if you dont have time to test these constructs very carefully in a caching scenario, I would advise setting all the inherit_cache to False for now; most of them look not ready for caching yet in any case as they contain state that seems likely to affect SQL generation or return types.

ideally for caching you'd have some SQL statements that are run many times, given a sample of the construct where you change it on each run, and ensure the correct SQL / parameters are emitted.

geoalchemy2/elements.py Show resolved Hide resolved
geoalchemy2/functions.py Show resolved Hide resolved
geoalchemy2/functions.py Show resolved Hide resolved
tests/gallery/test_summarystatsagg.py Show resolved Hide resolved
geoalchemy2/types.py Show resolved Hide resolved
geoalchemy2/types.py Show resolved Hide resolved
@adrien-berchet
Copy link
Member Author

Thank you very much for these insights @zzzeek !
I am still not sure to understand how the cache keys are constructed so it was hard to choose which objects should use the cache or not. For now I am going to disable the cache when I am not sure and we'll see if it causes perf issues.
Thanks again!

@zzzeek
Copy link
Contributor

zzzeek commented Dec 18, 2021

Thank you very much for these insights @zzzeek ! I am still not sure to understand how the cache keys are constructed so it was hard to choose which objects should use the cache or not. For now I am going to disable the cache when I am not sure and we'll see if it causes perf issues. Thanks again!

that's fine, you wont see perf issues compared to 1.3.x except in the area where applications use a lot of ORM lazy loading and your special functions happen to be in those queries, those will not be cached whereas in 1.3 they were.

re: the caching I would say there's no need to rush turning it on, when you have time I can try to give you more background on what exactly it does. I've worked with these caching approaches for years now but it should be expected they can easily seem opaque at first.

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.

Performance warning with SQLAlchemy 1.4.28
2 participants