Merged
Conversation
This "simplification", was tried before and rolled back because it caused some Memory Leaks. Inspecting a heapdump, we saw that the leaks were caused by the compilation creating many objects of type `Bind`, `BindBind`, or `DelegateBind`, in which the `step` (the pull on the left of >>=) was just a terminal. To avoid this problem, we add a "smart constructor" method `bindView`, used when creating a Pull by binding a Pull with a View. This constructor, instead of always creating a new object, tries to apply the evaluation-reduction instead. One problem, however, is that this seems to cause one test on the MonadError laws for Stream to fail, the stack-safety of `tailRecM`. more
diesalbla
commented
Jun 19, 2021
ebd1f49 to
e385771
Compare
The tailRecM - stack safety errors that we observed were genuine, and they were caused by we having an unlimited chain of recursive calls to the `cont` method from the BindBind class. To prevent this, we extract a loop (a tail-recursive method) that keeps unrolling this chain for as long as it is only terminals that it meets throughout.
mpilquist
approved these changes
Jun 24, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retries the change in #2196 , which were reverted in #2394.
This "simplification", was tried before and rolled back because it caused some Memory Leaks. Inspecting a heapdump, we saw that the leaks were caused by the compilation creating many objects of type
Bind,BindBind, orDelegateBind, in which thestep(the pull on the left of >>=) was just a terminal. To avoid this problem, we add a "smart constructor" methodbindView, used when creating a Pull by binding a Pull with a View. This constructor, instead of always creating a new object, applies the "continuation" function directly. This is admisible during compilation.Edit: one consequence of directly calling the
contmethod, to avoid accumulating theDelegateBindobjects, is that thetailrecMmethod of the Monad instance for Stream was no longer stacksafe. We could cause a stack overflow due to the long chain of recursive calls to the methodcontgoing up the chain ofBindBindobjects. To avoid this, we extract a loop (tail-recursive method) to perform this simplification.One problem, however, is that this seems to cause one test on the MonadError laws for Stream to fail, the stack-safety oftailRecM. However, when we run the code of the stack-safety test, using a different runtime, it seems to pass.