-
Notifications
You must be signed in to change notification settings - Fork 0
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: memoization improvements #81
Conversation
… value removal strategies
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a draft, as there are some open questions
…tion-improvements
…nction, as talked about in the PR
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 11 14 +3
Lines 530 721 +191
==========================================
+ Hits 530 721 +191 ☔ View full report in Codecov by Sentry. |
…tion-improvements
fix: apply wrapping / unwrapping recursively fix: comparison of ExplicitIdentityWrapper against other ExplicitIdentityWrapper with different identity, but same value test: memoization utils
…emoization-improvements # Conflicts: # src/safeds_runner/server/_memoization_strategies.py
…tion-improvements
@WinPlay02 Is this ready for review or are you still working on it? |
Some quick testing shows some major performance improvements compared to After clicking on "Explore Table", there's always still roughly a 3s delay between the That initial delay might be related to the startup delay we observed for the library, since each worker process has to load the library? |
Yes, that should be the Startup delay. That's now the hardest problem to solve. |
It was still in draft, as the strategies now use functions instead of lambdas (passing to the pipeline process lead to a I also tried to use a process pool, as discussed on Thursday, but I didn't find a satisfying solution on how it should be handled, if the max. amount of processes has been reached. (Waiting in a queue, vs sending an Error) Using any pool (library) doesn't map to this pipeline based usage model very well, as they are made with parallelism or concurrency in mind. Apart from these two things, it should be reviewable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, great work!
🎉 This PR is included in version 0.11.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary of Changes
Closes #44
Depends on Safe-DS/Library#609