Backport #2605 to 2.10.x: SI-7149 Use a WeakHashSet for type uniqueness #2901
Conversation
This file contains 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
Replaces scala.reflect.internal.WeakHashSet with a version that * extends the mutable.Set trait * doesn't leak WeakReferences * is unit tested
Currently type uniqueness is done via a HashSet[Type], but that means the Types live through an entire compile session, even ones that are used once. The result is a huge amount of unnecessarily retained memory. This commit uses a WeakHashSet instead so that Types and their WeakReferences are cleaned up when no longer in use.
perRunCaches was using a HashMap of WeakReferences which meant it would accumulate WeakReferences over time. This commit uses a WeakHashSet instead so that the references are cleaned up.
|
I fixed the binary compatibility errors. Hopefully tests will pass now. |
|
LGTM. I re-reviewed the change itself to re-convince myself that this can't hurt us. |
|
@retronym: thanks for the review. I'll go and merge it then. |
gkossakowski
added a commit
that referenced
this pull request
Sep 4, 2013
Backport #2605 to 2.10.x: SI-7149 Use a WeakHashSet for type uniqueness
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Jun 25, 2021
This mimics what Scala 2 has been doing for a long time now and serves the same purpose: it considerably reduces peak memory usage when compiling some projects, for example previously compiling the Scalatest tests required a heap of at least 11 GB, but now it fits in about 4 GB. This required changing the implementation of WeakHashSet to have overridable `hash` and `isEqual` methods just like HashSet, it also required making various private methods protected since NamedTypeUniques and AppliedUniques contain an inlined implementation of `put`. This commit also changes the default load factor of a WeakHashSet from 0.75 to 0.5 to match the load factor we use for HashSets, though note that Scala 2 has always been using 0.75. For a history of the usage of WeakHashSet in Scala 2 see: - scala/scala#247 - scala/scala#2605 - scala/scala#2901
xuwei-k
pushed a commit
to xuwei-k/scala3
that referenced
this pull request
Jul 6, 2021
This mimics what Scala 2 has been doing for a long time now and serves the same purpose: it considerably reduces peak memory usage when compiling some projects, for example previously compiling the Scalatest tests required a heap of at least 11 GB, but now it fits in about 4 GB. This required changing the implementation of WeakHashSet to have overridable `hash` and `isEqual` methods just like HashSet, it also required making various private methods protected since NamedTypeUniques and AppliedUniques contain an inlined implementation of `put`. This commit also changes the default load factor of a WeakHashSet from 0.75 to 0.5 to match the load factor we use for HashSets, though note that Scala 2 has always been using 0.75. For a history of the usage of WeakHashSet in Scala 2 see: - scala/scala#247 - scala/scala#2605 - scala/scala#2901
BarkingBad
pushed a commit
to BarkingBad/dotty
that referenced
this pull request
Jul 23, 2021
This mimics what Scala 2 has been doing for a long time now and serves the same purpose: it considerably reduces peak memory usage when compiling some projects, for example previously compiling the Scalatest tests required a heap of at least 11 GB, but now it fits in about 4 GB. This required changing the implementation of WeakHashSet to have overridable `hash` and `isEqual` methods just like HashSet, it also required making various private methods protected since NamedTypeUniques and AppliedUniques contain an inlined implementation of `put`. This commit also changes the default load factor of a WeakHashSet from 0.75 to 0.5 to match the load factor we use for HashSets, though note that Scala 2 has always been using 0.75. For a history of the usage of WeakHashSet in Scala 2 see: - scala/scala#247 - scala/scala#2605 - scala/scala#2901
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.
This is backport of #2605.
The issue of uniques accumulating types during the whole compilation run turned out to be especially bad for ScalaTest tests. Scala 2.10.2 requires at least 6GB to compile Scalatest tests. With this patch, 800MB is enough.
Such a dramatic difference in memory consumption and low-risk of this patch makes it appealing to backport it to 2.10.x.
Review by @JamesIry.