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

Add a max size for normalize cache #2878

Merged
merged 2 commits into from
Sep 29, 2023
Merged

Add a max size for normalize cache #2878

merged 2 commits into from
Sep 29, 2023

Conversation

wb14123
Copy link
Contributor

@wb14123 wb14123 commented Sep 27, 2023

Fixes #2484

Problem

There is no bound for normalize cache which results memory usage increase without limit for dynamic queries.

Solution

Added a capacity to normalize cache. Default to 1024 and can be override by quill.query.cacheDynamicMaxSize.

Notes

Added a new dependency Google Guava to use its cache implementation. I know it's better to avoid new dependency but Google Guava is a pretty widely adopted library. It's cache implementation is widely tested. For such a critical use case in quill, I think it's better to have a widely tested implementation than reinventing the wheel.

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

Note:

sbt scalariformFormat test:scalariformFormat give me error:

[error] Not a valid command: scalariformFormat                                                                                                                                       
[error] Not a valid project ID: scalariformFormat                                         
[error] Expected ':'                                                                                                                                                                 
[error] Not a valid key: scalariformFormat (similar: scalaArtifacts, scalafmt, scalafmtDoFormatOnCompile)                                                                            
[error] scalariformFormat                                                                                                                                                            
[error]     

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2023

CLA assistant check
All committers have signed the CLA.

@wb14123 wb14123 force-pushed the cache-fix branch 3 times, most recently from f8da003 to 19412f0 Compare September 27, 2023 21:39
@wb14123
Copy link
Contributor Author

wb14123 commented Sep 27, 2023

Pushed a new commit to make the default cache size 1024. However from my profiling, each AST seems to have pretty different sizes. I'm wondering if there is a way to get memory footprint from an AST efficiently so that we can use CacheBuilder.weigher(Weigher) to make the cache size smarter

@wb14123
Copy link
Contributor Author

wb14123 commented Sep 28, 2023

The GC looks much normal after this fix as well:

image

build.sbt Outdated Show resolved Hide resolved
Copy link
Member

@guizmaii guizmaii left a comment

Choose a reason for hiding this comment

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

@wb14123
Copy link
Contributor Author

wb14123 commented Sep 29, 2023

Oh seems f.asJava doesn't compile against Scala 2.12. Will push a fix. Sorry for rushing it ...

@guizmaii
Copy link
Member

Oh seems f.asJava doesn't compile against Scala 2.12

We're sorry for you and we're sorry to have to maintain Scala 2.12 compat 😄
Thanks for your work 🙏

@guizmaii
Copy link
Member

guizmaii commented Sep 29, 2023

Latest version of Caffeine doesn't support Java8 anymore apparently:

[error] java.lang.UnsupportedClassVersionError: com/github/benmanes/caffeine/cache/Caffeine has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0

The good news is that we're about to drop support of JDK8 too!

FYI: #2878

This fixes zio#2484. Added a capacity to normalize cache. Default to 1024
and can be override by `quill.query.cacheDynamicMaxSize`.

Added a new dependency Caffeine to use its cache implementation, which
is recommended by Guava. For a critical use case in quill, I think
it's better to have a widely tested implementation than reinventing the wheel.
@wb14123
Copy link
Contributor Author

wb14123 commented Sep 29, 2023

@guizmaii oh I didn't realize you are working on dropping Java 8. I added a check for Java version so we can use it without breaking Java 8

@guizmaii
Copy link
Member

oh I didn't realize you are working on dropping Java 8

The ZIO environment in its entirety decided to remove Java8 support (triggered by some needs in zio-quill)
Your work is just the motivation I needed to make this change 🙂

@guizmaii guizmaii merged commit 5402d84 into zio:master Sep 29, 2023
4 of 12 checks passed
@guizmaii
Copy link
Member

Thanks again for your work @wb14123! ❤️

jilen pushed a commit that referenced this pull request Jun 11, 2024
This fixes #2484. Added a capacity to normalize cache. Default to 1024
and can be override by `quill.query.cacheDynamicMaxSize`.

Added a new dependency Caffeine to use its cache implementation, which
is recommended by Guava. For a critical use case in quill, I think
it's better to have a widely tested implementation than reinventing the wheel.

Co-authored-by: Jules Ivanic <[email protected]>
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.

NormalizeCaching cache grows without bound
3 participants