Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Port memory leak tests to integration tests, runnable via it:test #1889

Merged
merged 6 commits into from
Jun 8, 2020

Conversation

mpilquist
Copy link
Member

Bad news: two tests are failing - parJoin and `signal continuous'.

@SystemFw
Copy link
Collaborator

SystemFw commented Jun 8, 2020

The silver lining is that this test strategy seems to be very useful :)

@mpilquist
Copy link
Member Author

Both leaks appear to have existed since 1.0.0 and didn't exist in 0.9.7 (this is definitely true for parJoin and I think it's true for the other, though I started focusing on parJoin).

I updated the PR to write a heap dump upon test failure. Analyzing the parJoin failure points to a leak in PubSub.Strategy.Discrete -- looks like the outOfOrder map is huge.

@SystemFw
Copy link
Collaborator

SystemFw commented Jun 8, 2020

btw I'm getting a bit worried about PubSub in general.
I appreciate the interface reusability that it gives, but we already know for sure that Topic has a contention issue that isn't immediately fixable with PubSub, and now this leak.

Hopefully Pavel can save the day :)
But if not, do you think it's something that needs addressing at the root, or should we just fix it as we go?

@mpilquist
Copy link
Member Author

I rolled SignallingRef back to its 0.9 implementation, which fixed both failing leak tests.

The discrete pubsub strategy definitely seems to have a leak where the outOfOrder map grows very very large, though I wasn't able to work out the exact case in which it happens. I find the pubsub stuff hard to reason about, but perhaps that's just unfamiliarity. I have no strong opinions on pubsub going forward, though I don't think we should be shy about writing custom concurrent data structures if we get stuff in exchange -- i.e., maximally reusing pubsub is not a goal.

@SystemFw
Copy link
Collaborator

SystemFw commented Jun 8, 2020

i.e., maximally reusing pubsub is not a goal.

Right, cause we already know that for Topic is for sure a regression

@mpilquist mpilquist merged commit c205273 into typelevel:master Jun 8, 2020
@mpilquist mpilquist added this to the 2.4.0 milestone Jun 8, 2020
@mpilquist mpilquist deleted the topic/leaks branch February 18, 2024 13:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants