scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

LinkedHashSet.addAll does not make call to sizeHint #12760

Closed KisaragiEffective closed 1 year ago

KisaragiEffective commented 1 year ago

Reproduction steps

Scala version: 2.13.10 (scastie)

import scala.collection.mutable

val random = new java.util.Random()
val randomSet: Set[Int] = (1 to 1000000).map(_ => random.nextInt()).toSet;
{
  val t = System.nanoTime()
    val hashSet: mutable.HashSet[Int] = new mutable.HashSet()
  hashSet.addAll(randomSet);

  println(s"${System.nanoTime() - t} ns")
};
{
  val t = System.nanoTime()
    val linkedHashSet: mutable.LinkedHashSet[Int] = new mutable.LinkedHashSet()
  linkedHashSet.addAll(randomSet);

  println(s"${System.nanoTime() - t} ns")  
}

Problem

scala.collection.mutable.LinkedHashSet#addAll is not overriden, causing unnecessary multiple allocations.

In above reproduction code, LinkedHashSet is slower than HashSet about 3 times.

SethTisue commented 1 year ago

seems plausible. @scala/collections crew might have further insight

liang3zy22 commented 1 year ago

Overriding scala.collection.mutable.LinkedHashSet#addAll should cause binary compatibility issue. See the link below: https://github.com/scala/scala/pull/10235#issuecomment-1336781619

One of the binary compatibility issue is from LinkedHashMap#addAll. I think LinkedHashSet#addAll should have same issue.

SethTisue commented 1 year ago

@KisaragiEffective have you tested on the current 2.13.11 nightly?

KisaragiEffective commented 1 year ago

@SethTisue No, not yet. I don't know how to test this with latest nightly.

SethTisue commented 1 year ago

@KisaragiEffective scala-cli -S 2.nightly; see also https://stackoverflow.com/a/40622879/86485 for other methods and more detail

KisaragiEffective commented 1 year ago

@SethTisue I've tested with 2.13.11-bin-8269bdd, and 5 of 5 attempts indicate LinkedHashSet is slower 2~4 times than HashSet.

lrytz commented 1 year ago

I didn't realize this was related to https://github.com/scala/scala/pull/10258, I'll take a look if we can include it in 2.13.11