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

Cross empty batches better #220

Merged
merged 3 commits into from Oct 5, 2019

Conversation

@frankmcsherry
Copy link
Member

frankmcsherry commented Oct 4, 2019

This PR attempts to fix #219 which observes memory leaks if data are not inserted. What seems to have happened is that the logic for spanning empty batches erroneously started from the upper limit of batches, where it should have started from the lower limit of batches. This was everywhere correct except when the first batch is empty, in that we would fail to see it.

In addition, both threshold_total and count_total erroneously announced themselves as unary operators, which do not get progress notifications. This means that they would also stall on intervals of empty time. That has also been fixed.

cc @ryzhyk

@ryzhyk

This comment has been minimized.

Copy link
Contributor

ryzhyk commented Oct 4, 2019

Many thanks for the fix! Do you know when the bugs were introduced? I am asking because I did not observe this behavior in 0.9.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Oct 4, 2019

Probably around then. I don't recall which features landed when, but empty batches had some work done in the 0.9 to 0.10 transition. Resources got stuck less (previously, memory leak on all empty intervals) but apparently the fix had issues. It may just have been different settings where the leaks happened (but the definitely happened before).

@ryzhyk

This comment has been minimized.

Copy link
Contributor

ryzhyk commented Oct 4, 2019

I still see the exact same behavior (memory footprint steadily increasing) using the code in #219 with the empty_batch branch.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Oct 4, 2019

Interesting. The smaller repro was fixed for me, but let me retry your larger repro.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Oct 4, 2019

Ah, I see the issue. It's another flavor of the same thing, but in join. Such a nuisance!

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

frankmcsherry commented Oct 4, 2019

This should be ready to try out again (your repro seems to work fine for me now).

@ryzhyk

This comment has been minimized.

Copy link
Contributor

ryzhyk commented Oct 4, 2019

Thanks! Let me try it on the DDlog-generated program where we observed this originally.

@ryzhyk

This comment has been minimized.

Copy link
Contributor

ryzhyk commented Oct 4, 2019

Yep, it all looks healthy now. Thanks a lot for the quick fix!

@frankmcsherry frankmcsherry merged commit 05ac090 into master Oct 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ryzhyk

This comment has been minimized.

Copy link
Contributor

ryzhyk commented Oct 5, 2019

Do you foresee any problems with merging this into continual_merge? (I can do it in my fork, just checking if you know of a reason it wouldn't work).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.