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

Concurrent recursive function memoization #2096

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@smillies
Copy link

smillies commented Sep 17, 2017

Hi there, I've gone and created a pull request. Changes compared to the original proposal: MemoizedConcurrently has an additional method that accepts a cache implementation, and ConcurrentTrampoliningMemoizer is hidden (package private).
Regards,
Sebastian

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 17, 2017

Codecov Report

Merging #2096 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2096      +/-   ##
============================================
- Coverage     97.49%   97.33%   -0.17%     
  Complexity     5188     5188              
============================================
  Files            92       94       +2     
  Lines         11928    11948      +20     
  Branches       1575     1577       +2     
============================================
  Hits          11629    11629              
- Misses          149      169      +20     
  Partials        150      150
Impacted Files Coverage Δ Complexity Δ
.../java/io/vavr/concurrent/MemoizedConcurrently.java 0% <0%> (ø) 0 <0> (?)
...avr/concurrent/ConcurrentTrampoliningMemoizer.java 0% <0%> (ø) 0 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22ac105...2a3ed44. Read the comment docs.

@danieldietrich

This comment has been minimized.

Copy link
Member

danieldietrich commented Sep 17, 2017

Thank you, I will review it tomorrow!

@danieldietrich danieldietrich added this to the vavr-1.0.0 milestone Sep 19, 2017

@smillies

This comment has been minimized.

Copy link

smillies commented Sep 27, 2017

I will close the pull request from GitHub, because it does not compile anymore with the demise of Promise in #2093

@smillies smillies closed this Sep 27, 2017

@danieldietrich

This comment has been minimized.

Copy link
Member

danieldietrich commented Sep 27, 2017

Mmhh, sorry. I think it is still possible. The new Future.join is a functional substitute for the imperative Promise. I will take a look!

Sorry for being so slow. There is so much to do - it will take some days until I'm able to check all PRs. Our blog will be deleted on Sep 30th because RedHat stops OpenShift v2. I'm currently creating a new blog on clever-cloud and migrating all the data. In parallel we urgently need a v0.9.2 because some nasty bugs where reported. Also we need to support JDK9. With four kids it is sometimes hard to find enough time beside the daylight job. Sorry for letting you down.

One thing about the PR - I need to check how we could integrate the feature with Vavr. I do not want to create new (public visible) types, our goal is to keep the core lib as thin as possible. We will see...

@smillies

This comment has been minimized.

Copy link

smillies commented Sep 27, 2017

Don't worry, I'm not feeling let down at all. I appreciate all the hard work you're putting into this library. (Four kids - happy you. We were four at home as well.)

@danieldietrich

This comment has been minimized.

Copy link
Member

danieldietrich commented Sep 27, 2017

Thx for the kind words

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