Attempt to make DeferredK rerunnable #1157

1Jajen1 · 2018-11-25T00:59:38Z

Solves #1156 and maybe #1131
Re-enable no-memoization law and fix deferredK to follow it (if used correctly)
Changes practically nothing from the public api.

I'll add some info to the docs later because if used wrong it might produce unexpected results.
Should the methods in DeferredK (well the public ones) have api docs (runnable ones) as well?

@pakoito I have skipped the invoke constructor for Wrapped because its syntax conflicts with the other one too much (the compiler was only able to infer which one to choose after passing more and more arguments which defeats the point of default argumens there), one should just use the other one or .k() instead. (And going from () -> Deferred<A> to suspend () -> A is trivial)

Re-enable no-memoization law and fix deferredK to follow it (mostly) Changes practically nothing from the public api

pakoito · 2018-11-25T01:59:49Z

modules/core/arrow-test/src/main/kotlin/arrow/test/laws/MonadDeferLaws.kt

@@ -146,25 +143,25 @@ object MonadDeferLaws {
    df.flatMap { df }.flatMap { df }.equalUnderTheLaw(just(3), EQ) shouldBe true
  }

-  fun <F> MonadDefer<F>.stackSafetyOverRepeatedLeftBinds(iterations: Int = 5000, EQ: Eq<Kind<F, Int>>): Unit {
+  fun <F> MonadDefer<F>.stackSafetyOverRepeatedLeftBinds(iterations: Int = 50000, EQ: Eq<Kind<F, Int>>): Unit {


5k is enough to trigger a SO, 50k makes them too slow maybe.

pakoito · 2018-11-25T02:03:43Z

modules/effects/arrow-effects-kotlinx-coroutines/src/main/kotlin/arrow/effects/DeferredK.kt

@@ -127,13 +165,13 @@ fun <A> DeferredKOf<A>.unsafeRunSync(): A =
  runBlocking { await() }

 fun <A> DeferredKOf<A>.runAsync(cb: (Either<Throwable, A>) -> DeferredKOf<Unit>): DeferredK<Unit> =
-  DeferredK.invoke(scope(), Dispatchers.Unconfined, CoroutineStart.DEFAULT) {
+  DeferredK.invoke(fix().scope(), Dispatchers.Unconfined, CoroutineStart.DEFAULT) {


Fix shouldn't be necessary here, scope() is defined as an alias to fix().scope atop of the file.

pakoito

LGTM, I like the approach of having two separate nodes. I need to recheck the name of the constructors to prevent exposing these nodes directly, and whether it allows @extension codegen work the way we want.

pakoito · 2018-11-25T02:14:48Z

modules/effects/arrow-effects-kotlinx-coroutines/src/main/kotlin/arrow/effects/DeferredK.kt


    fun <A> raiseError(t: Throwable): DeferredK<A> =
      failed(t)

+    fun <A> failed(t: Throwable): DeferredK<A> =


squint why is this here? why do we need an alias?

I know I wrote it, I cannot remember why.

Other than providing a more intuitive constructor for those that have never used raiseError I don't see the point either :) I'll remove it for now

raulraja

@ extension gen should work because for it to be considered a wrapper it just requires to extend the wrapped type which in this case it does.

raulraja

Awesome work! @ extension gen should work because for it to be considered a wrapper it just requires to extend the wrapped type which in this case it does. Before we merge this we need examples in the actual kdoc if required to distinguish about the instantiation strategies. All the markdown pages will be moved to kdocs and we don't want to keep making them bigger when they are referring to data typed or type classes. Text and examples should be in the kdoc unless it's a tutorial

1Jajen1 · 2018-11-25T13:17:20Z

It's not really required to distinguish between instantiation strategies (only for .k(), all others will work fine if a user does not use a deferred from outside that deferredK). So I think a section in the docs for DeferredK would be enough there. I can (and probably will in the next hour or so) add some kdoc to the api methods of DeferredK with examples. But except for .k() I don't think its worth mentioning retry-ability every time.

Edit: Nvm, I'll just add a note to the methods indicating if the result is rerunnable or not.

Added only to methods that may need it, mainly methods creating an instance of DeferredK since that may produce unexpected results as in memoization

1Jajen1 · 2018-11-25T15:28:50Z

I added kdoc to the methods constructing DeferredK instances with added notes about it being re-runnable or not. Should all other methods (even map/flatMap) use kdoc as well? If so I'l add that as well. Otherwise if the docs are alright this should be done. :)

pakoito · 2018-11-25T15:36:45Z

~~Before merging I would hide the internal nodes behind a constructor and mark them as internal data class, else it's an API leak the users don't care about.~~

EDIT: Oh, they're already hidden :D Could you please add one more override of invoke that works as .k(), maybe taking the scope too?

1Jajen1 · 2018-11-25T15:55:29Z

You mean marking Wrapped and Generated as Internal? They are already afaik. Changed them to data classes as well now. Do data classes override equals and stuff from their parent? o.O If so I may need to add equals/hashCode to those.

pakoito · 2018-11-25T16:12:51Z

Don't make them into data, you were right all along :D

This reverts commit 46501bc.

1Jajen1 · 2018-11-25T16:17:29Z

Good thing git makes it nice and easy to revert stuff :)

pakoito · 2018-11-25T17:09:35Z

Could you please add one more override of invoke that works as .k(), taking the scope too?

That and I'm good to merge :D

raulraja · 2018-11-25T17:16:52Z

@1Jajen1 yes ideally all methods are documented with at least one example and a brief description of what they do. We consider this extremely important and are trying to enforce it lately in all PRs to ensure new users have a good experience when visiting the Arrow documentation. Each one of your methods then will have a dedicated page like this one http://arrow-docs.s3-website-us-east-1.amazonaws.com/docs/apidocs/arrow-effects/arrow.effects.typeclasses/-bracket/bracket-case.html

1Jajen1 · 2018-11-25T17:31:19Z

@raulraja Yeah I saw those, built it locally. The jekyll command took a while but worked out fine o.O
Will add it then. 👍

@pakoito So like an invoke that takes a Deferred? As in a convenience method over defer { deferred.k() } I'll do that, but I don't think the deferred can be supplied by a function, as the type signature of that will be very similar to the other one, thus when invoking it'll be annoying the user that it does not know if it has a suspend () -> Deferred<A> or a () -> Deferred<A>.

pakoito · 2018-11-25T17:46:42Z

The function signature is operator fun <A> invoke(scope: CoroutineScope = GlobalScope, f: Deferred<A>): DeferredK<A>, with a value instead of a function.

1Jajen1 · 2018-11-25T22:37:58Z

Added a lot of kdoc to almost all methods of DeferredK and the invoke constructor.

Btw. can you give generics a different color, like orange or something, in the api docs? Would make it so much more readable :)

raulraja · 2018-11-25T22:45:40Z

@1Jajen1 yes, feel free to create an issue and assign it to @israelperezglez

1Jajen1 · 2018-11-25T23:26:01Z

Failed by reaching a time limit ... :/
The fact that the builds even take 50mins is wrong... Not sure what to change about it, I never had to deal with builds that long.

raulraja · 2018-11-25T23:31:35Z

It's the number of libraries. As we move past 1.0 most of them will be in their own repository. Effects, streams, recursion schemes etc... we'll all be able to evolve differently and then the build times will be much smaller. In the mean-time, if you find areas where the build times can improve feel free to raise issues.

1Jajen1 · 2018-11-25T23:40:16Z

I'll look into speeding up gradle somehow and what exactly takes how much time... Don't know if I find stuff to improve, but worth trying :)

1Jajen1 · 2018-11-26T13:30:43Z

So the build itself consistently takes about 20 mins (15-18 on my laptop) with almost all time spent on tests (kapt is also pretty slow, but its nothing compared to the tests).
Running dokka :arrow-docs:runAnk takes me about 2-3 mins, travis does not even get it done in 30 and my guess is that it's memory consumption. I frequently go up beyond 8-9 GB for that java task running it. (Which in turn slows down travis that does not have nearly as much memory available... Add another 10k files and let it run 2-3 hours and it'll probably throw a gc exception again)
So thats what needs to be dealt with. At that stage it's either dokka, ank or gradle. And considering memory consumption jumps up only when ank is processing it might still be it. I know it reports mem usage as ~1-2Gb all the time but something is obviously wrong :/

1Jajen1 · 2018-11-26T15:24:12Z

Welp, I am out of ideas (or better out of ways to properly test/debug this)...
The slowdown definitly comes from memory consumption and its likely not dokka. So that leaves gradle and ank. And again as it spikes when ank is running I'd blame it, except that I don't really see what is the cause exactly. Maybe I'm wrong and ank isn't at fault, I don't know tbh. I'll continue to investigate tomorrow when I have more time (and hopefully some new ideas)
@raulraja Even if split into multiple repos, unless every repo has its own docs :arrow-docs:runAnk will continue to be a problem if it takes that long, especially because large parts don't even have kdoc atm, it will only get worse. :/

raulraja · 2018-11-26T15:56:19Z

I fixed ank memory problems in master a while back, did you merge since then?

1Jajen1 · 2018-11-26T16:03:57Z

Yes. It reports memory usage to be arount 1.5GB.
The process that runs ank (not confirmed, just the only process actually doing something when ank prints output) however consumes a ton of memory, way more than printed out (usually going to 6-8GB).

Some ideas I had, but could not test yet:

The 1.5GB is inaccurate (which I don't think because it's read out correctly)
The script engine runs in a seperate process and consumes a ton of memory which the parent just does not include (just an idea, don't know how it works)
The ank plugin consumes a ton of memory. (Since this delegates to ank this seems unlikely)

1Jajen1 · 2018-11-26T16:32:16Z

I can confirm (using visualvm) the reported memory usage to be the actual heap memory usage, but also that the process running ank is actually consuming up to 4-5 times that, which means stuff not on the heap is leaking. Although I have no idea what causes that.

pakoito · 2018-11-26T22:39:36Z

modules/ank/arrow-ank/build.gradle

@@ -1,9 +1,10 @@
 apply plugin: 'kotlin'


Is this a refactor or a merge? O.O

Too fast again :)
Explained this below.

1Jajen1 · 2018-11-26T22:41:24Z

So I ended up not finding the reason why so much off-heap memory was used and resorted to rewriting ank to use java.nio instead of java.io (for the most part). I also changed and optimised a few things here and there. Most logic for processing input is still the same, just the way its invoked changed a bit.

It now consumes up to ~3GB memory at most. (Heap ~2GB, rest is probably memory mapped io)
It is also considerably faster (at least it feels faster, maybe that has to do with my laptop not freezing and putting stuff on swap to keep running o.O)

I ended up putting the changes on this branch to test it on travis as well, if this should be in a seperate pr please let me know and I'll revert that :) Either way please check if this code is correct or produces good ouput, I couldn't find differences from running ank on master but you never know 🙈

raulraja

The ank refactor is fine if it solves the memory issue for you but in whatever case we need to fix the nested Try signatures. You don't need those if you already have a constrain in MonadThrow<F>

raulraja · 2018-11-26T23:09:43Z

modules/ank/arrow-ank/src/main/kotlin/arrow/ank/algebra.kt


-fun getFileCandidates(target: File): List<File> =
-  getFileCandidatesImpl(target)
+  fun createTargetDirectory(source: Path, target: Path): Kind<F, Try<Path>>


There is no point on having concrete Kind<F, Try<Path> return type since they can just be Kind<F, Path> given you already have in scope a constrain on MonadThrow<F>. Why is the internal Try needed? You may just call MF().raiseError or use MF().catch { ... } if you just need to control synchronous error control. We can make the entire set of ops return IO<A> if you'd rather use an explicit concrete data type.

Yeah, I kinda forgot when defining the algebra and then did not change it when implementing ...
Will do in a sec. Just removing a bunch of them should be fine since MonadDefer.delay also catches errors.
I had the type as Kind<F, A> because I was starting with only Monad as the constraint on F. That would make testing with something like an id interpreter nice. Now with MonadThrow this may not be valid anymore. (Although Try might be a good fit there)

1Jajen1 · 2018-11-26T23:51:41Z

Changed the signatures. (They really were kinda dumb...)

At least the whole build gets done in like 35 mins now. That leaves about 15 mins leeway for new tests and more kdoc to process before something has to be done about builds again, and if the repo gets split before that it likely won't be a problem ever :)

The ank refactor is fine if it solves the memory issue for you but in whatever case we need to fix the nested Try signatures. You don't need those if you already have a constrain in MonadThrow

Did you never get memory issues running it? o.O Maybe its a os or even openjdk specific thing then.

raulraja · 2018-11-26T23:52:48Z

No issues here and the latest in master is gonna push those to s3 instead of github pages because jekyll times out in github

pakoito · 2018-11-27T01:59:10Z

Soooooo...is anyone pushing this while the builds aren't broken and master is stable? :D

1Jajen1 · 2018-11-27T09:29:38Z

This should be good to go.
There is a bug in generating diagrams right now. Typeclasses which don't inherit from any other typeclass don't have a diagram atm. This is happening on master as well and I don't really know how and where they are generated or if this even may be intentional. For now I think merging this and creating a ticket should be fine.

codecov · 2018-11-27T11:33:57Z

Codecov Report

Merging #1157 into master will increase coverage by 0.04%.
The diff coverage is 12.15%.

@@             Coverage Diff              @@
##             master    #1157      +/-   ##
============================================
+ Coverage     42.93%   42.98%   +0.04%     
- Complexity      836      841       +5     
============================================
  Files           404      403       -1     
  Lines         11428    11443      +15     
  Branches       1276     1317      +41     
============================================
+ Hits           4907     4919      +12     
+ Misses         6130     6120      -10     
- Partials        391      404      +13

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/kotlin/arrow/effects/DeferredKInstances.kt	`65.21% <0%> (-4.35%)`	`0 <0> (ø)`
...les/ank/arrow-ank/src/main/kotlin/arrow/ank/ank.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...arrow-ank/src/main/kotlin/arrow/ank/interpreter.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...es/ank/arrow-ank/src/main/kotlin/arrow/ank/main.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../src/main/kotlin/arrow/test/laws/MonadDeferLaws.kt	`69.81% <100%> (+2.08%)`	`30 <0> (+1)`	⬆️
...outines/src/main/kotlin/arrow/effects/DeferredK.kt	`53.93% <44.68%> (-3.43%)`	`10 <7> (+4)`

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 a0c51ea...6dbbdc1. Read the comment docs.

1Jajen1 added 2 commits November 25, 2018 01:37

Attempt to make DeferredK rerunnable

85431e2

Re-enable no-memoization law and fix deferredK to follow it (mostly) Changes practically nothing from the public api

Remove unused defer

7a2dc76

pakoito reviewed Nov 25, 2018

View reviewed changes

1Jajen1 added 3 commits November 25, 2018 10:27

Added some docs, change stack safety test range and reduce usage of fix

3c6b93b

Remove raiseError alias failed

c8032a4

Fix build ....

c57f8f9

raulraja approved these changes Nov 25, 2018

View reviewed changes

Add some kdoc to some methods

fde6406

Added only to methods that may need it, mainly methods creating an instance of DeferredK since that may produce unexpected results as in memoization

Make Wrapped and Generated data classes

46501bc

Revert "Make Wrapped and Generated data classes"

3238371

This reverts commit 46501bc.

Add lots of kdoc and another invoke constructor for Deferred values

55da6e1

Test different ank implementation based on nio insted of io

45f2f0f

pakoito reviewed Nov 26, 2018

View reviewed changes

Refractor to please detekt :)

ee16856

raulraja approved these changes Nov 26, 2018

View reviewed changes

1Jajen1 added 2 commits November 27, 2018 00:42

Fix method signatures and redundant Try return types

9f44f44

Fix unused import

5e12e53

Output percentage correctly

60a87bd

Merge branch 'master' into refractor-deferredK

6dbbdc1

pakoito merged commit 8182042 into arrow-kt:master Nov 27, 2018

1Jajen1 mentioned this pull request Nov 27, 2018

DeferredK memoizes its result #1156

Closed

Attempt to make DeferredK rerunnable #1157

Attempt to make DeferredK rerunnable #1157

Conversation

1Jajen1 commented Nov 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakoito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulraja left a comment

Choose a reason for hiding this comment

raulraja left a comment

Choose a reason for hiding this comment

1Jajen1 commented Nov 25, 2018 • edited

1Jajen1 commented Nov 25, 2018

pakoito commented Nov 25, 2018 • edited

1Jajen1 commented Nov 25, 2018

pakoito commented Nov 25, 2018

1Jajen1 commented Nov 25, 2018

pakoito commented Nov 25, 2018

raulraja commented Nov 25, 2018 • edited

1Jajen1 commented Nov 25, 2018

pakoito commented Nov 25, 2018

1Jajen1 commented Nov 25, 2018

raulraja commented Nov 25, 2018 • edited

1Jajen1 commented Nov 25, 2018

raulraja commented Nov 25, 2018

1Jajen1 commented Nov 25, 2018

1Jajen1 commented Nov 26, 2018 • edited

1Jajen1 commented Nov 26, 2018

raulraja commented Nov 26, 2018

1Jajen1 commented Nov 26, 2018

1Jajen1 commented Nov 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Jajen1 commented Nov 26, 2018

raulraja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1Jajen1 commented Nov 26, 2018 • edited

raulraja commented Nov 26, 2018

pakoito commented Nov 27, 2018

1Jajen1 commented Nov 27, 2018

codecov bot commented Nov 27, 2018

Codecov Report

1Jajen1 commented Nov 25, 2018 •

edited

pakoito commented Nov 25, 2018 •

edited

raulraja commented Nov 25, 2018 •

edited

raulraja commented Nov 25, 2018 •

edited

1Jajen1 commented Nov 26, 2018 •

edited

1Jajen1 commented Nov 26, 2018 •

edited

1Jajen1 commented Nov 26, 2018 •

edited