Replace Set with UniqList in memberNames

[dwijnand]

This is intended to be a fix for the filename-based behaviour change observed in the nightly java 8 failure #13878.

But I'm unable to recreate the problem because of a stupid "Cyclic Error" involving $throws.scala and object Predef...

But it's doing something reasonable, IMHO.

[test_java8]
[test_non_bootstrapped]
[test_windows_full]

[odersky]

Set is there for a reason. UniqList would introduce too many duplicates. We should not admit changes that have the potential for significant slowdowns, even if it is only in specific situations.

I am still not convinced we should fix it. And if we should we should have a very good argument that no performance could possibly be lost.

[dwijnand]

Duplicates how? I believe it to be behaving like Set, removing any duplicates. Perhaps the name UniqList imply something else to you? Do you have a better name for it?

[odersky]

So it removes the duplicates at linear cost. good. That's still unacceptable.

[nicolasstucki]

Can we use a LinkedHashSet?

[odersky]

It's hot code, so I would really like to understand what the problem is, exactly, before putting in a patch. I mean I worked like crazy to get 10-20% speedup. I don't want to see it slip away bit by bit (and no, you can't measure that individually, our benchmarks are not good enough for this).

…

On Mon, Nov 22, 2021 at 11:02 AM Nicolas Stucki ***@***.***> wrote: Can we use a LinkedHashSet? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#13973 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVUY3MOMYVKC5URBBQDUNIIMJANCNFSM5IHKJXDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

[dwijnand]

So there aren't many uses of the various duplicate-removing APIs (+ is only for RefinedType for example). I tried to do something reasonable in the construction, using a builder and calling distinct only at the end. Similarly we can revise &, | and filter.

The motivation is compiler determinism: avoid that the names of the input source files vary the order in which names are iterated (which is determined by the name hashes, but also from the order in which names are forced onto the names table). The difference in hash-based iteration order was one of the key contributors to the reoccurring nightly failures we had some 10 days ago.

I was initially worried that this set was heavily used for membership testing, but that didn't turn out to be true, so I went with a sequence-based structure. I care more about the intent than how it's done, so if a LinkedHashSet is preferred, I can look to switch to that, or any other ideas.

[dwijnand]

Perhaps I can keep it as an immutable Set, but hide it behind an opaque type so we can sort the names according to Name's Ordering on any iterating operations (foreach, iterator, etc)?

[dwijnand]

I guess that's similar to a SortedSet, except with the ordering only used for iteration rather than also storage.

[dwijnand]

Apart from another use of memberNames in RefChecks (for default methods), the main point in which names are iterated is in:

     def memberDenots(keepOnly: NameFilter, f: (Name, mutable.Buffer[SingleDenotation]) => Unit)(using Context): Seq[SingleDenotation] = {
       val buf = mutable.ListBuffer[SingleDenotation]()
-      for (name <- memberNames(keepOnly)) f(name, buf)
+      for (name <- memberNames(keepOnly).toList.sorted) f(name, buf)
       buf.toList
     }

But I think that's going to be objectively worst, as that sorting isn't even persisted...

[odersky]

SortedSet is also very inefficient. Since the original issue #13878 is already closed, let's just keep things as they are.