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.

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

Avoid some intermediate Lists #18572

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

lolgab
Copy link
Contributor

@lolgab lolgab commented Sep 19, 2023

No description provided.

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/MainGenericRunner.scala Outdated Show resolved Hide resolved
@@ -2132,7 +2132,7 @@ class Definitions {
if (!isInitialized) {
// force initialization of every symbol that is synthesized or hijacked by the compiler
val forced =
syntheticCoreClasses ++ syntheticCoreMethods ++ ScalaValueClasses() :+ JavaEnumClass
syntheticCoreClasses ::: syntheticCoreMethods ++ ScalaValueClasses() :+ JavaEnumClass
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is a bit strange. Maybe we could force them in a simpler way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I mechanically edited the code without realizing what it was used for.
I changed it to just reference the lists.
ScalaValueClasses() doesn't need to be initialized since it's not a lazy val, so I removed it. Let me know if I'm wrong.
989c961

@lolgab lolgab marked this pull request as ready for review September 19, 2023 13:56
@dwijnand
Copy link
Member

I don't think we should bother with this. List.++ calls directly calls concat, which is overridden in List to match the suffix (the argument) to List and then call :::. So we're removing that overhead, which is constant, so not worth it.

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Sep 19, 2023

I don't see an override for concat in List or its parents.

Oh, it goes through Seq.concat which calls appendedAll.

@lolgab lolgab force-pushed the avoid-plusplus-concatenate-lists branch from bf09c1e to c3377d4 Compare September 19, 2023 15:47
@lolgab lolgab changed the title Use ::: instead of ++ to concatenate Lists Avoid some intermediate Lists Sep 19, 2023
@plokhotnyuk
Copy link

How about replacing usage of List by ArraySeq (or just Array in method implementations) to reduce allocations during building and concatenations and for faster access during reading?

@lolgab
Copy link
Contributor Author

lolgab commented Sep 19, 2023

I dropped the change from ++ to ::: and left the only changes that seem to make sense now.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Looks fine now

The call to `apply` perform side effects that need to be executed
@nicolasstucki nicolasstucki merged commit b1fc943 into scala:main Sep 20, 2023
17 checks passed
@lolgab lolgab deleted the avoid-plusplus-concatenate-lists branch September 20, 2023 14:12
@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
Backports #18572 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
WojciechMazur added a commit that referenced this pull request Jun 20, 2024
Backports #18572 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants