search

scala / collection-strawman

Implementation of the new Scala 2.13 Collections
Apache License 2.0
200 stars 72 forks source link

resolve status of `scala.collection.immutable.useBaseline` #572

Closed SethTisue closed 5 years ago

SethTisue commented 6 years ago

the new immutable.Set and immutable.Map implementations provide a scala.collection.immutable.useBaseline system property that let you flip back to the old code

if people notice regressions, especially performance regressions, when they move to M4, it might be nice to provide a switch they can flip to go back to the old Set and Map implementations to quickly see if that’s the culprit

whether we would want to support that all the way through M4->M5->RC1->final, I don’t know, probably not. ("I assume that in the final version of the collections only one hash-map/set will ship”, commented @msteindorfer at the time, at https://github.com/scala/collection-strawman/pull/342#issue-163115662)

SethTisue commented 6 years ago

(scala/scala#6623 makes it an environment variable instead of a system property, for Scala.js friendliness)

SethTisue commented 6 years ago

Sébastien notes that removing the old implementation in time for 2.13 final will save nearly 10% on Scala.js's baseline code size.

msteindorfer commented 6 years ago

@SethTisue the intention of immutable.Champ(HashSet|HashMap) is to replace the old immutable.(HashSet|HashMap) altogether. Since these collections are widely used, I introduced the flag for easy performance regression testing once collection-strawman is included in Scala 2.13.0-M4. As you already mentioned my plan was that the flag and the old collections get removed after people are using the new collections in milestone builds and get confidence using them in practice.

In my opinion the flag should definitely be removed before RC or final builds. Steps for removing the flag should include:

  1. delete immutable.(HashSet|HashMap)
  2. rename immutable.Champ(HashSet|HashMap) to immutable.(HashSet|HashMap)
  3. remove scala.collection.immutable.useBaseline flag

If yet performance regressions occur that cannot be easily solved by the Scala core team, please contact me and we will find a way to work it out together. (If at all, regressions should mostly stem from specializations of operations that may have been implemented in the old collections, but aren't yet specialized in the new ones.)

SethTisue commented 6 years ago

I suggest we leave it in M5 and then remove for RC1.

(M4 library publishing is proceeding rather slowly, so not many people have yet had the chance to test with real application code.)

SethTisue commented 6 years ago

I made a start on this at https://github.com/sethtisue/scala/tree/champ-is-the-champ, thinking it would be straightforward, but it turns out there are still tests referencing OldHashMap/OldHashSet, somebody needs to take a close look at those tests and decide whether to have them use the new structures instead, or just delete them as no longer relevant. volunteer?

SethTisue commented 5 years ago

scala/scala@5762002498cb9c6c1a973267df1056c949f4e6d5