New immutable hash set and map #342
Conversation
|
Seems that I still have to sign the CLA, in order to make the Travis build happy; will do so later on. |
There was a problem hiding this comment.
Thanks a lot @msteindorfer!
This looks great! I haven’t reviewed the implementation details specific to the CHAMP data structure, but only the way you use the collections framework.
| */ | ||
|
|
||
| @SerialVersionUID(2L) | ||
| private[immutable] sealed case class ChampHashMap[K, +V](val rootNode: MapNode[K, V], val cachedJavaHashCode: Int, val cachedSize: Int) |
There was a problem hiding this comment.
I would be in favor of making the type public. (the primary constructor can stay private, though)
There was a problem hiding this comment.
Also, it should not be a case class, just a plain class, because we don’t want to have the case class generated equals and hashCode.
| with StrictOptimizedIterableOps[(K, V), Iterable /* ChampHashMap */, ChampHashMap[K, V]] | ||
| with Serializable { | ||
|
|
||
| override def iterableFactory = List |
There was a problem hiding this comment.
In my opinion it’s better to omit the override keyword when we only “implement” a member. That prevents to mistakenly override something.
| case _ => super.equals(that) | ||
| } | ||
|
|
||
| override def hashCode(): Int = collection.Set.unorderedHash(toIterable, "Map".##) |
There was a problem hiding this comment.
We should already inherit the same implementation from collection.Map, so this line seems unnecessary.
|
|
||
| val BitPartitionMask = (1 << BitPartitionSize) - 1 | ||
|
|
||
| val MaxDepth = ceil(HashCodeLength.asInstanceOf[Double] / BitPartitionSize).asInstanceOf[Int] |
There was a problem hiding this comment.
ceil(HashCodeLength.toDouble / BitPartitionSize).toInt?
|
|
||
| val SizeMoreThanOne = 2 | ||
|
|
||
| def $mask(hash: Int, shift: Int): Int = (hash >>> shift) & BitPartitionMask |
There was a problem hiding this comment.
This is not a good idea to start an identifier with the $ character because the scala compiler also uses this prefix for compiler-generated identifiers.
There was a problem hiding this comment.
The $ prefix was a workaround for compiler issues that I had, but maybe you can recommend me a better solution.
Without the $ prefix, the statement val mask = mask(elementHash, shift) will fail compilation:
[error] ChampHashSet.scala:160: recursive value mask needs type
[error] val mask = mask(elementHash, shift)
[error] ^
[error] one error found
Even when using a type annotation as in val mask: Int = mask(elementHash, shift), the compiler cannot infer that I want to call the mask function:
[error] ChampHashSet.scala:160: Int does not take parameters
[error] val mask: Int = mask(elementHash, shift)
[error] ^
[error] one error found
Any suggestions apart from fully qualifying the method (e.g., with SetNode.mask)?
There was a problem hiding this comment.
One solution is to use a different name for $mask but a name that has no dollar sign in it. Like maskValue, for instance. Otherwise the solution that consists in fully qualifying the method is good too.
| with StrictOptimizedIterableOps[A, ChampHashSet, ChampHashSet[A]] | ||
| with Serializable { | ||
|
|
||
| def iterableFactory = ChampHashSet |
There was a problem hiding this comment.
Would you mind adding an explicit type annotation?
def iterableFactory: IterableFactory[ChampHashSet] = ChampHashSet| case _ => false | ||
| } | ||
|
|
||
| override def hashCode(): Int = collection.Set.unorderedHash(toIterable, "Set".##) |
There was a problem hiding this comment.
Agree. I'm removing it here, but typically when overriding equals I also add an explicit override of hashCode to avoid hash-code contract related issues. This is also advised in http://www.artima.com/pins1ed/object-equality.html.
| (this eq set) || | ||
| (set canEqual this) && | ||
| (toIterable.size == set.size) && | ||
| (this subsetOf set) |
There was a problem hiding this comment.
This branch is not necessary (provided that you don’t use a case class anymore)
|
|
||
| val BitPartitionMask = (1 << BitPartitionSize) - 1 | ||
|
|
||
| val MaxDepth = ceil(HashCodeLength.asInstanceOf[Double] / BitPartitionSize).asInstanceOf[Int] |
There was a problem hiding this comment.
Here too you should use toDouble and toInt rather than asInstanceOf.
You could also move the common code between ChampHashMap and ChampHashSet in a separate place to not repeat it. (like we do with the Hashing thing)
There was a problem hiding this comment.
Thanks for the hint. I was aware that there's similar code between both implementations that should be factored out. It's still on my TODO list. However, I preferred to expose the current implementation earlier to the community to get feedback.
| (this.content.size == node.content.size) && | ||
| (this.content.filterNot(element0 => node.content.contains(element0))).isEmpty | ||
| case _ => false | ||
| } |
There was a problem hiding this comment.
Are you referring to the equals method? It's part of the structural equality implementation and gets invoked in the ChampHashSet#equals in the expression (this.rootNode == set.rootNode). So, whenever you compare two sets that have hash collisions, then this method is executed.
There was a problem hiding this comment.
Yes, the equals method. Thanks for the clarification. So why don’t you need to also implement hashCode in that case?
There was a problem hiding this comment.
BitmapIndexedSetNode and HashCollisionSetNode are private to the file and are never hashed or put into any hashed data structures that's why I omitted and the hashCode implementation; these classes represent the trie encoding of the data structure. On the other hand, equals is called as shown before to faster compare to trie data structures.
There was a problem hiding this comment.
Excellent, thanks for the clarification! Maybe you can put a comment in the file about that?
|
@msteindorfer you will also have to sign the Scala CLA: https://travis-ci.org/scala/collection-strawman/builds/329365881#L464-L469 |
| def empty[K, V]: MapNode[K, V] = | ||
| EmptyMapNode.asInstanceOf[MapNode[K, V]] | ||
|
|
||
| val TupleLength = 2 |
There was a problem hiding this comment.
We should not expose these constants publicly. They could also be made into compile time constants.
private final val TupleLength = 2 // final, no type annotation, callers will constant fold this in.
There was a problem hiding this comment.
Thanks. I "finalized" all the constants. (I assumed that vals in companion objects are final anyways, but seems that I wrong there.)
I agree in not exposing these constants, but then it would suffice to make the companion object private (e.g., private[immutable] object MapNode {}) instead of each constant, right?
|
It looks like Dotty doesn’t like what you wrote :( https://travis-ci.org/scala/collection-strawman/builds/330072462#L1269 |
|
@julienrf how I can build |
|
Modify the scalaVersion := dotty.value // instead of "2.13.0-M2" |
|
That being said, it’s likely to be a bug in Dotty (the compiler crashed instead of reporting an error) |
| with StrictOptimizedIterableOps[(K, V), Iterable /* ChampHashMap */, ChampHashMap[K, V]] | ||
| with Serializable { | ||
|
|
||
| def iterableFactory: IterableFactoryLike[List] = List |
There was a problem hiding this comment.
I would use the default immutable.Iterable here:
def iterableFactory: IterableFactory[Iterable] = Iterable(currently the default Iterable happens to be List but that could change…)
| (this eq node) || | ||
| (this.hash == node.hash) && | ||
| (this.content.size == node.content.size) && | ||
| (this.content.filterNot(keyValueTuple => node.content.contains(keyValueTuple))).isEmpty |
There was a problem hiding this comment.
This could be optimized even further by using forall:
this.content.forall(node.content.contains)
| (this eq node) || | ||
| (this.hash == node.hash) && | ||
| (this.content.size == node.content.size) && | ||
| (this.content.filterNot(element0 => node.content.contains(element0))).isEmpty |
There was a problem hiding this comment.
You can apply the same optimization here.
|
Great, now the CI is green! I found a couple more issues or possible improvements. Then, to merge it we need the following:
Does that work for you? |
|
@julienrf, yes, that works for me. I'll probably go over it in the weekend, and also apply some further enhancements by factoring out commonalities. Regarding the tests, I've to see how to integrate them best with |
|
You can straightforwardly port the junit tests to our test/junit module. For the property based tests, we already have some tests that use scalacheck, in the test/scalacheck module, you can try to port your tests in that module, and I can help you if needed. |
|
@julienrf the pull-request now also contains the junit and scalacheck tests. Regarding, squashing of commits: do I really have to do it myself? As far as I know, the person who merges a PR in GitHub can select do squash all commits to one. |
|
Over in scala/scala, we typically ask that you squash and reformat / copy-edit the commit message so that it reads nicely. This is super important especially for new, tricky stuff. The maintainers send their thanks from the future. |
|
@adriaanm understood; makes sense. Then I'll take care myself of squashing the commits and preparing a nice and elaborate commit message. |
| this.hash == hash && content.find(key == _._1).isDefined | ||
|
|
||
| override def contains[V1 >: V](key: K, value: V1, hash: Int, shift: Int): Boolean = | ||
| this.hash == hash && content.find(payload => key == payload._1 && value == payload._2).isDefined |
There was a problem hiding this comment.
You can simplify find(p).isDefined by exists(p)
|
|
||
| def tuple[KV](keyValue: KV): (KV, KV) = tuple(keyValue, keyValue) | ||
|
|
||
| def tuple[K, V](key: K, value: V): (K, V) = Tuple2.apply(key, value) |
There was a problem hiding this comment.
Why don’t you just write (key, value) to make a tuple? Alternatively, you can use the key -> value syntax.
|
|
||
| override def equals(other: Any): Boolean = other match { | ||
| case that: DummyValue => | ||
| (this eq that) || hash == that.hash && value == that.value |
There was a problem hiding this comment.
Alternatively, you can define DummyValue as a case class and not worry about hashCode and equals (they will be defined to implement value equality).
BTW, in your implementation DummyValue(1, 2) != DummyValue(2, 2) although DummyValue(1, 2).## == DummyValue(2, 2).##, is that expected?
There was a problem hiding this comment.
Thanks for the question, yes this is intended behaviour in order to create values for testing hash-collisions. I documented my intend and renamed the class to CustomHashInt in order to make this clear.
| private def convertToScalaMapAndCheckHashCode(input: ChampHashMap[K, V]) = | ||
| HashMap.from(input).hashCode == input.hashCode | ||
|
|
||
| property("convertToJavaMapAndCheckEquality") = forAll { (input: ChampHashMap[K, V]) => |
There was a problem hiding this comment.
Your implementation does not convert to Java
| property("containsAfterInsert") = forAll { (inputValues: HashMap[K, V]) => | ||
| var constructedMap = ChampHashMap.empty[K, V] | ||
|
|
||
| inputValues.foreach(item => constructedMap += item) |
There was a problem hiding this comment.
You could also write:
val constructedMap = ChampHashMap.empty[K, V] ++ inputValues
| var constructedMap = ChampHashMap.empty[K, V] | ||
|
|
||
| inputValues.foreach(item => constructedMap += item) | ||
| inputValues.forall(keyValueTuple => constructedMap.get(keyValueTuple._1).get == keyValueTuple._2) |
There was a problem hiding this comment.
Instead of myOption.get == something you can write myOption.contains(something):
inputValues.forall { case (key, value) => constructedMap.get(key).contains(value) }
|
|
||
| protected[this] def newSpecificBuilder(): Builder[(K, V), ChampHashMap[K, V]] = ChampHashMap.newBuilder() | ||
|
|
||
| override def knownSize: Int = cachedSize |
There was a problem hiding this comment.
You might also want to override size (the default implementation tests whether knownSize is defined or not, so you would just save that comparison…)
There was a problem hiding this comment.
Also, isEmpty and nonEmpty could be overridden for performance too (the default implementation creates an Iterator and then checks whether that iterator is empty or not)
|
|
||
| assert(TupleLength * payloadArity + nodeArity == content.length) | ||
| assert(Range(0, TupleLength * payloadArity).forall(i => !content(i).isInstanceOf[MapNode[_, _]])) | ||
| assert(Range(TupleLength * payloadArity, content.length).forall(i => content(i).isInstanceOf[MapNode[_, _]])) |
There was a problem hiding this comment.
These assertions will be evaluated by user code unless they use a specific compiler option. Is it possible to move them to tests instead?
| } | ||
|
|
||
| @SerialVersionUID(2L) | ||
| private[this] final class BitmapIndexedMapNode[K, +V](val dataMap: Int, val nodeMap: Int, val content: Array[Any]) extends MapNode[K, V] { |
There was a problem hiding this comment.
I’m surprised that this code compiles, I have no idea what this is bound to here…
msteindorfer
force-pushed
the
new-immutable-hash-set-and-map
branch
from
4101a85
to
e8a7a1e
Compare
|
@julienrf I addressed the comments you made above, added further performance optimizations, and also squashed all commits of this PR to a single commit that contains a descriptive commit message summarizing this effort. I'm looking forward having this PR merged. |
| ys = ys.init | ||
| } | ||
| } | ||
| // // TODO: currently disabled, since it does not finish |
There was a problem hiding this comment.
BTW do you have any idea on why this benchmark doesn’t finish?
There was a problem hiding this comment.
Yes, I do. It's a performance issue since the code calls twice per iteration last (once explicitly, and one time hidden in init) and last causes an iteration through the whole collection (front-to-back). This performance issue is solved with the CHAMP data structures, since I implemented a backwards iterator that avoids iterating front-to-back for obtaining the last element.
There was a problem hiding this comment.
Do you guys run the benchmarks regularly? I wondered why this problem wasn't spotted earlier on.
There was a problem hiding this comment.
No, we run them occasionally, and we always select a subset of the benchmarks to run.
|
I’ve run the benchmarks comparing
Sorry, the definition of these charts is not super good and sometimes we have huge confidence intervals that make it even harder to compare the lines. So, here are the raw numbers: |
|
@julienrf thanks. Could you also a) run the memory benchmarks, and b) run a more extended set of set operations? I'll report back in more detail later on. |
|
Here is the result of the memory benchmark:
ChampHashSet is 25% smaller than our current hashset. |
|
@julienrf I started looking into your reports and I'm trying to run and replicate things locally. So far I've looked into the After adding an override of
|
|
Regarding the memory results, I found out that the The memory benchmark does count the total amount of memory allocated, thus it measures the total retained heap size of the data structures, including all the boxed (!) For sets, even in the case of the vast under-approximation the savings are ~35%, whereas the precise measurement shows savings beyond 50%. For maps, the flaw of the measurement is even more severe, since it contains the double amount of boxed Once again, even in the case of the vast under-approximation the space savings for maps are ~50%, whereas the precise measurement shows space savings of 3-4x. |
|
I hope that the comments above help to clarify some of the concerns mentioned above. Looking into the other issues will take me some time, I can earliest start on the weekend looking into it, but will report back afterwards. I'm also going to add the |
|
@msteindorfer Thanks a lot for your answers! Could you please help us fixing our memory benchmark? |
|
Would you mind adding the following benchmarks to @Benchmark
def traverse_subsetOf(bh: Blackhole): Unit = bh.consume(xs.subsetOf(xs))
@Benchmark
def traverse_equals(bh: Blackhole): Unit = bh.consume(xs == xs) |
| var zs: ChampHashSet[Long] = _ | ||
| var zipped: ChampHashSet[(Long, Long)] = _ | ||
| var randomIndices: scala.Array[Int] = _ | ||
| def fresh(n: Int) = ChampHashSet((1 to n).map(_.toLong): _*) |
There was a problem hiding this comment.
It might be worthwhile varying the key types during the benchmark to prevent JIT from having an unrealistically easy time inlining the hashing/equality (for Double keys, that goes through BoxesRuntime.{equals,hashCode}).
This is a general problem for our collection benchmarks, but it is particularly important for hash-structures IMO.
There was a problem hiding this comment.
This is what I tried in the benchmark of the existing Scala hash map: https://github.com/scala/scala/blob/5c93cd2431276fe3c712cb60e8a7a696c1776f10/test/benchmarks/src/main/scala/scala/collection/mutable/HashMapBenchmark.scala#L25-L33
|
I’ve run the benchmarks for
The numbers: |
|
I've had a look at
|
|
Wow, this is awesome, your implementation is an order of magnitude faster than the current one! |
|
@julienrf, yes, I can also work on PS: I the meantime, I was already prototyping a specialized
|
msteindorfer
force-pushed
the
new-immutable-hash-set-and-map
branch
from
e8a7a1e
to
777e5dc
Compare
|
If you think that you would need to spend a substantial amount of time to optimize the operations then I’m happy to merge this PR as soon as possible and let you open other PRs for performance improvements. Since we want to our code to compile with Dotty we are a bit constrained and can not use the |
|
Just saw the build not succeeding on Dotty, will fix the case with |
msteindorfer
force-pushed
the
new-immutable-hash-set-and-map
branch
from
7000f5a
to
566b143
Compare
The reimplementations are based upon Compressed Hash-Array Mapped Prefix-trees (CHAMP), see paper "Optimizing Hash-Array Mapped Tries for Fast and Lean Immutable JVM Collections" by Steindorfer and Vinju (OOPSLA'15) for more details and descriptions of low-level performance optimizations (a pre-print of the paper is available under https://michael.steindorfer.name/publications/oopsla15.pdf). This commit closes scala#192. The new implementations (i.e., ChampHashSet and ChampHashMap) currently exist next to the previous HashMap and HashSet. By default immutable.Map and immutable.Set now pickup the CHAMP data structures. A JVM flag (-Dstrawman.collection.immutable.useBaseline=true) allows to switch back to the previous HashSet and HashMap implementations for testing. Note, the flag and the previous HashSet and HashMap implementations will be removed in the final version of collection-strawman, but for the time being they remain to support comparing the different trade-offs and performance characteristics of the current and the new data structures. Preliminary performance numbers of the new CHAMP data structures were presented in issue scala#192. Overall one can summarize that the CHAMP data structures significantly lower memory footprints and significantly improve all iteration-based operations and equality checks. Basic operations such as lookup, insertion, and deletion may slow down. The current state of the reimplementation does not optimize for hash-collisions yet. Note that the CHAMP design / implementation differs from the previous immutable hashed data structures by not memoizing the hash codes of the individual elements (which may change the performance of certain workloads). If necessary, CHAMP's design allows to modularly add memoized hash codes of the individual elements (at the expense of some memory savings). Details are discussed in the paper mentioned above.
msteindorfer
force-pushed
the
new-immutable-hash-set-and-map
branch
from
566b143
to
d5ae93e
Compare
|
I did implement and add a specialized implementation of the Note, with respect to the performance regression discussion of issue #380, the CHAMP design may have to re-calculate hash-codes (for a fraction of the elements) since the elements's hash codes are not cached in the the leaf nodes. See https://github.com/msteindorfer/collection-strawman/blob/new-immutable-hash-set-and-map/collections/src/main/scala/strawman/collection/immutable/ChampHashSet.scala#L433. This is desired behaviour, and enables significant memory savings, while sacrificing only little runtime performance.
|
|
I haven't checked it in depth but the benchmarks look great, and once we have it in we can catch any remaining correctness issues more easily. (I'll run collections-laws on it.) |
|
Let’s merge it so that we can move forward! |
This pull request contains reimplementations of immutable
HashSetandHashMapusing Compressed Hash-Array Mapped Prefix-trees (CHAMP). A prototype and preliminary evaluation of CHAMP data structures in collection-strawman was already discussed in issue #192.The new implementations (
ChampHashSetandChampHashMap) currently exist next to theHashMapandHashSet. By defaultimmutable.Mapandimmutable.Setnow pickup the CHAMP versions, but I also implemented a JVM flag (-Dstrawman.collection.immutable.useBaseline=true) to default to the currentHashSetandHashMapimplementations. I assume that in the final version of the collections only one hash-map/set will ship, but for now, having a flag helps with comparing the different trade-offs and performance characteristics of the current and the new data structures.The data structures contained in this pull request represent a first basic re-implementation of
HashSetandHashMap. The data structures should be functionally complete at this point, however there are still parts and operations that can benefit from (further) optimizations. I plan to continue working on those parts that still need attention, but in the meantime I request a review of the current state.Preliminary performance numbers of the new CHAMP data structures were presented in issue #192; those characteristics didn't change with the Scala re-implementaiton. (I can post further results here upon request, but you might be interested to give them a spin yourself.) Overall one can summarize that the CHAMP data structures significantly lower memory footprints and significantly improve all iteration-based operations and equality checks. Lookups slow down, but insertion and deletion also seem to benefit as well.
Note that the CHAMP design / implementation differs from the current hashed data structures by not memoizing the hash codes of the individual elements (which may change the performance of certain workloads). If necessary, CHAMP's design allows to modularly add memoizing the hash codes of the individual elements (at the expense of some memory savings); details are discussed in the OOPSLA'15 paper. I plan to further explore the memoized approach in the context of collection-strawmen as well in the near future.
I'm looking forward to receiving feedback on the current implementation, to further improve the code based on your reviews.