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

LinkedHashSet.addAll does not make call to sizeHint #12760

Closed
KisaragiEffective opened this issue Mar 27, 2023 · 7 comments
Closed

LinkedHashSet.addAll does not make call to sizeHint #12760

KisaragiEffective opened this issue Mar 27, 2023 · 7 comments

Comments

@KisaragiEffective
Copy link

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
Copy link
Member

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

@liang3zy22
Copy link

liang3zy22 commented Mar 27, 2023

Overriding scala.collection.mutable.LinkedHashSet#addAll should cause binary compatibility issue. See the link below:
scala/scala#10235 (comment)

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

@SethTisue
Copy link
Member

@KisaragiEffective have you tested on the current 2.13.11 nightly?

@KisaragiEffective
Copy link
Author

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

@SethTisue
Copy link
Member

SethTisue commented Mar 27, 2023

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

@KisaragiEffective
Copy link
Author

@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
Copy link
Member

lrytz commented Apr 4, 2023

I didn't realize this was related to scala/scala#10258, I'll take a look if we can include it in 2.13.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants