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

Fix LazyBehavior bug #122

Closed
jam40jeff opened this issue Feb 6, 2018 · 6 comments
Closed

Fix LazyBehavior bug #122

jam40jeff opened this issue Feb 6, 2018 · 6 comments
Assignees
Labels

Comments

@jam40jeff
Copy link
Contributor

jam40jeff commented Feb 6, 2018

Lazy cells (and behaviors) have the purpose of delaying their initial value computation such that they work with loops. For example, give a Stream<int> s and a CellLoop<int> c, the following will work:

Cell<int> cell = s.HoldLazy(c.SampleLazy());

Most (if not all) primitives use HoldLazy such that they will work with loops.

However, currently the initial value for the lazy cell is only computed when it is first sampled. This can be problematic if the initial value of a cell is another cell. For example, the following should produce the value 1, but will instead currently produce the value 0:

int lastValueSeen = -1;
StreamSink<int> s = new StreamSink<int>();
Cell<Cell<int>> c = Cell.Constant(s).Map(v => v.Hold(0));
s.Send(1);
Cell<int> cell = c.SwitchC();
cell.Listen(v => lastValueSeen = v);

The problem is that the inner cell is created during the SwitchC call rather than the moment the transaction created by the line Cell.Constant(s).Map(v => v.Hold(0)); ends, and it therefore misses the sending of the value 1.

Lazily created cells and behaviors should thus have their values created at the end of the transaction in which they were created instead of the first time they are sampled.

@jam40jeff jam40jeff added the bug label Feb 6, 2018
@jam40jeff jam40jeff self-assigned this Feb 6, 2018
@jam40jeff
Copy link
Contributor Author

jam40jeff commented Feb 6, 2018

This bug was caught when debugging the issue @dakom raised about initial cells within cells losing not being listened to correctly after SwitchC. That issue was caused by SampleLazy computing its value during the "last" phase of the transaction. A new "sample" phase was added as part of the "prioritized loop" to fix this. I would propose using this same phase to compute the values for LazyBehavior as well.

@the-real-blackh
Copy link

Thank you for finding this issue! I'm keen to fix it in all versions. I can see that it's fixed in

73cbcf9
c1b2aa0
523c276
5a3504f

Have I missed any?

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Feb 6, 2018

@the-real-blackh Sorry, I should have clarified. Although I found this issue while testing the bug @dakom found, that issue was separate. I should open an issue for that one as well (although it has already been fixed in the C# code there was no issue created for it). This is a slightly different issue that isn't fixed by the code in the 4 revisions you mentioned and has yet to be fixed. I have a potential fix waiting on my machine, but I want to do some performance profiling first to make sure it's the right solution to the problem.

@the-real-blackh
Copy link

I'll rename the LazyBehaviour bugs that I've raised to "Apply in SwitchC". Is that correct?

@jam40jeff
Copy link
Contributor Author

jam40jeff commented Feb 6, 2018

No, the LazyBehavior bug is new (and yet to be fixed). I created a new issue for the commits you mentioned called Fix SampleLazy bug. That is the bug which has already been fixed in C# (for which the commits say something about Apply in SwitchC, although that was just one manifestation of the bug and not the root cause).

Both bugs should be fixed in the other languages, though.

@jam40jeff
Copy link
Contributor Author

Also, I should mention that although the title refers to LazyBehavior, this bug will still exist in the other languages in the LazyCell class even if Behavior has not yet been added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants