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

Fishy implementation of WeakHashSet in a compiler #7150

Closed
scabug opened this issue Feb 19, 2013 · 7 comments
Closed

Fishy implementation of WeakHashSet in a compiler #7150

scabug opened this issue Feb 19, 2013 · 7 comments
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Feb 19, 2013

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 hash table. Check implementation of Java's WeakHashmap for inspiration how pruning of slots should be done.
@scabug
Copy link
Author

@scabug scabug commented Feb 19, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7150?orig=1
Reporter: @gkossakowski
Assignee: @JamesIry
Affected Versions: 2.10.0
Other Milestones: 2.11.0-M4
See #7149

@scabug
Copy link
Author

@scabug scabug commented Feb 21, 2013

@dragos said:
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].

@scabug
Copy link
Author

@scabug scabug commented Mar 4, 2013

@gkossakowski said (edited on Mar 4, 2013 8:00:45 PM UTC):
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 #7149 for details.

@scabug
Copy link
Author

@scabug scabug commented May 18, 2013

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

@scabug
Copy link
Author

@scabug scabug commented Jun 5, 2013

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

@scabug
Copy link
Author

@scabug scabug commented Jul 10, 2013

@adriaanm said:
Unassigning and rescheduling to M6 as previous deadline was missed.

@scabug
Copy link
Author

@scabug scabug commented Sep 13, 2013

@retronym said:

2.11.0-M4 ad63f3630705bd192d9c983399c572b655b3612b
2.10.3-RC2 3ada7038877928711176da6ebde68528ab5fc39c

@scabug scabug closed this Sep 13, 2013
@scabug scabug added this to the 2.10.3-RC2 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant