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

Low-hanging perf improvements #10462

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Low-hanging perf improvements #10462

merged 3 commits into from
Jul 8, 2024

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Jul 5, 2024

Pull Request Description

  • avoid loading shapeless for the sole purpose of having a compile-time
    type inequality
  • don't use sys.env to avoid some Scala conversions
  • lazy initialization of fields

Important Notes

On a slow machine, so easier to spot.

Screenshot from 2024-07-05 16-15-06
Screenshot from 2024-07-05 18-14-53

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

hubertp added 2 commits July 5, 2024 18:25
- avoid loading shapeless for the sole purpose of having a compile-time
  type inequality
- don't use `sys.env` to avoid some Scala conversions
- lazy initialization of fields
@hubertp hubertp added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jul 5, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Removing a library dependency is always a good sign.


// https://stackoverflow.com/questions/6909053/enforce-type-difference

sealed class =!=[A, B]
Copy link
Member

Choose a reason for hiding this comment

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

Not that copying this code into our repository would simplify the magic...

@@ -2226,7 +2226,6 @@ lazy val `runtime-compiler` =
annotationProcSetting,
(Test / fork) := true,
libraryDependencies ++= Seq(
"com.chuusai" %% "shapeless" % shapelessVersion % "provided",
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any other usages of shapelessVersion. I guess:

val shapelessVersion        = "2.3.10"

should also be removed then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks

@@ -146,7 +145,7 @@ object IRPass {
*/
@throws[CompilerError]
def unsafeAs[T <: Metadata: ClassTag](implicit
@unused ev: T =:!= Metadata
@unused ev: T =!= Metadata
Copy link
Member

Choose a reason for hiding this comment

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

Is this a Scala way of doing Kotlin's reified classes?

Wouldn't it be simpler to just delete the method implicit magic altogether and just pass in the argument explicitly?

Btw. Why is the method called unsafeAs then? Reified classes can be checked during runtime in Kotlin and I see some runtime check here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sole purpose of this logic is to ensure at compile time that neither type inferencer nor explicit type argument result in T being Metadata rather than its subclass.

I don't think providing explicit argument to every usage site is better, plus it doesn't prevent incorrect usage this implicit prevents from.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jul 8, 2024
@mergify mergify bot merged commit 53eec66 into develop Jul 8, 2024
42 checks passed
@mergify mergify bot deleted the wip/hubert/startup-perf-nits branch July 8, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants