Fix #1619: Reduce collision rate in NameTable #3768
Conversation
|
test performance please |
|
performance test scheduled: 4 job(s) in queue, 1 running. |
| private def hashValue(cs: Array[Char], offset: Int, len: Int): Int = { | ||
| var i = offset | ||
| var hash = 0 | ||
| while (i < len + offset) { |
There was a problem hiding this comment.
Compute len+offset only once.
There was a problem hiding this comment.
Pretty sure the jvm should figure it out :).
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3768/ to see the changes. Benchmarks is based on merging with master (5b9dfe5) |
| @@ -549,7 +549,8 @@ object Names { | |||
| private def hashValue(cs: Array[Char], offset: Int, len: Int): Int = { | |||
| var i = offset | |||
| var hash = 0 | |||
| while (i < len + offset) { | |||
| val until = offset + len | |||
There was a problem hiding this comment.
I doubt this has any effect. Hoisting constant expressions out of loops is among the easiest optimizations of a JIT compiler. So I think it's better to revert this commit.
Otherwise LGTM.
|
Not sure if we should merge this. It seems to have negligible effects on our benchmarks, but the corresponding scalac PR (scala/scala#5474) hasn't been merged because of a %2 slowdown on scalap. Unless someone can find a benchmark where we actually do better with this PR in, what's the point anyway? |
|
The situation between dotc and scalac is a bit different because with semantic names we have on average much shorter names in the NameTable. So the fix is likely less costly (and potentially also less useful) than for scalac. I think it would be good to benchmark I believe the benefit of this fix is not so much a speedup on average but avoiding outliers. |
allanrenucci
force-pushed
the
fix-#1619
branch
from
e1b3726 to
26282c7
Compare
|
@retronym How come scala/scala#5474 changes the magic prime used in the hashcode from 41 to 31? |
|
I wanted to use the same values as |
|
For reference, we ended up merging scala/scala#5506 instead, which avoids needlessly creating new names for fresh type symbols created during overload resolution. |
The fix is consistent with Scalac: https://github.com/scala/scala/pull/5474/files.
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3768-1/ to see the changes. Benchmarks is based on merging with master (14187e2) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3768-2/ to see the changes. Benchmarks is based on merging with master (14187e2) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3768-3/ to see the changes. Benchmarks is based on merging with master (962b4bf) |
Rebased #1727