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

Avoid memory leaks in Stream methods #8367

Merged
merged 2 commits into from Aug 28, 2019
Merged

Conversation

szeiger
Copy link
Member

@szeiger szeiger commented Aug 23, 2019

Stream.collect/collectFirst/find kept references to the original
receiver of the call which prevented any part of the Stream from
getting garbage collected as soon as possible.

The previous implementation of Stream.collect claimed to avoid this
but this only worked insofar as the resulting Stream did not hold on
unnecessarily to any references to the original Stream. It still kept
a reference to the original receiver while iterating over non-matching
elements which can only be avoided with a tail-recursive
implementation.

Fixes scala/bug#11443

@szeiger szeiger added this to the 2.13.1 milestone Aug 23, 2019
@NthPortal NthPortal added the library:collections PRs involving changes to the standard collection library label Aug 24, 2019
@szeiger
Copy link
Member Author

szeiger commented Aug 26, 2019

Huh, that's interesting. It fails when the optimizer is enabled.

@lrytz
Copy link
Member

lrytz commented Aug 26, 2019

We had that before, maybe it's a different situation than the one fixed in 505b01c. I can take a look.

@lrytz lrytz force-pushed the issue/11443 branch 4 times, most recently from 9c07efd to 51a890c Compare August 27, 2019 14:29
@lrytz
Copy link
Member

lrytz commented Aug 27, 2019

[error] - testJarSize
[error]   - root/testJarSize failed: The library jar is too big: 5692760 bytes.

nice, the test works :-)

@NthPortal
Copy link
Contributor

nice, the test works :-)

what's the solution to this?

@lrytz lrytz force-pushed the issue/11443 branch 2 times, most recently from c85daff to 595bc2b Compare August 27, 2019 20:29
@lrytz
Copy link
Member

lrytz commented Aug 28, 2019

The @tailrec method in Stream doesn't compile with dotty due to scala/scala3#7116.

@szeiger
Copy link
Member Author

szeiger commented Aug 28, 2019

I'll add final. I didn't even know it was supposed to work for sealed classes...

lrytz and others added 2 commits August 28, 2019 14:45
The inliner replaces `return` instructions by `store`s to save the
result of an inlined method. After loading that result, the local
variable should be nulled out to preven memory leaks.

This case was overlooked in 505b01c.
`Stream.collect/collectFirst/find` kept references to the original
receiver of the call which prevented any part of the Stream from
getting garbage collected as soon as possible.

The previous implementation of `Stream.collect` claimed to avoid this
but this only worked insofar as the resulting Stream did not hold on
unnecessarily to any references to the original Stream. It still kept
a reference to the original receiver while iterating over non-matching
elements which can only be avoided with a tail-recursive
implementation.

Fixes scala/bug#11443
@lrytz
Copy link
Member

lrytz commented Aug 28, 2019

MiMa doesn't know about children of sealed classes either :-) added an exception.

@szeiger szeiger merged commit d6bb22a into scala:2.13.x Aug 28, 2019
@szeiger
Copy link
Member Author

szeiger commented Aug 28, 2019

Phew, finally!

@szeiger szeiger deleted the issue/11443 branch August 28, 2019 15:01
@szeiger szeiger added the release-notes worth highlighting in next release notes label Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:collections PRs involving changes to the standard collection library release-notes worth highlighting in next release notes
Projects
None yet
3 participants