Use util Collections more widely #9680

odersky · 2020-08-29T11:28:59Z

We now have high performance mutable set and map implementations in dotc.util. Generalize them and use them more widely.

odersky · 2020-08-30T13:15:46Z

test performance please

dottybot · 2020-08-30T13:16:05Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2020-08-30T14:40:13Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680/ to see the changes.

Benchmarks is based on merging with master (4c99388)

odersky · 2020-08-30T16:49:42Z

test performance please

dottybot · 2020-08-30T16:50:56Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2020-08-30T17:16:47Z

performance test failed:

Please check http://lamppc37.epfl.ch:8000/pull-9680-08-30-18.51.out for more information

odersky · 2020-08-30T21:24:54Z

test performance please

dottybot · 2020-08-31T07:12:09Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2020-08-31T08:35:35Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680/ to see the changes.

Benchmarks is based on merging with master (75c8176)

odersky · 2020-09-01T13:12:25Z

This PR constitutes some further steps where we use exclusively our own hash maps and hash sets in dotc.util. These use open hashing with linear scan which tends to be faster than closed hashing and also consumes less memory. The main disadvantages of this open hashing scheme are:

performance deteriorates faster if the hash function is not good
no Linked... versions (at least for now)

liufengyun · 2020-09-02T08:41:22Z

retest performance please

dottybot · 2020-09-02T08:41:42Z

performance test scheduled: 15 job(s) in queue, 1 running.

smarter · 2020-09-02T09:12:45Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -1564,14 +1564,14 @@ object SymDenotations {
    initPrivateWithin: Symbol)
    extends SymDenotation(symbol, maybeOwner, name, initFlags, initInfo, initPrivateWithin) {

-    import util.HashTable
+    import util.IdentityHashMap


util.IdentityHashMap and java.util.IdentityHashMap are too easy to confuse IMHO. Can we use a different name like EqHashMap?

In fact, if you happen to write your package declaration as package dotty.tools.dotc.core, then import util.IdentityHashMap will compile but import the Java map!

That's somewhat intentional. There's no reason anymore to use a java.util.IdentityHashMap. The dotc.util map is uniformly better in both performance and types.

In fact, if you happen to write your package declaration as package dotty.tools.dotc.core, then import util.IdentityHashMap will compile but import the Java map!

Ah I see what you mean now. Yes, we should do something about that.

dottybot · 2020-09-02T10:07:47Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680-1/ to see the changes.

Benchmarks is based on merging with master (0d22c74)

odersky · 2020-09-02T14:57:20Z

test performance please

dottybot · 2020-09-02T14:59:21Z

performance test scheduled: 21 job(s) in queue, 1 running.

dottybot · 2020-09-02T16:27:49Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680-1/ to see the changes.

Benchmarks is based on merging with master (95a6b44)

Also, give it a more standard map interface. Also, fix removals in small tables

Breakout specialized version for `eq` and `equals`. The duplicate some methods for efficiency.

The reason for using a linear map instead of a mutable.Map here is that most derived instances are very small.

It is already `empty` in SimpleIdentitySet. That way we use the same convention as in the stdlib. # Conflicts: # compiler/src/dotty/tools/dotc/util/LinearIdentityMap.scala

- Increase load factor to 0.5 -- the miss rate seems to be still OK (around one miss per hit or better) while re-sizing costs and go down. - Don't lose lowest hashCode bit.

Reduce synchronized region to updates.

Per-run cache of what is returned for a staticRef

Make lookup return a Value | Null result instead, which makes it a bit safer to use. To support this well, forward port a casting method from the explicit-nulls branch.

It's now an alias of util.IdentityHashMap[Symbol, _]

I overlooked a backport of a fix from HashMap to HashSet.

liufengyun · 2020-09-03T10:16:22Z

test performance please

dottybot · 2020-09-03T10:17:56Z

performance test scheduled: 15 job(s) in queue, 1 running.

dottybot · 2020-09-03T13:37:44Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680-1/ to see the changes.

Benchmarks is based on merging with master (0f757c5)

odersky · 2020-09-03T15:00:17Z

test performance please

dottybot · 2020-09-03T15:01:10Z

performance test scheduled: 16 job(s) in queue, 1 running.

dottybot · 2020-09-03T17:49:28Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9680-1/ to see the changes.

Benchmarks is based on merging with master (cdfc76e)

liufengyun

LGTM

The speedup is small, but at least there is no regression.

liufengyun · 2020-09-03T10:14:31Z

compiler/src/dotty/tools/package.scala

+  extension [T](x: T | Null):
+
+    /** Assert `x` is non null and strip `Null` from type */
+    inline def nn: T =


Minor: for readability

Suggested change

inline def nn: T =

inline def notNull: T =

The current proposal is to use .nn when explicit nulls are in. The theory is that that's something you'll have to do fairly often, so it's good to have a non-obtrusive name.

liufengyun · 2020-09-03T10:15:02Z

compiler/src/dotty/tools/package.scala

+     *  Flow-typing under explicit nulls will automatically insert many necessary
+     *  occurrences of uncheckedNN.
+     */
+    inline def uncheckedNN: T = x.asInstanceOf[T]


Minor: for readability

Suggested change

inline def uncheckedNN: T = x.asInstanceOf[T]

inline def uncheckedNotNull: T = x.asInstanceOf[T]

I'd leave it as is, since I believe that will be the name of the (compiler-generated) method when explicit-nulls is enabled.

liufengyun · 2020-09-03T20:03:45Z

compiler/src/dotty/tools/dotc/util/LinearSet.scala

+ *  and as a HashSet for larger sizes.
+ */
+opaque type LinearSet[Elem >: Null <: AnyRef] =
+  immutable.Set[Elem] | HashSet[Elem]


Minor: LinearSet is not a great name in the context of data structures, linear associates closer to complexity theory than type theory :)

What would be a good other name?

odersky force-pushed the optimize-collections branch 3 times, most recently from 33eabb4 to 733f812 Compare August 30, 2020 10:51

odersky requested a review from liufengyun September 1, 2020 12:51

odersky assigned liufengyun Sep 1, 2020

odersky force-pushed the optimize-collections branch from e39b1a0 to 5232d5a Compare September 1, 2020 13:06

odersky force-pushed the optimize-collections branch from 5232d5a to 0f31e85 Compare September 1, 2020 13:14

smarter reviewed Sep 2, 2020

View reviewed changes

odersky force-pushed the optimize-collections branch from ac5f608 to 54d5466 Compare September 2, 2020 16:43

odersky added 5 commits September 2, 2020 18:57

Rename HashTable -> IdentityHashMap

ddc849b

Also, give it a more standard map interface. Also, fix removals in small tables

Generalize HashMap

cf671f8

Breakout specialized version for `eq` and `equals`. The duplicate some methods for efficiency.

Remove wrong comment

c48898f

Break out dual maps in Names#derived into its own LinearMap data type.

0e534d8

The reason for using a linear map instead of a mutable.Map here is that most derived instances are very small.

Rename Empty -> empty in SimpleIdentityMap

228aee7

It is already `empty` in SimpleIdentitySet. That way we use the same convention as in the stdlib. # Conflicts: # compiler/src/dotty/tools/dotc/util/LinearIdentityMap.scala

odersky added 12 commits September 2, 2020 18:57

Tuning of HashMap operations

435820a

- Increase load factor to 0.5 -- the miss rate seems to be still OK (around one miss per hit or better) while re-sizing costs and go down. - Don't lose lowest hashCode bit.

Implement NameTable in terms of HashSet

7cf9998

Reduce synchronized region to updates.

Correct comment

0918782

Avoid use @volatile in toTypeName

c8e13d2

Use IdentityHashMaps in pickler

799c088

Cache staticRef

712cb06

Per-run cache of what is returned for a staticRef

Drop bounds on HashMap Value type

8a5cb06

Make lookup return a Value | Null result instead, which makes it a bit safer to use. To support this well, forward port a casting method from the explicit-nulls branch.

Drop bounds on HashMap Key type

abeb4e3

Drop bounds on HashSet entry type

c75a555

Drop MutableSymbolMap as a separate class

fa7698e

It's now an alias of util.IdentityHashMap[Symbol, _]

Rename IdentityHashMap -> EqHashMap

b13f68e

Use a util.EqHashMap for BaseTypeCache

8a5d9a2

odersky force-pushed the optimize-collections branch from 54d5466 to e6597f3 Compare September 2, 2020 16:59

odersky added 2 commits September 3, 2020 09:36

Add ReadOnlySet

30f03e8

Convert more sets to util.HashSet

785a31b

odersky force-pushed the optimize-collections branch from e6597f3 to 785a31b Compare September 3, 2020 08:42

odersky added 2 commits September 3, 2020 10:45

Fix HashSet.-=

eebdcba

I overlooked a backport of a fix from HashMap to HashSet.

Convert sets in TreeChecker to util.HashSet

29ddc17

liufengyun approved these changes Sep 3, 2020

View reviewed changes

odersky added 2 commits September 3, 2020 22:28

Convert some mutable.HashMaps to util.HashMaps

57c0a43

Convert more mutable.HashMaps to util.HashMaps (1)

5e2d801

odersky merged commit b3f908d into scala:master Sep 3, 2020

odersky deleted the optimize-collections branch September 3, 2020 21:37

griggt mentioned this pull request Oct 20, 2020

Class scala defined twice (Ycheck:all) #5033

Closed

	inline def uncheckedNN: T = x.asInstanceOf[T]
	inline def uncheckedNotNull: T = x.asInstanceOf[T]

Use util Collections more widely #9680

Use util Collections more widely #9680

Uh oh!

Conversation

odersky commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Aug 30, 2020

Uh oh!

dottybot commented Aug 30, 2020

Uh oh!

dottybot commented Aug 30, 2020

Uh oh!

odersky commented Aug 30, 2020

Uh oh!

dottybot commented Aug 30, 2020

Uh oh!

dottybot commented Aug 30, 2020

Uh oh!

odersky commented Aug 30, 2020 • edited by liufengyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot commented Aug 31, 2020

Uh oh!

dottybot commented Aug 31, 2020

Uh oh!

odersky commented Sep 1, 2020

Uh oh!

liufengyun commented Sep 2, 2020

Uh oh!

dottybot commented Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dottybot commented Sep 2, 2020

Uh oh!

odersky commented Sep 2, 2020

Uh oh!

dottybot commented Sep 2, 2020

Uh oh!

dottybot commented Sep 2, 2020

Uh oh!

liufengyun commented Sep 3, 2020

Uh oh!

dottybot commented Sep 3, 2020

Uh oh!

dottybot commented Sep 3, 2020

Uh oh!

odersky commented Sep 3, 2020

Uh oh!

dottybot commented Sep 3, 2020

Uh oh!

dottybot commented Sep 3, 2020

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odersky commented Aug 29, 2020 •

edited

Loading

odersky commented Aug 30, 2020 •

edited by liufengyun

Loading