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.

Sign up for GitHub

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

Jump to bottom

Make sure synthetic apply methods are generated in deterministic order #18210

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Make sure synthetic apply methods are generated in deterministic order #18210

merged 1 commit into from
Jul 18, 2023
+90 −22

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Jul 14, 2023

Fixes #17330

nicolasstucki
nicolasstucki reviewed Jul 14, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/core/Definitions.scala Outdated
Function1SpecializedReturnTypes

@tu lazy val Function1SpecializedParamClasses: PerRun[collection.Set[Symbol]] =
@tu lazy val Function1SpecializedParamClasses: PerRun[List[Symbol]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do rely on a fast contains operation on some of these.

Maybe we could type them as collection.Set and implement them with collection.mutable.LinkedHashSet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How large can those sets grow? Which is the largest one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those can indeed get relatively large, like I think 216 elements for Function2SpecializedApplyNames. I changed those back to Sets, AFAICS now lists are used for iterating over and sets are used for contains checks. I'm not so familiar with PerRun though, does that look OK?

@dwijnand
Copy link
Member

If you like this fix, you should put a test on it. Otherwise the next performance binge might rebreak it.

@raboof raboof force-pushed the deterministic-synthetic-apply-method-ordering branch 4 times, most recently from e7ea119 to 6d1ae5a Compare July 15, 2023 22:10
@raboof
Copy link
Contributor Author

raboof commented Jul 15, 2023

If you like this fix, you should put a test on it. Otherwise the next performance binge might rebreak it.

Fair ;) - it's a bit of an awkward thing to test, but SpecializeFunctionsTests seemed like a reasonable place for it.

nicolasstucki
nicolasstucki reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

compiler/src/dotty/tools/dotc/core/Definitions.scala Outdated Show resolved Hide resolved
@nicolasstucki nicolasstucki self-assigned this Jul 17, 2023
@raboof raboof force-pushed the deterministic-synthetic-apply-method-ordering branch from 6d1ae5a to 03a7ba5 Compare July 17, 2023 13:51
@nicolasstucki nicolasstucki merged commit e239b78 into scala:main Jul 18, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
@Kordyjan Kordyjan mentioned this pull request Dec 1, 2023
Merged
Kordyjan added a commit that referenced this pull request Dec 8, 2023
@Kordyjan
Backport "Make sure synthetic apply methods are generated in determin…
202e0d2 
…istic order" to LTS (#19118)

Backports #18210 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan modified the milestones: 3.4.0, 3.3.2 Dec 14, 2023
raboof added a commit to raboof/pekko that referenced this pull request Jan 31, 2024
@raboof
Update Scala 3 to 3.3.2
f39d657 
Follow-up on apache#855

Gets us the fix for scala/scala3#18210
@raboof raboof mentioned this pull request Jan 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki approved these changes

Assignees

@nicolasstucki nicolasstucki

Labels
None yet
Projects
None yet
Milestone
3.3.2
Development

Successfully merging this pull request may close these issues.

Order of 'mc' synthetic methods is not deterministic
4 participants
@raboof @dwijnand @nicolasstucki @Kordyjan