Scala Programming Language
  1. Scala Programming Language
  2. SI-7150

Fishy implementation of WeakHashSet in a compiler

    Details

      Description

      It turns out that we have an implementation of WeakHashSet in a compiler:
      https://github.com/scala/scala/blob/master/src/reflect/scala/reflect/internal/util/WeakHashSet.scala

      However, it's semantics are very fishy. I noticed two problems:
      1. The WeakReferenceWithEquals caches hashCode. Now imagine that two WeakReferenceWithEquals point at different objects so they store different hashCodes. Later on the underlaying objects get GCed so now equals return true (because null == null) but hashCode would be still different. That breaks the basic contract between equals and hashCode.
      2. We do not register a ReferenceQueue so WeakReferenceWithEquals instances themselves are never GC'ed and they block slots in has table. Check implementation of Java's WeakHashmap for inspiration how pruning of slots should be done.

        Issue Links

          Activity

          Hide
          Iulian Dragos added a comment -

          Greg, this was discussed on one of the scala mailing lists. The proper solution is to remove the whole class and replace it with a WeakHashMap[A, Unit].

          Show
          Iulian Dragos added a comment - Greg, this was discussed on one of the scala mailing lists. The proper solution is to remove the whole class and replace it with a WeakHashMap [A, Unit] .
          Hide
          Grzegorz Kossakowski added a comment - - edited

          Iulian, I agree that WeakHashMap[A, Unit] or WeakHashMap[A, Null] would be an improvement but we would waste a lot more memory than necessary in that case due to all WeakHashMap.Entry objects having unused field for value. If we want to use WeakHashSet in a very hot places we need to optimize more. See SI-7149 for details.

          Show
          Grzegorz Kossakowski added a comment - - edited Iulian, I agree that WeakHashMap [A, Unit] or WeakHashMap [A, Null] would be an improvement but we would waste a lot more memory than necessary in that case due to all WeakHashMap.Entry objects having unused field for value. If we want to use WeakHashSet in a very hot places we need to optimize more. See SI-7149 for details.
          Hide
          Michael Thorpe added a comment -

          I've got a branch here: https://github.com/mt2309/scala/compare/SI-7150-weakhashmap which wraps java's WeakHashMap, what do people think?

          Show
          Michael Thorpe added a comment - I've got a branch here: https://github.com/mt2309/scala/compare/SI-7150-weakhashmap which wraps java's WeakHashMap, what do people think?
          Hide
          Grzegorz Kossakowski added a comment -

          Michael, wrapping Java's WeakHashMap wouldn't help with the issue I mentioned in SI-7149 where we need an access to getEntry method to implement hash consing.

          Show
          Grzegorz Kossakowski added a comment - Michael, wrapping Java's WeakHashMap wouldn't help with the issue I mentioned in SI-7149 where we need an access to getEntry method to implement hash consing.
          Hide
          Adriaan Moors added a comment -

          Unassigning and rescheduling to M6 as previous deadline was missed.

          Show
          Adriaan Moors added a comment - Unassigning and rescheduling to M6 as previous deadline was missed.
          Hide
          Jason Zaugg added a comment -

          2.11.0-M4 ad63f3630705bd192d9c983399c572b655b3612b
          2.10.3-RC2 3ada7038877928711176da6ebde68528ab5fc39c

          Show
          Jason Zaugg added a comment - 2.11.0-M4 ad63f3630705bd192d9c983399c572b655b3612b 2.10.3-RC2 3ada7038877928711176da6ebde68528ab5fc39c

            People

            • Assignee:
              James Iry
              Reporter:
              Grzegorz Kossakowski
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development